Beep6581 / RawTherapee

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

USM threshold tools needs better documentation in rawpedia #2906

Closed Beep6581 closed 8 years ago

Beep6581 commented 8 years ago

I think something might be off with the USM threshold. At the maximal setting http://i.imgur.com/W5KFlrH.png it seems to work fine because the image looks horrible. But move the top-right point one step to the left and it results in a major image change http://i.imgur.com/LqU5rgk.png Reproduced on a 900x900px TIFF image and on amsterdam.pef

Top-right 2000: http://i.imgur.com/VFfxygL.jpg Top-right 1994: http://i.imgur.com/Vy4Sw7y.jpg

sguyader commented 8 years ago

Reproduced this bug on a Fujifilm X-T1 raw file.

heckflosse commented 8 years ago

hmm, this one : https://github.com/Beep6581/RawTherapee/blob/master/rtengine/procparams.h#L111 ?

Hombre57 commented 8 years ago

This code is correct. You can even see that the line doesn't go back to the bottom when the top left slider is completely to the right.

Hombre57 commented 8 years ago

I'd rather try to find out if the upper bound of 2000.f is correct in rtengine/ipsharpen.cc, line 267, 357, 1522 and 1615. Or maybe that the processing algorithm is wrong...

heckflosse commented 8 years ago

Hombre, I didn't say that the code is not correct. I just wanted to point out that it's a special case (as you commented in code). I'm still searching for the reason of the behaviour...

Hombre57 commented 8 years ago

ok.

The upper bound should (to be confirmed) be the max-value of the image data. Maybe it has changed over the time and my algo doesn't reflect this change.

heckflosse commented 8 years ago

Hombre, don't waste your time on this. You know, I'll find the reason ;-)

Hombre57 commented 8 years ago

I just wanted to give some hints, as I did that Threshold slider. Thanks for taking this issue, I know you'll find it, as usual :). I'm off to bed now, and will continue my patches tomorrow.

heckflosse commented 8 years ago

I think there is a bug in this two lines:

https://github.com/Beep6581/RawTherapee/blob/master/rtengine/procparams.h#L144 https://github.com/Beep6581/RawTherapee/blob/master/rtengine/procparams.h#L148

If we swap 2 and 3 it should work as expected, but not as described in rawpedia. http://50.87.144.65/~rt/w/index.php?title=Sharpening#Threshold According to rawpedia the threshold tool controls sharpness in a function of Luminance but if we look at the code https://github.com/Beep6581/RawTherapee/blob/master/rtengine/ipsharpen.cc#L268 it's not a function of Luminance but a function of the difference between original and gaussian blurred Luminance. Maybe we should change the documentation in rawpedia...

Ingo

heckflosse commented 8 years ago

Here's what I meant by swapping 2 and 3

https://gist.github.com/heckflosse/44b7a7274a52d8aa75ef

heckflosse commented 8 years ago

I was wrong with my suggestion. I thought value[2] is top right and value[3] is bottom right, but it's vice versa. The point about wrong rawpedia documentation is still valid...

heckflosse commented 8 years ago

When top right == bottom right and top_right == 2000 the following happens: All pixels which meet the condition abs(L - gauss(L)) >= 2000 will get full sharpening

When top right = 1999 and bottom right = 2000 the following happens: All pixels which meet the condition 1999< abs(L - gauss(L)) < 2000 will get from full to zero sharpening. All pixels which meet the condition abs(L - gauss(L)) >= 2000 will get zero sharpening.

In my opinion all works as designed but it needs better documentation in rawpedia

Beep6581 commented 8 years ago

Fixed in 9fec32cbfab6a9fe60010dd627be17757d7c1f72 Documentation on its way.