Darktable: histogram in tethering mode not displayed

[TurboGit]

Describe the bug

No histogram displayed in tethering view.

To Reproduce

1. Go to tethering view
2. capture a picture, it is properly displayed in center area
3. the histogram is not updated

Expected behavior

Histogram should be displayed.

Platform (please complete the following information):

• Darktable Version: 2.6 & 3.0 & current master
• OS: Linux Debian

Additional context

Was broken quite some time ago as it does not work on 2.6.

[TurboGit]

@dtorop : Is that something you could have a look at as you've been working on the histogram recently?

[dtorop]

@TurboGit I'll take a look at it. It may take a week or so. But sounds like if it's been broken since v2.6, that it can wait that long...

[TurboGit]

@dtorop : Sure nothing urgent ! Thanks for looking at it.

[github-actions[bot]]

This issue did not get any activity in the past 30 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[johnny-bit]

I think this issue also has wrong scope ;) it's not "CLI" related...

[TurboGit]

@dtorop : Any news ? Do you think you could have a look before feature freeze in 10 days ?

[TurboGit]

@johnny-bit : Right, changed from CLI to UI of course !

[dtorop]

@TurboGit I'll look at it this weekend! I don't know if I actually have a camera here which can tether, but at the worst I can look at the code.

[dtorop]

@TurboGit My camera doesn't work with tethering. I have a line on a Canon which might do better. Otherwise I'll have to wait until things open up a bit around here before I can borrow a camera which can tether. (Maybe there's a way to fake a camera device, meantime, but it doesn't seem worth the trouble.)

At a quick look at the code makes me wonder if some signal from a background job which used to tell the histogram to update is no longer getting through, or is no longer being noticed. That's as far as I've gotten.

[johnny-bit]

Huh... that thing with signal not getting thought or something might be on some track here. Whenever I opened tethering view to test #5371 i got:

[image_cache_allocate] failed to open image 4294967295 from database: no more rows available

[TurboGit]

@dtorop : I'm pretty sure some signal are missing. Do you know which one to look for ?

[dtorop]

Histogram listens for DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED. Maybe it should also listen for DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED when it is in tethering view?

The catch is that histograms are currently only calculated for the preview pipe. And is only the full pixelpipe calculated in tethering view? What if the libs/navigations.c were enabled for tethering view? Would this cause the preview pipe to run and hence update the histogram? I know that's not the real solution, but I'd be curious.

[dtorop]

@johnny-bit

Whenever I opened tethering view to test #5371 i got:

[image_cache_allocate] failed to open image 4294967295 from database: no more rows available

I just saw this in darkroom view. See #5398 for how to reproduce it.

[dtorop]

Presumably the "failed to open" message is from here: https://github.com/darktable-org/darktable/blob/f6842cc0d100eb44079394566ab07558259fd9c6/src/common/image_cache.c#L153-L154

[TurboGit]

Huh... that thing with signal not getting thought or something might be on some track here. Whenever I opened tethering view to test #5371 i got:

[image_cache_allocate] failed to open image 4294967295 from database: no more rows available

This seems to happen when no image are selected on the lighttable when moving to tethering mode. This another issue independent of the histogram.

[TurboGit]

What if the libs/navigations.c were enabled for tethering view? Would this cause the preview pipe to run and hence update the histogram? I know that's not the real solution, but I'd be curious.

Tried that, no luck. The navigation window stays grey, no information at all. So it seems the preview is not even happening when in tethering mode.

[dtorop]

The navigation window stays grey, no information at all. So it seems the preview is not even happening when in tethering mode.

I'll take a look at the code. Worst case, I guess it's possible just to disable histogram in tethering in 3.2 until the bug is fixed.

[dtorop]

It looks like the various hooks/mechanics to make histogram work in tethering long since vanished.

In darkroom mode, things work: The preview pixelpipe calculates the needed histogram and stores it in darktable.develop. The histogram module then catches DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED and displays the histogram data from darktable.develop.

In tether view, when an image is activated (either latest tethered shot or an image manually picked from the filmstrip), DT_SIGNAL_DEVELOP_MIPMAP_UPDATED is raised. At that point we know the imgid of the image which needs the histogram, but that's it.

It should be possible to call dt_mipmap_cache_get_matching_size() and dt_mipmap_cache_get() with DT_MIPMAP_BLOCKING to get a lo-res version of the current image. Then one could calculate the histogram of that image and display it.

The catch is that the histogram-calculation code is currently woven into pixelpipe_hb.c. Some code (especially for waveform) would need to be pulled over into common/histogram.c.

Open questions:

• Should lib/histogram.c do all the work of catching DT_SIGNAL_DEVELOP_MIPMAP_UPDATED and constructing the histogram? Or would views/tethering.c do this work, then send a signal (something new, like DT_SIGNAL_CONTROL_HISTOGRAM_REDRAW) to the histogram module when the histogram was ready?
• The histogram buffers are stored in darktable.develop. This makes sense for darkroom view but not for tethering view (unless tethering view generated and ran a new preview pixelpipe by hand). Should the histogram buffers end up in dt_lib_histogram_t? Then the histogram module would get a callback from the current view whenever an image needed a histogram calculated, or needed to store the result of a calculation. Or would there be separate buffers in the tethering view, which the histogram module would have to know how to find? Or -- probably even worse -- should there be some sort of global histogram buffer pointed to via darktable_t?
• How will this all interact with the histogram color profile? Where should the code go which handles that conversion? Currently it's in _pixelpipe_final_histogram() in pixelpipe_hb.c. (Though there's no colorspace conversion for waveform histograms!) What color profile is the result of dt_mipmap_cache_get()? Is the preview pixelpipe data (before being converted to histogram profile) in the display color profile?
• How important is a histogram in tethering mode? Presumably the photographer would use it to make sure they weren't clipping exposure, just as one would use the camera's own histogram. But is the photographer getting the benefit of a custom pixelpipe? I'm unclear on this...
• Would there be more tricks needed to make the histogram work in live view?

Despite a bunch of awkward things, it does look like it'd be possible to retrofit the code, and the end result might even be tidier. I'd work on this, but don't know if we're way past when it would get into the 3.2 release. Would it be better to make a small commit to turn off histogram in tethering for now, then work on a real fix?

Incidentally, I still don't have a camera which can tether. I realized that I could turn on histogram in print view, which should be a way to test this. For a long time I've wished that I could see a histogram of a selected image in lighttable. If histogram is able to work with the mipmap of a current image, this might become possible.

[TurboGit]

Would it be better to make a small commit to turn off histogram in tethering for now, then work on a real fix?

I'll do that yes. As this is a bugfix we can reintroduce the histogram into 3.2 release if not too intrusive otherwise it can wait for 3.4. No pressure on this, that's a nice feature but certainly not something main stream as few people are using the tethered mode.

[TurboGit]

For a long time I've wished that I could see a histogram of a selected image in lighttable. If histogram is able to work with the mipmap of a current image, this might become possible.

Sounds nice indeed!

[dtorop]

@TurboGit All sounds good. I've got a pretty clear sense of what needs to happen, but it'll take a bunch of refactoring. I'll be careful, but it could be a risky change right before a release.

Another question will be what to do about the draggable exposure areas on the histogram -- can a non-darkroom view sustain altering/redisplaying an image. It'd be easy to make non-darkroom histogram non-draggable, of course. But I like the thought of being able to do basic exposure work without going into darkroom.

Histogram (and tethering) look to have both been around for about a decade, and they haven't necessarily received significant attention as the code all around them got revamped. So it's about time to look them over.

[johnny-bit]

Depending on how deep you can/are willing to go... I still would be hesitant about histogram being able to modify anything in non-darkroom view, it should IMO be preview at most :)

[dtorop]

I still would be hesitant about histogram being able to modify anything in non-darkroom view, it should IMO be preview at most

There's some precedent for image modification in lighttable: the image module modifies orientation, and it's also possible to erase/refresh/paste history and apply styles. There's definitely a good bit of mechanics to make it work... Flipping at least happens via the dt_control_add_job() mechanism.

The histogram exposure changes does its work via dt_dev_proxy_exposure_t, which is pretty bound with the develop module. It'd take a bit of work to make this work when not in darkroom view, and I'm not clear if changing exposure via the jobs mechanism would make the control sufficiently responsive -- though the current mechanism isn't ideal either on slow machines.

[TurboGit]

Another question will be what to do about the draggable exposure areas on the histogram -- can a non-darkroom view sustain altering/redisplaying an image.

I would say no, this should be disable on tethering mode.

[TurboGit]

Would it be better to make a small commit to turn off histogram in tethering for now, then work on a real fix?

Now done.

[TurboGit]

I'll reclassify this for 3.4.

[dtorop]

There's some more work on this here: https://github.com/darktable-org/darktable/compare/master...dtorop:tether-histogram-fix

The good news is that this branch can display a histogram in print view. I know that's not the goal, but I still don't have a camera which can tether, so this is an approximation for testing. I'm curious how it works in tether view.

The bad news is that the histogram doesn't necessarily match that in develop view, either in information or quality. Outside of develop, the histogram is generally made from 8-bit clamped data via the mipmap cache. I'll work on fixing this. In the case of tether, histogram should be able to get data directly from the pixelpipe when an image is made, but the data may be superseded by the 8-bit data. And if the user flips between images on the filmstrip, they'll generally get the 8-bit histogram from the cache.

Here's an extreme example, using waveform, for an overexposed raw file. Darkroom view histogram:

Same image, print view histogram:

Also, the current commit history is messy, with lots of blind alleys. I'll clean that up.

The changes:

• Pull histogram processing out of the pixelpipe_hb.c, and into lib/histogram.c.
• Store histogram data in dt_lib_histogram_t rather than dt_develop_t
• Make proxy hooks in dt_lib_t to let the pixelpipe send data to be processed, and provide information about histogram display to iops.
• Disable the code which makes the colorpicker state alter the histogram -- this is vestigial and never activates, though it could be nice to re-enable.
• In darkroom view, about the same should happen as always: a histogram is made of the preview pixelpipe data before gamma, and once the preview pixelpipe is done, the histogram is redrawn via a signal.
• Outside darkroom view: Best case, the histogram sees that the pixelpipe is processing data for the center view, and makes a histogram from that data. If the center view is updated via a cache, the histogram requests the cached 8-bit image and makes a histogram from that. The histogram may also throw away the good float data and use the 8-bit data without warning.

Also, as always, the "regular" histogram colorspace is managed, but the waveform histogram is always output in display colorspace. I would like to fix this.

[dtorop]

Also, I believe that the histogram is now -- erroneously -- flipping the red/blue channels when it works from the 8-bit cached data! Something else to fix...

[dtorop]

One more note -- I think the better solution may be to construct/run a preview pipe to generate histograms in tether mode. (And possibly even use the existing develop preview pipe.) Then it's just a matter of catching appropriate signals when an image has arrived from gphoto or the filmstrip changes. Either case would start the preview pipe. No other code refactoring would be needed. I'm not sure how this would work in live view -- I haven't looked at that code enough.

So far no luck on a working version of this, but more in a bit.

[dtorop]

Update: constructing/running a pixelpipe to generate histograms should now work, at least for print view (in which I'm testing, due to not having a tetherable camera). It's possible to write pretty straightforward, where the darkroom and non-darkroom cases are quite similar. I don't think there's too much overhead by this method, and I think this is the "right" way to do this.

There's a working version at https://github.com/darktable-org/darktable/compare/master...dtorop:tether-histogram-fix, still with a very messy commit history, debugging printfs, and more cleanup/fixes to do. If anyone has a camera which can tether, and time to test, I'd be curious if it works in tether view, and when using live view. I've particularly spent almost no time thinking about the latter.

The bulk of the code changes in https://github.com/dtorop/darktable/commits/tether-histogram-fix are to make lib/histogram.c rather than develop/pixelpipe allocate and process the final histogram. I'm pretty sure that this is actually orthogonal to the work to make the histogram display outside of darkroom view. It might make a PR easier to handle if I pull out only the latter changes.

[johnny-bit]

Since #5714 landed, this issue may be closed, right? :)

[dtorop]

Since #5714 landed, this issue may be closed, right? :)

I hope so! It's been a while...

[TurboGit]

Yep, can be closed !