Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graphical glitch in the histogram while color picking (rgb module) #11003

Closed
c-h-r-i-s-t-i-a-n opened this issue Jan 26, 2022 · 47 comments · Fixed by #14166
Closed

Graphical glitch in the histogram while color picking (rgb module) #11003

c-h-r-i-s-t-i-a-n opened this issue Jan 26, 2022 · 47 comments · Fixed by #14166
Labels
bug: pending someone needs to start working on that reproduce: peculiar the bug seems to affect only a specific target and cannot be reproduced elsewhere scope: image processing correcting pixels scope: UI user interface and interactions understood: incomplete devs lack some important info to start fixing

Comments

@c-h-r-i-s-t-i-a-n
Copy link

c-h-r-i-s-t-i-a-n commented Jan 26, 2022

I'm getting graphical glitches in the histogram while color picking in the rgb curve module.

Reproduce

Open the RAW file from this thread:
https://discuss.pixls.us/t/darktables-filmic-rgb-defaults-render-this-desaturated/29068

with this xmp file (sorry for zip, xmp is not allowed for upload):
P1200988.RW2.zip

Go to the rgb curve module, select the color picker and drag the cross around in the picture.
Now the graphical glitch is appearing occasionally within the histogram (see screenshot).

Screenshots

Bild1

Platform

  • darktable version : 3.8.0
  • OS : Win10 64bit
  • Memory : 16 GB
  • Graphics card : Intel/Nvidia
  • Graphics driver :
  • OpenCL installed : yes
  • OpenCL activated : no
@c-h-r-i-s-t-i-a-n
Copy link
Author

In the vectorscope display the glitch is appearing as a little speck.

Bild2

@c-h-r-i-s-t-i-a-n
Copy link
Author

Maybe it's related to demosaic RCD? When I switch to another method the glitch is gone.
Also turning on demosaic color smoothing seems to help.

@Nilvus
Copy link
Contributor

Nilvus commented Jan 27, 2022

@jenshannoschwalm: if this could be related to RCD demosaicer, maybe your view could be helpful here.

@Nilvus Nilvus added scope: image processing correcting pixels scope: UI user interface and interactions bug: pending someone needs to start working on that labels Jan 27, 2022
@jenshannoschwalm
Copy link
Collaborator

I will have a look tonight

@jenshannoschwalm
Copy link
Collaborator

@c-h-r-i-s-t-i-a-n
before me looking into this deeper (i tried this morning for a short time and couldn't trigger the issue yet), a few answered questions will help

  1. what setting are you using in preferences->pixel interpolator (scaling)? What you see could be the result of using lanczos3.
  2. It seems you are taking histogram data in point mode. Does the issue persist in area mode?
  3. Do you see a difference between OpenCL on/off?
  4. Is the issue persisting if you switch off modules that might correct chromatic aberration (lens ...)? I ask this because such low-level artefacts could also produced here by lensfun algo as can be regularly seen while working on monochrome images.

@Nilvus Nilvus added understood: incomplete devs lack some important info to start fixing reproduce: peculiar the bug seems to affect only a specific target and cannot be reproduced elsewhere labels Jan 28, 2022
@c-h-r-i-s-t-i-a-n
Copy link
Author

  1. Also occurs with other interpolator methods like bilinear, bicubic.
  2. issue persists in area mode.
  3. issue persists with opencl on.
  4. issue persists when leaving on only: blackpoint/white bal/demosaic/rgb curve/input output profile
  5. the glitch in the histogram can also be triggered in the tone curve module.

@jenshannoschwalm
Copy link
Collaborator

Leaves me clueless atm, just to make sure: it's only happening with RCD?

@jenshannoschwalm
Copy link
Collaborator

Just tried for some time to trigger the issue and couldn't reproduce it a single time. I confess i don't fully understand the way the histogram data are fed but any demosaicer might do some overshoots but like this? I have never observed something like that on PPG, RCD or LMMSE (those are the ones i know the code quite well).

I also tried with "performance mode" or downscaled previews btw.

Maybe it would help to see a video to get a clue? Could you produce one?

@TurboGit
Copy link
Member

Maybe @dtorop can shed some light on the histogram stuff?

@jenshannoschwalm
Copy link
Collaborator

@TurboGit @Nilvus could you reproduce the issue?

@c-h-r-i-s-t-i-a-n
Copy link
Author

Video:

2022-01-28.20-09-21.mp4

@dtorop
Copy link
Contributor

dtorop commented Jan 28, 2022

Hmmm. Interesting video. And seriously confusing, as the appearance is so glitchy.

Out of curiosity, what is the "display profile" and "histogram profile" settings? Right click the toggle gamut or softproofing buttons at the bottom right of the center view to see these in a pop-up menu. If the display profile is non-standard (e.g. it's a calibrated screen), it may be helpful to upload the ICC profile.

@jenshannoschwalm
Copy link
Collaborator

The "range numbers" look a bit strange

@jenshannoschwalm
Copy link
Collaborator

I still can't reproduce as reported but there is something not working correctly with the shadowed histogram data presented in the rgb-curve module.

Bildschirmfoto von 2022-01-29 08-31-38

You can see low-signal data at the right side of both histograms. If you scroll though the demosaicers you will see that changing in the main hist correctly. But in the rgb-curve histo this is not always correct like here
Bildschirmfoto von 2022-01-29 08-37-53

For me it seems to be a gtk widget update problem ???

@TurboGit
Copy link
Member

@jenshannoschwalm : I'll try to reproduce today.

@TurboGit
Copy link
Member

Tried, tried hard and cannot reproduce on my GNU/Linux box. Either a Gtk+ issue or a general issue only seen on Windows. Would be hard to say.

@wpferguson : as you build on Windows can you try to reproduce this with the RAW + xmp linked on first comment?

@Windows-Users: If you read this, can you test and report is you reproduce or not? TIA.

@gi-man
Copy link
Contributor

gi-man commented Jan 29, 2022

I'm in windows 11 with 3.9.0+116~gc543d2cb2

I cant reproduce this (with or without OpenCL). I tried to click in the same areas as the posted video.

@c-h-r-i-s-t-i-a-n
Copy link
Author

The "range numbers" look a bit strange

Which numbers do you mean?

@c-h-r-i-s-t-i-a-n
Copy link
Author

Out of curiosity, what is the "display profile" and "histogram profile" settings? Right click the toggle gamut or softproofing buttons at the bottom right of the center view to see these in a pop-up menu.

display profile = system
softproof profile = sRGB
histogram profile= sRGB

I'm not using a calibrated screen / icc stuff.

On my linux laptop (manjaro) I'm also not able to reproduce the glitch.

@wpferguson
Copy link
Member

Tried to reproduce and couldn't in either windows or linux with current master

@c-h-r-i-s-t-i-a-n
Copy link
Author

Could reproduce this also with a Canon CR2 raw file.

@dtorop
Copy link
Contributor

dtorop commented Feb 2, 2022

display profile = system softproof profile = sRGB histogram profile= sRGB

I'm not using a calibrated screen / icc stuff.

A long shot, but if you change "display profile" to something known, such as sRGB (or really anything on the list except for system), does the problem still occur?

@c-h-r-i-s-t-i-a-n
Copy link
Author

Yes, problem still occurs.

@jenshannoschwalm
Copy link
Collaborator

@dtorop i looked into the histogram code somewhat deeper yesterday, also watching the video again and tried to understand what's going on.

Form my current understanding it seems we have possibly two different issues.

  1. The helper histograms inside the modules are not updated if data are read from pipeline cache. (You can do whatever changes visible output like switching on/off the highlights reconstruction module offering a nice purple cast - the main histo is changed always, the modules histo is not) This is easy to understand, as in dt_dev_pixelpipe_process_rec we immediately go to post_process_collect_info, where we only update the main histo but do not check/update inside the modules. (This can and should probably be fixed there)
  2. This leaves out: where are the wrong data for the main histogram are coming from? I looked at the histo Worker code and there we use c/realloc for the histogram buffer. (From the docs this should be aligned for the largest native datatype, which would be 8 on windows ...) But the code seems to assume we are simd aligned - this would take/write bogus data.

What do you think?

@dtorop
Copy link
Contributor

dtorop commented Feb 2, 2022

@jenshannoschwalm These are both really good points. Regarding 2), all that code seems heavily optimized for decade-old CPUs/compilers, and could use some attention.

I spent a bit of time looking at the pixelpipe code:

  • In the normal case for a "modern" pixelpipe, the final histogram is calculated from gamma input. But as gamma nowadays is usually just after colorout, it makes sense to instead calculate this from colorout input, which saves a colorspace hop.
  • But this cause caching problems. If colorout is cached, we don't have access to its input.
  • Probably the right thing to do then is just not update the histogram. But if I turn off caching for colorout, I see some interesting noise in the upper part of the preview image.

I haven't traced this noise, but wonder if this is connected to 2), or yet another bug. Here is a diff which can trigger the bug for me:

modified   src/develop/pixelpipe_hb.c
@@ -1110,7 +1110,7 @@ static int dt_dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, dt_develop_t *
   // do not get gamma from cache on preview pipe so we can compute the final histogram
   if((pipe->type & DT_DEV_PIXELPIPE_PREVIEW) != DT_DEV_PIXELPIPE_PREVIEW
      || module == NULL
-     || strcmp(module->op, "gamma") != 0)
+     || !(strcmp(module->op, "colorout") == 0 || strcmp(module->op, "gamma") == 0))
   {
     dt_dev_pixelpipe_cache_fullhash(pipe->image.id, roi_out, pipe, pos, &basichash, &hash);
     cache_available = dt_dev_pixelpipe_cache_available(&(pipe->cache), hash);

(Also, because the gamma is never fetched from cache, some later code that handles no gamma input data for the histogram is irrelevant.)

Yet another thing is that colorout always asks for Lab input data, which doesn't seem to make sense anymore when the pipeline up until then could be in RGB -- an unnecessary hop?

Maybe the question is just who will work on fixes. I have some time, if it helps... Of course none of this may fix this unsual Windows bug.

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

This issue did not get any activity in the past 60 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.

@jenshannoschwalm
Copy link
Collaborator

@dtorop @c-h-r-i-s-t-i-a-n coming back to this - i can't reproduce any longer now!

From my current understanding the issue is likely related to the (now updated) pixelpipe cache code. As we have a module expanded we do (and did) reweigh the cacheline of the module with the inside gtk widget. NOW very likely we have correct data from the cache, with the old cache code - likely not as there were only 5 cachelines and overwritten data was often taking place.

Could you check?

@c-h-r-i-s-t-i-a-n
Copy link
Author

Which version? Bug is still there in 4.0.0.

@jenshannoschwalm
Copy link
Collaborator

In master from today ...

@c-h-r-i-s-t-i-a-n
Copy link
Author

It's still occuring in 4.1.0+338.

@jenshannoschwalm
Copy link
Collaborator

Thanks for testing...

@github-actions
Copy link

This issue did not get any activity in the past 60 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.

@jenshannoschwalm
Copy link
Collaborator

@c-h-r-i-s-t-i-a-n i checked this after some time - and after finding some very old pixelpipe-cache bug possibly resulting in wrongly read histogram data from the preview pixelpipe.

I can't reproduce any longer, could you please check & confirm the fix and possibly close the issue?

@c-h-r-i-s-t-i-a-n
Copy link
Author

Is this in master? If not how can I integrate the patch?

@jenshannoschwalm
Copy link
Collaborator

In master

@c-h-r-i-s-t-i-a-n
Copy link
Author

Glitch in histogram is still there.

Version tested: 4.1.0+997~g49764adbe

@jenshannoschwalm
Copy link
Collaborator

I can't reproduce in top histogram any more...

@Mark-64
Copy link
Contributor

Mark-64 commented Dec 29, 2022

Can't reproduce, Windows 11, DT 4.2, nVidia 1060

@dtorop
Copy link
Contributor

dtorop commented Dec 29, 2022

Just looking at this again, after long hiatus. I should have a bit more time and would finally like to modernize/rework the common/histogram.c code, for all the reasons above (it's optimized for obsolete compilers/CPUs and may have alloc bugs on Windows). This could be a follow-up to cleanup of the common colorpicker code (#13216).

Is part of the weirdness because of pixelpipe's handling of histograms and colorpickers? Unless recent work by @jenshannoschwalm changed this, the pixelpipe will update per-module histogram/pickers after processing that module, but if the module output is pulled from the cache, these aren't updated. I dabbled a while back with a fix for this via forcing re-processing of iops which needed histogram output. This added a bunch of special-case code and processing. Wouldn't it be better if the pixelpipe cache included not just the iop's output data but also the associated histogram? (And picker data as well?)

@jenshannoschwalm
Copy link
Collaborator

@dtorop there is a pending PR still but that is not related to histogram code.

I am somewhat surprised about this "but if the module output is pulled from the cache". From how i understand the pixelpipe code i would say that a module might possibly get a cached buffer (from a module earlier in the pipeline) as input. That's btw how the important hint for expanded modules works. An expanded module is very likely to change a parameter so it wants it's input to be available. And a heavy-processing module (A) passes a hint to the next module (B) in the pipeline to take it's (B) input as "important" to avoid reprocessing (A). Or do we misunderstand here?

@github-actions
Copy link

This issue did not get any activity in the past 60 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.

@dtorop
Copy link
Contributor

dtorop commented Feb 28, 2023

I'm curious if this bug is reproducible now that #13330 is merged, as that PR altered a bunch of the histogram-calculating code?

@c-h-r-i-s-t-i-a-n
Copy link
Author

Hi, I can still reproduce it (V 4.3.0+558, build on Feb-10).

grafik

@dtorop
Copy link
Contributor

dtorop commented Feb 28, 2023

Hi, I can still reproduce it (V 4.3.0+558, build on Feb-10).

Well, being able to reproduce a bug is at least a start to the process of fixing it!

I'm off the table for dt work for a bit (weeks, at least), but it is good that github auto-reminds once the report goes fallow -- that will be about the right interval for me.

jenshannoschwalm added a commit to jenshannoschwalm/darktable that referenced this issue Apr 10, 2023
For an overview about "how the pixelpipe cache works" see darktable-org#14130

There has been an issue related to some modules internally showing histograms (hist-module),
the underlying reasons are

1. those histograms are updated **while** processing the module
2. if we develop an image in darkroom we often do **not** process the whole pixelpipe but take
   data from the pixelpipe cache thus the histogram updating as in (1) is not done.

We have discussed basically two options until now
1. do some sort of pre-processing before executing the pipe
2. introduce some special widget updating code

both seem to be pretty difficult and likely over-designed.

This pr follows a different approach:

1. If we develop an image with one of the hist-modules **expanded** we accept some performance
   penalty and force the pipeline to be processed from the first expanded hist-module.
   As these modules are late in the pipe the penalty is pretty small.
2. A new mask `DT_REQUEST_EXPANDED` in `dt_dev_request_flags_t` takes care of this.
   The modules in question set this flag.
3. In `dt_dev_pixelpipe_t` we have a new gboolean `nocache`, this flag is checked in the pixelpipe
   and it's caching code.
   If **TRUE**, the requested cachelines are marked as invalid.
   (This is like we do for visualizing masks in some ways)
4. While processing each module in the **full** pixelpipe we check for
     a) the status of `DT_REQUEST_EXPANDED`
     b) the module being enabled & expanded
   and possibly unvalidate the current cacheline and set the pixelpipe `nocache` flag.

Fixes darktable-org#11069
Likely Fixes darktable-org#11003
jenshannoschwalm added a commit to jenshannoschwalm/darktable that referenced this issue Apr 10, 2023
For an overview about "how the pixelpipe cache works" see darktable-org#14130

There has been an issue related to some modules internally showing histograms (hist-module),
the underlying reasons are

1. those histograms are updated **while** processing the module
2. if we develop an image in darkroom we often do **not** process the whole pixelpipe but take
   data from the pixelpipe cache thus the histogram updating as in (1) is not done.

We have discussed basically two options until now
1. do some sort of pre-processing before executing the pipe
2. introduce some special widget updating code

both seem to be pretty difficult and likely over-designed.

This pr follows a different approach:

1. If we develop an image with one of the hist-modules **expanded** we accept some performance
   penalty and force the pipeline to be processed from the first expanded hist-module.
   As these modules are late in the pipe the penalty is pretty small.
2. A new mask `DT_REQUEST_EXPANDED` in `dt_dev_request_flags_t` takes care of this.
   The modules in question set this flag.
3. In `dt_dev_pixelpipe_t` we have a new gboolean `nocache`, this flag is checked in the pixelpipe
   and it's caching code.
   If **TRUE**, the requested cachelines are marked as invalid.
   (This is like we do for visualizing masks in some ways)
4. While processing each module in the **full** pixelpipe we check for
     a) the status of `DT_REQUEST_EXPANDED`
     b) the module being enabled & expanded
   and possibly unvalidate the current cacheline and set the pixelpipe `nocache` flag.

Fixes darktable-org#11069
Likely Fixes darktable-org#11003
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: pending someone needs to start working on that reproduce: peculiar the bug seems to affect only a specific target and cannot be reproduced elsewhere scope: image processing correcting pixels scope: UI user interface and interactions understood: incomplete devs lack some important info to start fixing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants