Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.89k stars 323 forks source link

RT anomaly with custom curve in CIECAM - brightness suddenly changes #3887

Closed RawConvert closed 6 years ago

RawConvert commented 7 years ago

This occurs in RT 5.1 Stable under Ubuntu 16.10. Scenario: You are editing a RAW file and using CIE CAM 02 to adjust exposure via a custom curve. You have not added any points yet, so you have a 45 degree line running from black to white. You now place a point exactly on the 45 degree line, or as close to this as possible. Place the point roughly central, but this does not seem to matter. As you place the point, the brightness jumps noticeably, and the main histogram ("top left" of screen) changes accordingly. This does not happen with all RAWs but does for this RAW+PP3 loaded here - https://filebin.net/vdx0rgylh2en45ev Screen grab just before placing the point - rock-before Just after placing point - rock-after Please also see related issue "RT anomaly with custom curve in CIECAM - extreme white point"

heckflosse commented 7 years ago

If I place the control point exactly on the diagonal using a single click, it works fine. Did you hold ctrl while placing the control point on the curve? That's important!

Edit: If you want to add a curve point without effect (just for future use) hold ctrl-key while adding the curve point. If you don't, you most likely add a curve point which changes the curve (even if it's a tiny change)

RawConvert commented 7 years ago

No, I didn't use CTRL. That's a good tip though, thanks - sometimes you add a second point as a "isolator" and just want it to land on the curve next to an existing point. Presumably using CTRL makes that easier.

RawConvert commented 7 years ago

The brightness is also jumping when I use CTRL, at least most times, but there were a couple of times just now when it didn't change. Placing a point uses a lot of CPU - all cores going to 50% even with CTRL when presumably no pixels to re-calculate?

heckflosse commented 7 years ago

@RawConvert I will investigate!

heckflosse commented 7 years ago

@RawConvert I tried to reproduce but I can not confirm that the brightness or the histogram changes when I add a curve point while holding CTRL

Desmis commented 7 years ago

I just have a look...to my "old code".

The problem comes from calculation of "adaptation scene luminosity"...in this case, it is probably bad. This value is calculated from : fcomp = exposure compensation of the camera fnum = F number fspeed = Speed Fiso = ISO and more

What to do 1) unchecked auto scene luminosity 2) change the value with the slider , about 200...and now look at the curve :)

RawConvert commented 7 years ago

hi @heckflosse , so there's no jump for you with CTRL, but do you get a jump without CTRL? I have no trouble getting it to jump with or without CTRL.

hi @Desmis , I'm not 100% sure what you're suggesting here, however... With my 5.1, the brightness still jumps when I uncheck Scene Luminosity. But if I enter value 200, it no longer jumps. So I try some other values - 60 (jumping), 130 (jumping but less), 165 (jumping much less). So as the value increases, the effect reduces. Where you say "now look at the curve" - I can't see any difference!

I like CIECAM because I've had some nice results in the past, but I haven't tried to understand the theory and I don't use a number of the controls such as Scene L. , Viewing Conditions, I don't know what they do or when I should use them. It seems there are definitely some gremlins in this tool, and it would be nice to fix it, but of course that's easy for me to say!

Desmis commented 7 years ago

@RawConvert What I try to say...my english is very bad...there is a link between "Scene luminosity (Cd /m2) and CIECAM. It is one ot the concepts of CIECAM as also "Viewing conditions" with Surround, Dark surround, CAT adaptation, Brightness, Colorfulness (both very different from usual Lighness or Chroma...)

In most cases calculates values of Scene luminosity is good, but in some cases not ==> auto button. Look at the curve means no jump.

You can read the doc in Rawpedia, I have write (then translate in English).

What gremlins do you speak ?

RawConvert commented 7 years ago

hi again. The “gremlins” are these -

I’ve read your Rawpedia article and I think it’s good, and not a simple subject to write about. It might benefit from having a practical section suggesting what sort of photos do and don’t work well, for example. Best regards, Andrew P.S. Your English is a whole lot better than my French!

Beep6581 commented 7 years ago

that the jump occurs just by placing a point. I think common sense says that if you just put a point on and don’t change the curve’s path, then the image shouldn’t change using CTRL tends to cause a jump, but not always, so something weird is happening

Sounds like in some cases you move the point, in others you don't. Keeping Ctrl pressed does not prevent the point from moving, it merely slows it down.

RawConvert commented 7 years ago

"Sounds like in some cases you move the point" - no, this is not the explanation, please try it. The point can be placed pretty accurately and the "jump" is out of all proportion to any slight misalignment, I think my two screen shots above show that. "Keeping Ctrl pressed does not prevent the point from moving, it merely slows it down" - this is true if you're moving a point, but not if placing one. RT very conveniently puts it on the curve at the nearest place without altering the curve, see Ingo's comment above!

gaaned92 commented 7 years ago

@RawConvert I just tried your files 1- what is strange, with your pp3, I don't get the same histogram. 2- I confirm the weird behaviour of custom curve in brightness mode. The same change of brightness occurs even if a point is added in the very shadow. Playing with this curve shows other curious or erratic behaviours.

In ligthness mode, behaviour is better but not really understandable (e.g. is the curve applied to the histogram shown with the curve?) I think the way to add a point on the curve is not relevant here.

RawConvert commented 7 years ago

Thanks @gaaned92 , I'm glad someone else is also seeing the behaviour! Regarding -1-, are you getting Scene Luminosity = 57.116 as @Desmis noted, and which I also get, please?

gaaned92 commented 7 years ago

@RawConvert Regarding 1, I was using a bad pp3. Ok after renaming rock.cr2.pp3. @Desmis Jacques, You can confirm the bad behaviour using the " edition of node in/out value" button on upper right of curve. If you input 50,50, there is a clipping. if you input 50, 50.001 no clipping! as it should be more clipped. Now I really think there is a bug. Thanks in advance to have a look and bring a correction. André

Desmis commented 7 years ago

I used the pp3 "rock.pp3", and I don' have clip...

But I repeat another time, CIECAM is complex...The first thing to have good results, is that "Scene luminosity" is good, otherwise all seems to work bad. Scene luminosity (La) is only used by CIECAM, not by RGB, or Lab, or LCH, etc. "La" correspond to the real luminosity of the scene. In this case it is low value.

Normaly "La" is calculate from EXIF + exposure compensation + White point compensation. But in some cases, due to ??, EXIF does not match reality, or "auto exposure' is wrong. It is why 5 yeras ago, I put the auto button.

But, after many many try, I don't have clip. :)

Beep6581 commented 7 years ago

There seem to be several points of confusion here so lets sort those out first:

  1. We should all test on RawTherapee 5.1.
  2. Let's make sure we're all using the same CIECAM02 settings in Preferences: imgur-2017_05_31-11 21 41
  3. Set your monitor profile to "None" under the preview. This has no influence on the histograms but it lets us make sure our previews match. @RawConvert why are you using RT_Medium_gsRGB?
  4. There is a strong change in the preview because you're jumping from completely bypassing the curve from the pipeline to using it, even though the curve's shape is linear or almost linear. @Desmis this is what puzzles @RawConvert . We have discussed this in past years and the reply was that it is not a bug, but it certainly feels strange.

Regardless of the above, there is a bug in RT's logic as to when to ignore a curve from the pipeline and when not to, at least in CIECAM02 Tone Curve 1 - issue #3902 opened.

RawConvert commented 7 years ago

Some comments -

  1. I'm on 5.1
  2. I'm using these, guess they are the defaults.
  3. I've tried "None" but the jump still occurs. I'm using"medium" because I have an adobeRGB panel and I believe "medium" is the RT equivalent of adobeRGB .
  4. Afraid I don't understand the first sentence. What puzzles me are the various things I've noted above and in the related Issue I raised. Even if something was not considered a bug in the past, it can be re-visited.

Are you sure it's not a bit premature opening #3902? Wouldn't it be better to get to the root of what's wrong before starting on fixes?

Beep6581 commented 7 years ago

Are you sure it's not a bit premature opening #3902? Wouldn't it be better to get to the root of what's wrong before starting on fixes?

I am sure. 3902 is a bug in logic/consistency, while this issue is about whether it's correct for a linear curve to lead to a difference in output (my point 4). In the past it was said that that behavior was not a bug, but as you wrote, the question could be revisited.

Desmis commented 7 years ago

@Beep6581

GitHub seems (for me) to work bad... I just push a new branch "ciecamtest"....but impossible to connect to GItHub after

More, I cannot directly see all "Issues"...is it a problem with GitHIB ?...Oh now it work :)

So I just created a branch "ciecamtest" with a small change in the calculation of "wh" ... when "La" is low and the algorithm "brigthness" or "all" is used... :)

I think with that, no jump in the curves :)

here the little change if (alg >= 2 && la < 200.f) { la = 200.f; }

gaaned92 commented 7 years ago

@Beep6581 perhaps you are right to split the problem into small chunks, but beyond #3902 bug, the fact is that the behaviour is not sane. I think there is a global problem here in brightness mode and correction of 3902 will not really improve the situation. for instance (be sure to be in brightness mode):

or -select the point (100,100) -manually give it value (40,100) photo should become much more exposed and overexposed but surprise! it is less exposed.

Lightness curve is obviously better.

I greatly respect the hard work done here by Jacques, but if the behaviour of a tool ( here brightness curve) is not understandable by the basic user (as me), It should be shown only on option to power users.

Desmis commented 7 years ago

I just try with "rock" and my simple patch. When I test the 2 conditions above - in "algo : Brightness + Colorfullness" and Tone curve 1 : Brightness, all seems to work well for me :)

I say seems, because I do not have clip :)

RawConvert commented 7 years ago

@gaaned92 , @Desmis , regarding getting / not getting clipping, my thresholds in Preferences are set at 3 and 252. These might not be the defaults.

Beep6581 commented 7 years ago

Clipping is not the problem here, it is just the way @RawConvert observed the anomaly. The anomaly is that the brightness of the image jumps (to darker) the moment the curve enters the pipeline, even if it is linear or almost linear.

@Desmis I confirm that the jump in brightness does not happen in branch ciecamtest.

gaaned92 commented 7 years ago

@Desmis No more jump in brightness in branch ciecamtest and now brightness change according to modification of curve. Now there remain to understand the other behavior indicated by @RawConvert #3886

RawConvert commented 7 years ago

I noticed today a couple of other things to do with the ciecam Colour Curve underneath tone curves 1 & 2. Firstly, just a cosmetic thing, I think the vertical axis "graphic" is upside down, since the colouration of the bar is at its max at zero. Secondly, the histogram is all in the first quarter (on the play raw isalnd pic). Perhaps that is ok with Prophoto working space, what do you think? Thirdly, the histogram is the same whether you choose chroma, saturation or colourfulness. Is that right? BUT thanks for looking at CIECAM, and it seems there is a fix for the jump issue, really good.

Desmis commented 7 years ago

In fact the problem is due to the lack of precision of calculations in "float" mode - which is the default mode. To remedy this, I made this change in the calculation of "La" which should not have consequences.

If you use the "double" mode (Preferences), I have not made any changes because everything works correctly.

If nobody sees any objections I will "merge dev" this change tomorrow.

I'll look at the others point after :)

heckflosse commented 7 years ago

@Desmis Jacques,

I touched a lot of float ciecam02 code during my optimizations the last years but I never touched the double ciecam02 code. That means the double precision code is not up to date with the float precision code and imho should not be used (except maybe for tests).

Desmis commented 7 years ago

@heckflosse

Ingo You are right, I only used "double" for testing :)

Desmis commented 7 years ago

@RawConvert Yes "color vertical axis" is inversed....it is the same in others curves in RT...It is a GUI problem, I'll look, but it is not my speciality :)

For histogram in CIECAM: 1 -As it is said in Rawpedia "The tone curve histograms in the CIE Color Appearance Model 2002 tool can show values before or after CIECAM02 is applied. To view the values after CIECAM02 adjustments, enable the "Show CIECAM02 output histograms in curves" option. If disabled, the histograms show values before CIECAM02. " (Same thing for Chroma curve) 2 - the values displays for chroma depend of chroma! The range is about 0..200 (in Lab mode), and for usual colors we saw, we are between 0..70 often less than 70.

:)

RawConvert commented 7 years ago

Re -2- , it's good that the fine control with CTRL is available. Without this, I think this function would not be useable, due to all the action happening in one small area. It might be nice if the histogram could be scaled to enlarge on the x-axis, but I appreciate this is probably a very minority "use case". (Though the hist. is a lot more spread out for Saturation with "output historgrams" ticked)

Desmis commented 7 years ago

@RawConvert I push a chnage in ciecamtest

I have increase about 25% the range of color histogram in Ciecam mode (for chroma and colorfullness). I cannot more, because if current image is saturated, all we be wrong :)

jacques

Beep6581 commented 7 years ago

The fix was confirmed, @Desmis can we close this?

Desmis commented 7 years ago

@Beep6581

I think we can close :)

RawConvert commented 7 years ago

Yes, close, and in addition, is there anything we can add to a "wish list" for future changes. For example, @Desmis has added a 25% expansion of the histogram... it would be nice to let the user pick a scaling amount in the tool or in Preferences and so get the curve easier to work on. Perhaps this could be efficiently bundled up with some future change?

Beep6581 commented 7 years ago

@RawConvert stick to one bug/enhancement request per issue.

RawConvert commented 7 years ago

@Desmis , @gaaned92 , @Beep6581 , I built RT 5.2-113-gd1fe99f8 yesterday, branch remotes/origin/dev, but the brightness jumping is still happening. Perhaps the fix has not reached Dev yet? What's the position please?

Beep6581 commented 7 years ago

@RawConvert the branch was merged into dev 3 months ago.

This does not happen with all RAWs but does for this RAW+PP3 loaded here - https://filebin.net/vdx0rgylh2en45ev

Please upload the raw + PP3 again.

RawConvert commented 7 years ago

here we are: https://filebin.net/aixbk635yem3tcbw To reproduce - select Brightness and Colorfulness, scene luminosity to about 2000 to make jump very obvious, custom tone curve for Brightness, cursor on line, CTRL+click to place point, then CTRL drag very slowly south, brightness will jump up appreciably. Probably only one unit of mouse movement necessary to do this. Remove point and repeat process dragging north, same result. So brightness increases a lot no matter which way you start to drag the first point.

Beep6581 commented 7 years ago

Confirmed. Simpler steps to reproduce:

  1. Open @RawConvert 's raw,
  2. Apply his PP3 https://i.imgur.com/vUtltPr.jpg
  3. Place a point on tone curve 1 in "Brightness" mode. If you use Ctrl to place it perfectly on the curve, the preview brightness does not jump https://i.imgur.com/jwvsVVe.jpg
  4. But move it the tiniest amount - jump. https://i.imgur.com/5mUYJlG.jpg
Desmis commented 7 years ago

I just push a new branch "ciecamtest2"

I found an old mistake in calculation of histogram needs by curve brigtness ciecam. I suppress precedent modification . if (la < 200.f) la = 200.f

Now "float version" works as "double version".

But, in all cases it cannot be perfect, it is almost impossible :)

If you don't change "La" and "Yb", in most cases all calculations needs for Q (brightness) are good.

if you change "La" and or "Yb", there will be some differences and you can see histogram change (a little), but small changes to image.

jacques

RawConvert commented 7 years ago

Thanks for fixing Jacques. (I don't understand the detail e.g. if la<200f etc. but no worries, I don't need to understand this!).

When should I look out for this coming through? That is, it's gone into ciecamtest2, does it go into Dev following some testing?

Desmis commented 7 years ago

Hello I found another bug in calculation for brightness curve in Ciecam

Now, I think the curve(s) work fine :) I just push this change 2e9c742

@RawConvert It will go in Dev when somebody (s) will test the branch ciecamtest2 :)

Jacques

heckflosse commented 7 years ago

@Desmis Jacques, I'm already compiling ciecamtest2

Ingo

heckflosse commented 7 years ago

@Desmis Jacques, fix confirmed

Desmis commented 7 years ago

@heckflosse Ingo Thank you for testing :)

I have push the same modification for ciecam "double"

If no other remark, I'll merge "ciecamtes2" in "dev" tomorrow :)

Desmis commented 7 years ago

merge in dev :)

Beep6581 commented 7 years ago

Fix confirmed, but there is one more.

  1. Open any photo and apply this PP3: https://filebin.net/x3pkm9nivny2u0rt
  2. There is a knob in the first CIECAM02 lightness curve. Move that knob the tiniest amount and the preview jumps.
Beep6581 commented 6 years ago

Link expired, closing.