Improvements to change main histogram to 65535 bins

[Thanatomanic]

Following the discussion here (#5899), I made a branch histogram-improved to work on implementing a more accurate main histogram. The initial commit simply upscales the number of bins naively and spreads out values over all bins. The three main consequences of this:

• The RAW histogram now looks much more like the one in RawDigger 👍
• Viewing the histogram is dead slow 👎
• There are visual artifacts due to many empty bins 👎

About these visual artifacts. For example, the histogram of the working image is constructed from an 8-bit Image8 source. I now simply spread out the 265 pixel values over all 65536 bins, so many bins remain empty. This leaves a lot of empty room between lines. I already changed the drawing mode to single vertical lines, instead of connected lines, but I am still thinking about how this could be improved.

Expect more changes in the upcoming week(s). Suggestions are highly appreciated.

[Thanatomanic]

Sample shot of RAW histogram:

Same shot, but now the log-linear regular histogram:

[Lawrence37]

The new histogram looks much more useful.

One think I immediately noticed is that the green histogram covers the red, and the blue covers red and green. Would it be possible to compose the individual histograms to make all of them more visible?

[Thanatomanic]

@Lawrence37 What do you mean exactly by 'to compose'? Put the separate RGB histograms next to each other, or on top of each other, instead of overlapping? I'm not sure that would be very useful considering the relatively small scale of the histogram...
One thing that I could do is to reduce the opacity of the lines, but then you get mix colors as well.

This is one of the points that are currently less than optimal. It will probably only really improve if there is a better way to connect the points, instead of drawing lines from the axis. For that I need a way to skip empty bins or something smarter, like dynamic binning depending on the width of the histogram.

[afr-e]

An alternative to opacity is to introduce a pattern instead of a solid fill.

[Lawrence37]

I mean blending the histograms. darktable, for example, uses additive blending. Some people find it confusing though. Using the old line style avoids the issue, but I have a feeling that it's much harder to implement than it sounds.

[Thanatomanic]

@afr-e What kind of pattern would you propose?

Side-note: currently, drawing 65536 lines per histogram is slow, but I believe that drawing a polygon and filling it, is virtually unworkable (the L channel used to use a filling mode, but it took 20 seconds when you enabled it, and then it didn't draw anything).

[Thanatomanic]

@Lawrence37 I experimented once with additive blending here, but there was some opposition.

[heckflosse]

Viewing the histogram is dead slow

I am willing to help on this

[heckflosse]

@Thanatomanic Can you point me to the code which causes the dead slowdown?

[jackho9]

Excellent Roel, I like the top log-log raw histogram, I can clearly see if/how much specular clipping there is (a little bit), where reflected exposure actually begins (less than a stop from clipping) and if/how many blocked pixels there are (a few).

What are the units of the two axes? Ideally gradation every stop, with 1/3EV just in the highlights, some indication of where mid-gray could be (EV0, -3EV from clipping?). I personally do not mind a toothy histogram, though you could widen the bars in the deep shadows if you felt that was more readable. There is a lot of information there so readability is important even when the histogram is small, I find it a bit hard to see the blocked/clipped histogram 'bars' against the dark background.

One of the linearX-logY histogram's main uses for me is to nail down how close To The Right reflected Exposure ended up, so it would be useful to have gradation every thousand or two on the X axis. I personally practically never use the linear-linear histogram view.

As for vertical log axis units, If we take something of the order of 0.01% as a typical acceptable number of clipped/blocked pixels, the axis should show clearly whether a few, a few hundred, thousand or tens of thousand pixels are clipped/blocked. I see that RD shows gradation at 1, 10, 50, 200, 1k, 5k, 20k, 100k, 500k etc., not bad.

As for speed, the raw histogram(s) needs to be computed once only, and it doesn't necessarily need to be the first thing to come up. Anyways, just one user's usage, I hope others will contribute theirs. Thanks a lot for doing this!

Jack

[heckflosse]

@Thanatomanic
Here's the quick hack patch (still without using vectorized xlogf)

diff --git a/rtgui/histogrampanel.cc b/rtgui/histogrampanel.cc
index 6331e8364..77d13670d 100644
--- a/rtgui/histogrampanel.cc
+++ b/rtgui/histogrampanel.cc
@@ -25,6 +25,9 @@
 #include "../rtengine/LUT.h"
 #include "rtimage.h"
 #include "../rtengine/color.h"
+#include "../rtengine/sleef.h"
+#define BENCHMARK
+#include "../rtengine/StopWatch.h"
 
 using namespace rtengine;
 
@@ -365,10 +368,16 @@ void HistogramPanel::toggleButtonMode ()
 //
 //
 // HistogramScaling
-double HistogramScaling::log(double vsize, double val)
+float HistogramScaling::log(float vsize, float val) const
 {
     //double factor = 10.0; // can be tuned if necessary - higher is flatter curve
-    return vsize * std::log(factor / (factor + val)) / std::log(factor / (factor + vsize));
+    return vsize * xlogf(factor / (factor + val)) / xlogf(factor / (factor + vsize));
+}
+
+float HistogramScaling::log_test(float vsize, float val, float mult) const
+{
+    //double factor = 10.0; // can be tuned if necessary - higher is flatter curve
+    return vsize * xlogf(factor / (factor + val)) * mult;
 }
 
 //
@@ -958,22 +967,25 @@ void HistogramArea::on_realize ()
 void HistogramArea::drawCurve(Cairo::RefPtr<Cairo::Context> &cr,
                               const LUTu & data, double scale, int hsize, int vsize)
 {
+    BENCHFUNMICRO
     double s = RTScalable::getScale();
 
     cr->set_line_width(s);
     //cr->move_to (padding, vsize - 1);
     scale = scale <= 0.0 ? 0.001 : scale; // avoid division by zero and negative values
 
+    const float temp1 = 1.f / xlogf(HistogramScaling::factor / (HistogramScaling::factor + vsize));
+    const float temp2 = 1.f / xlogf(HistogramScaling::factor / (HistogramScaling::factor + 65535.0));
     for (int i = 0; i < 65536; i++) {
         double val = data[i] * (double)vsize / scale;
 
         if (drawMode > 0) { // scale y for single and double log-scale
-            val = HistogramScaling::log ((double)vsize, val);
+            val = HistogramScaling::log_test (vsize, val, temp1);
         }
 
         double iscaled = i;
         if (drawMode == 2) { // scale x for double log-scale
-            iscaled = HistogramScaling::log (65535.0, (double)i);
+            iscaled = HistogramScaling::log_test (65535.f, i, temp2);
         }
 
         double posX = padding + iscaled * (hsize - padding * 2.0) / 65535.0;
diff --git a/rtgui/histogrampanel.h b/rtgui/histogrampanel.h
index 740b0a12c..497a0c3aa 100644
--- a/rtgui/histogrampanel.h
+++ b/rtgui/histogrampanel.h
@@ -49,9 +49,10 @@ struct HistogramRGBAreaIdleHelper {
 class HistogramScaling
 {
 public:
-    double factor;
-    HistogramScaling() : factor(10.0) {}
-    double log (double vsize, double val);
+    float factor;
+    HistogramScaling() : factor(10.f) {}
+    float log (float vsize, float val) const;
+    float log_test (float vsize, float val, float mult) const;
 };
 
 class HistogramRGBArea final : public Gtk::DrawingArea, public BackBuffer, private HistogramScaling, public rtengine::NonCopyable

[heckflosse]

@Thanatomanic
I pushed the speedup for log-Histogram. Currently I kept the timing code. I will remove it later, when the SSE speedup is done.

[afr-e]

@afr-e What kind of pattern would you propose?

I was thinking of the classic Morse code for lines, and stippling and hatching for area under the curve, a different pattern for each channel. However, care must be taken so as not to disturb or distract the user.

---

General thoughts

Histograms are meant to help the user glean as much useful info as possible. In my opinion, that can (and often should) mean that you don't necessarily have to represent the data exactly; i.e. it should be represented symbolically and emphasize important elements. E.g. the outlier lines are not only difficult to read because of the dark background as @jackho9 noted but because they are thin. Simply putting a dot on top of those lines and highlighting them (persistently or on hover) would make them more apparent and recognizable.

I tend to be brimming with ideas, so feel free to ask me for more feedback. 😉

[heckflosse]

@Thanatomanic With the SSE code for the log mode I'm now at ~5ms per color from which ~4.5ms are for drawing the lines. I think, that's fine now.

Interestingly, latest clang auto vectorizes the two loops, but latest gcc does not...

[Thanatomanic]

@heckflosse Thank you, these speed ups make the performance return to how it was before the change. Only when you show the RAW histogram for huge files (100 MP), and drag to change the factor, you have a very slow response. But that's perfectly fine for me.

[heckflosse]

@Thanatomanic

Only when you show the RAW histogram for huge files (100 MP), and drag to change the factor, you have a very slow response.

I do not understand why the MP count of a file has an influence on the response time when dragging.
Edit: But I can confirm the behaviour...

Can you elaborate on this?

[jackho9]

@heckflosse Thank you, these speed ups make the performance return to how it was before the change. Only when you show the RAW histogram for huge files (100 MP), and drag to change the factor, you have a very slow response. But that's perfectly fine for me.

Yes, thank you. Question: is the factor necessary?

[Thanatomanic]

@heckflosse I've timed both drawCurve and updateBackBuffer. The first remains mostly constant no matter what the file and hovers at 3-5 ms. This makes sense, since it needs to process the same number of bins.
The second function is different. It should probably take at least 3×5 ms because of its calls to drawCurve, but actually it takes anything between 40 ms an 500 ms depending on the bit-depth and resolution of the image. I see no obvious clue as to why...
I'll think about this.

@jackho9 I think I implemented the flexible factor a few years back to have some form of 'zoom' functionality in the histogram and allow you to choose which area to look for details. Maybe that is no longer necessary because the histogram now actually renders as it should and more details are visible anyhow.

As for the axes: the x-axis divisions need some work, fully agreed on that. I'll see what I can do for a proper EV scale.
The y-axis is simply a linear division into equal parts. If you see three lines, they are at 25%, 50% and 75%. If you see 15 lines, they are at 6,25%, 12,5%, etc. I don't intend to make this any more complicated, unless there is a very good reason to.
I think that checking for clipping for 0,01% is pretty arbitrary, but maybe it would be cool to have a movable line so you can set clipping levels yourself? (wild idea.. )

The more pressing matter for me is to get a nicer non-raw histogram that looks smoother than it does now. And I just got an idea on how to do that 😄

[afr-e]

One thing that I suggested for PhotoFlow that may be applicable here is that it would be nice to have labels for the axes and lines.

I think that checking for clipping for 0,01% is pretty arbitrary, but maybe it would be cool to have a movable line so you can set clipping levels yourself? (wild idea.. )

Not wild. At the very least, if there is an indication of the clipping on the graph, I would be happy.

[Thanatomanic]

@heckflosse I think that drawCurve does not actually draw the curve, but only prepares the lines to draw. The calls to cr->stroke() in updateBackBuffer are the actual Cairo command to start drawing. This explains the timing difference between the two functions.

It does not explain why the drawing scales with the resolution of the image. What does factor in, is what @Lawrence37 mentioned before:

If each line averages 50 pixels long, that's 65536 x 50 x 3 = about 10 million pixels of line for the raw histogram. That's a lot of line...

So, the more the lines need drawing, the slower it gets. A histogram with a lot of variation would be slower than a flat one. That still doesn't quite explain why the time scales with megapixels...

[afr-e]

Would it have to do with the calculations that generate the histogram?

[Thanatomanic]

@heckflosse First step fixed I believe. I'm quite happy I figured this out on my own eventually, because there is so much going on with derived classes and object types going around that I found it quite difficult to pinpoint where the changes were needed... This exposes the tiff_bps property from dcraw nicely. It is 0 when we're dealing with a non-raw file, so that still needs fixing to handle higher bitdepth non-raw images.
Next step is to elegantly forward this to the histogram class.

diff --git a/rtengine/imagesource.h b/rtengine/imagesource.h
index 25c024ed2..d92fd56bf 100644
--- a/rtengine/imagesource.h
+++ b/rtengine/imagesource.h
@@ -123,6 +123,10 @@ public:
     virtual void        WBauto(double &tempref, double &greenref, array2D<float> &redloc, array2D<float> &greenloc, array2D<float> &blueloc, int bfw, int bfh, double &avg_rm, double &avg_gm, double &avg_bm, double &tempitc, double &greenitc, float &studgood, bool &twotimes, const procparams::WBParams & wbpar, int begx, int begy, int yEn, int xEn, int cx, int cy, const procparams::ColorManagementParams &cmp, const procparams::RAWParams &raw) = 0;
     virtual void        getrgbloc(int begx, int begy, int yEn, int xEn, int cx, int cy, int bf_h, int bf_w) = 0;
 
+    virtual unsigned int getBitdepth() const
+    {
+        return 0;
+    }
     virtual double      getDefGain  () const
     {
         return 1.0;
diff --git a/rtengine/rawimage.h b/rtengine/rawimage.h
index 871267dac..41b32f960 100644
--- a/rtengine/rawimage.h
+++ b/rtengine/rawimage.h
@@ -109,6 +109,11 @@ public:
         return fuji_width;
     }
 
+    unsigned int get_bitdepth() const
+    {
+        return tiff_bps;
+    }
+
     float const * get_FloatRawImage() const
     {
         return float_raw_image;
diff --git a/rtengine/rawimagesource.cc b/rtengine/rawimagesource.cc
index 7ae919e0a..9c1dd496f 100644
--- a/rtengine/rawimagesource.cc
+++ b/rtengine/rawimagesource.cc
@@ -6091,6 +6091,11 @@ ColorTemp RawImageSource::getSpotWB (std::vector<Coord2D> &red, std::vector<Coor
     }
 }
 
+unsigned int RawImageSource::getBitdepth() const
+{
+    return ri->get_bitdepth();
+}
+
 void RawImageSource::transformPosition (int x, int y, int tran, int& ttx, int& tty)
 {
 
diff --git a/rtengine/rawimagesource.h b/rtengine/rawimagesource.h
index 16677b1da..064e76da2 100644
--- a/rtengine/rawimagesource.h
+++ b/rtengine/rawimagesource.h
@@ -158,6 +158,7 @@ public:
     {
         return rawData;
     }
+    unsigned int getBitdepth() const override;
 
     double      getDefGain  () const override
     {

[heckflosse]

It is 0 when we're deadline with a non-raw file, so that still needs fixing to handle higher bitdepth non-raw images.

ImageIO::getTIFFSampleFormat

[lgtm-com]

This pull request introduces 2 alerts when merging a11dffd into d02ed52 - view on LGTM.com

new alerts:

• 2 for Multiplication result converted to larger type

[lgtm-com]

This pull request introduces 2 alerts when merging 65ff20a into d02ed52 - view on LGTM.com

new alerts:

• 2 for Multiplication result converted to larger type

[Thanatomanic]

@heckflosse @Lawrence37 @afr-e @Desmis @Beep6581 Finally I have made good progress today, up to the point that I believe this is ready for finetuning, testing and a merge. Your feedback would be much appreciated, in particular on whether you like the look or how it could be improved.

Biggest changes since the last time I worked on this, is that the histogram now knows what the bit depth of the raw image is. The gridlines and histogram are adjusted accordingly. You can easily check the difference by comparing raw images with e.g. 12 bit and 16 bit. The normal histogram is always 8 bit and is now scaled properly again (it was stretched into 65536 bins before...).
You will see three vertical white lines: one for the lowest value, one for the (arbitrary) 0EV line and one for the highest value.

Thanks for taking a look!

[Thanatomanic]

I just spotted that the RGB readout bar is not working in all cases. I'll take a look in a few days when I have some time again.

[lgtm-com]

This pull request introduces 2 alerts when merging 2e58bae into d02ed52 - view on LGTM.com

new alerts:

• 2 for Multiplication result converted to larger type

[Lawrence37]

Some observations:

• Horizontal lines in the histograms, RGB parade, and waveform are gone.
• Switching to a vectorscope with no image open causes a crash.
• The fact that blue hides green and green hides red makes the histogram harder to use. We've discussed this before and I think it is important to resolve before merging.
• I agree the 0 EV line is arbitrary. What is the goal of this line? Is it for middle grey? If so, do we use 12%, 12.5% (-3 EV), 18%, or 18.4% (Lab L=50)?

[Thanatomanic]

@Lawrence37

• The RGB bar readout should be working now
• The horizontal gridlines should be back, but need work. They don't always work correctly depending on the mode. TODO
• I'll investigate the crash, not sure what caused it... Inadvertently fixed below
• The scopes scale weirdly compared to dev. Try enlaring the histogram to see for yourself. Fixed below
• Any suggestion for the visual? For the regular histogram, I could reintroduce the lineplot instead of the filled plot.
• A middle-grey line was suggested before, I took the RawDigger approach and put it at -3 stops from maximum. I would actually prefer the L = 50% approach and put it at 18,42% of the maximum raw value. But opinions are welcome.

[Lawrence37]

@Thanatomanic
Was there a reason for not using the old style lines for the histograms? I though I read an argument against it but I couldn't find it.
It occurred to me that the middle gray line for the normal histogram is not correct because there is a hard-coded sRGB gamma. The correct placement should be closer to the middle using a linear x-axis. I also think using L=50 is a reasonable choice.

[Thanatomanic]

@Lawrence37 The primary reason for not using the lines was that prior to my bitdepth changes, there could be a lot of empty bins, making the line go up and down a lot and messing up the visualization. I will try out how it will look now.
Another, more fundamental, argument is that connecting the dots implies something is known about the values in between points. For the regular histogram this is fine, for the RAW histogram it is not.

Good point about the sRGB middle gray.

[Lawrence37]

Another idea is to only plot points. The down side is that they will be hard to see on the left side because they are spaced apart.

[Thanatomanic]

I have tried to re-introduce the old line mode and it works well for the normal histogram, but for the raw histogram things look pretty terrible:

The problem is that the really raw sensor data are rescaled according to the black and white levels. Because the histogram has integer bins, this leads to quantization errors. I see three possible ways out of this:

1. We provide the really raw data in the histogram without scaling. Downside is that you would need to understand black and white levels in order to interpret the histogram.
2. We do the scaling and adapt the histogram width to the actual width of the data (instead of mapping the data to the available width). This would give you a histogram that grows and shrinks whenever you play with the raw black and white level controls. Getting the gridlines on the right locations is going to be tricky.
3. We leave things as they are, but don't use the old line graph mode, but the newer 'bar chart' mode.

I think the 2nd option is the nicest, but also the most work (I think). The 1st option is not very useful, except to analyse the actual sensor data. The 3rd option is easy to implement but the quantization errors remain.

Thoughts? Perhaps I should pose this question on Pixls for some wider feedback.

[Lawrence37]

I don't quite understand what the second option means so I apologize if this is the same as your idea. My approach would be this:
Bin the raw data after black point adjustment. Draw the histogram such that the white point touches the right side. The gridlines stay where they are now.

[Thanatomanic]

So, let's assume you have a 16 bit file, with a black point of 512 and a white point of 65400. Currently we subtract 512 from all sensor values and then rescale so that (65400 - 512) becomes 65535.
Edit: reworded parts...

How would your approach differ?

[Lawrence37]

After black point subtraction, the binning occurs. There are 64889 bins that may have values (bins number 0 to 64888) so we plot a line using 64889 points. On the linear histogram, bin 0 is on the left edge and bin 64888 is on the right edge. On the log histogram, bin 64888 is on the right edge. Bin 32444 is on the first EV line from the right.

[Beep6581]

The 1st option is not very useful, except to analyse the actual sensor data

That makes it very useful to me. I need to see two things in my work:

1. the raw histogram, to help find the right values for camconst.json,
2. and the final histogram (end of pipeline).
3. Additionally, anyone who uses the raw black and white point correction sliders would probably want to see a histogram of the data just after those corrections. Maybe this does not need a histogram of its own though, because maybe its enough to apply the neutral profile and then to do those corrections while viewing the final histogram.