Beep6581 / RawTherapee

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

New RT Out Of Gamut function in RT5.4 is too sensitive #4438

Closed RawConvert closed 6 years ago

RawConvert commented 6 years ago

This new functionality allows out-of-gamut areas to be highlighted in relation to the RT Output profile or the Monitor profile. You would surely not expect to see OOG areas if you opened an sRGB jpeg and set sRGB for output, however you do, as this photo shows -

indira-jpeg-shows-oog

To reproduce - . take the uploaded _6D_7984-Test-RT_sRGB.jpg.pp3 which has profile RT_sRGB . open in RT with Neutral profile and with no printer profile set in Preferences and with RT_sRGB set as Output in Colour / Output Profile . press both the soft-proofing buttons at the bottom of the preview window.

Note that the cyan areas do not disappear quickly if you reduce saturation, you can reduce Saturation to -52 and still see small OOG areas on the side of the black drum.

Another example - _6D_7667-Test-canal-RT_sRGB.jpg - this also has fairly persistent OOG areas. I have not had to search around to find these examples, I imagine most sRGB jpegs will show the behaviour. _6d_7984-test-rt_srgb _6d_7667-test-canal-rt_srgb

uglybrian89 commented 6 years ago

Delete my account. Please. Thank you

On Mar 12, 2018 5:52 AM, "Andrew" notifications@github.com wrote:

This new functionality allows out-of-gamut areas to be highlighted in relation to the RT Output profile or the Monitor profile. You would surely not expect to see OOG areas if you opened an sRGB jpeg and set sRGB for output, however you do, as this photo shows -

[image: indira-jpeg-shows-oog] https://user-images.githubusercontent.com/16868433/37284509-af4ada1e-25f3-11e8-9260-3bc664608b38.png

To reproduce - . take the uploaded _6D_7984-Test-RT_sRGB.jpg.pp3 which has profile RT_sRGB . open in RT with Neutral profile and with no printer profile set in Preferences and with RT_sRGB set as Output in Colour / Output Profile . press both the soft-proofing buttons at the bottom of the preview window.

Note that the cyan areas do not disappear quickly if you reduce saturation, you can reduce Saturation to -52 and still see small OOG areas on the side of the black drum.

Another example - _6D_7667-Test-canal-RT_sRGB.jpg - this also has fairly persistent OOG areas. I have not had to search around to find these examples, I imagine most sRGB jpegs will show the behaviour. [image: _6d_7984-test-rt_srgb] https://user-images.githubusercontent.com/16868433/37284543-cd48a9e2-25f3-11e8-9a47-3d99949f6c14.jpg [image: _6d_7667-test-canal-rt_srgb] https://user-images.githubusercontent.com/16868433/37284575-f4b44ca2-25f3-11e8-95b4-edd6e391eeab.jpg

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Beep6581/RawTherapee/issues/4438, or mute the thread https://github.com/notifications/unsubscribe-auth/Ah1ty57XT58GfMCHwgkjy8PKJAot5Ffeks5tdm-WgaJpZM4SmlXR .

agriggio commented 6 years ago

@RawConvert I'm afraid this is a LCMS issue -- darktable behaves exactly like RT:

Beep6581 commented 6 years ago

Ping @mm2

agriggio commented 6 years ago

Also @aferrero2707, IIRC PhotoFlow uses some custom code for OOG checks (at least reading from here)

aferrero2707 commented 6 years ago

I will give my soft-proofing code a try on this image, and post the result.

aferrero2707 commented 6 years ago

@RawConvert @agriggio - The gamut warning in PhotoFlow does not give me any OOG pixel in the sRGB image, when soft-proofed to sRGB (as expected):

screen shot 2018-03-12 at 21 18 06

In order to check that the gamut warning function is working properly, I checked the image against this MPL-Chromira-Matte.icc profile, which correctly gives some OOG areas:

screen shot 2018-03-12 at 21 18 22
aferrero2707 commented 6 years ago

One more check: the same image converted to linear Rec.2020, boosted in saturation and then soft-proofed to sRGB:

screen shot 2018-03-12 at 21 36 35
RawConvert commented 6 years ago

Thanks @aferrero2707 , well done PhotoFlow! @agriggio , is it possible to port the OOG warning from PhotoFlow to RT?!

agriggio commented 6 years ago

@RawConvert well I'll have to understand the code first :-) @aferrero2707 where can I find it? Anyway, definitely not for 5.4 (too late)

aferrero2707 commented 6 years ago

@agriggio - sure, here is where to start:

https://github.com/aferrero2707/PhotoFlow/blob/stable/src/operations/convert_colorspace.hh#L103

https://github.com/aferrero2707/PhotoFlow/blob/stable/src/operations/convert_colorspace.cc#L322

We should probably discuss this in a new issue.

agriggio commented 6 years ago

I just pushed a new branch improved-gamut-warning, in which I tried to use the logic of PhotoFlow. It seems to work from my limited tests, but the possibility that I did not understand it correctly are non-negligible :-) If someone can take a look, the code is here and here (and yes, it needs a cleanup :-)

RawConvert commented 6 years ago

Many thanks @agriggio for working on this. I just tried your branch and it is much better. I'm tempted to say it's working perfectly, but I'm not sufficiently clued up to be so definitive! I loaded the same jpeg and no cyan. Then increased saturation from 0 to just 1 and got this - new-oog-logic

aferrero2707 commented 6 years ago

@agriggio - I will have a look at the RT implementation, but not before a few days...

agriggio commented 6 years ago

Thank you @aferrero2707. I have just cleaned up the code a little bit. You can find it here.

Beep6581 commented 6 years ago

@agriggio what are your thoughts about having this in 5.4? It's not a new feature but a bug fix since the currently implementation shows wrong results.

agriggio commented 6 years ago

@Beep6581 I have no technical objection, but I am still not 100% sure that the new code is correct. In particular, I haven't been able to test with pure LUT-based profiles, so I don't know if that code path works as intended...

heckflosse commented 6 years ago

@Beep6581 @agriggio In this case I see two solutions: 1) release 5.4 as is now 2) make a new 5.4 rc which includes this patch and realease 5.4 later. It's up to you

agriggio commented 6 years ago

I think we have enough features pending for a quick 5.5 release not very far from now, tbh

gaaned92 commented 6 years ago

@agriggio Crash if "no ICM: sRGB output" is selected

I don(t know why displaying RT window under gdb is now so slow

[Thread 168 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 12116.0x3c20]
0x000000006b24b080 in ?? ()
   from D:\rawtherapee\rtinstall\improved-gamut-warningrelease64\liblcms2-2.dll
(gdb) bt full
#0  0x000000006b24b080 in ?? ()
   from D:\rawtherapee\rtinstall\improved-gamut-warningrelease64\liblcms2-2.dll
No symbol table info available.
#1  0x000000006b24d8ad in ?? ()
   from D:\rawtherapee\rtinstall\improved-gamut-warningrelease64\liblcms2-2.dll
No symbol table info available.
#2  0x0000000000867754 in rtengine::GamutWarning::GamutWarning (
    this=0x1eebe4d0, iprof=0x2fbe7480, gamutprof=0x0,
    intent=rtengine::RI_RELATIVE, gamutbpc=false)
    at D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtengine/gamutwarning.cc:36
No locals.
#3  0x0000000000785c95 in rtengine::ImProcFunctions::updateColorProfiles (
    this=this@entry=0x1f79a0a8, monitorProfile=...,
    monitorIntent=<optimized out>, softProof=<optimized out>, gamutCheck=true)
    at D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtengine/improcfun.cc:381
        lcmsLock = {<rtengine::NonCopyable> = {<No data fields>},
          mutex = @0xd86bf50, locked = true}
        flags = <optimized out>
        gamutprof = 0x0
        softProofCreated = <optimized out>
        iprof = 0x2fbe7480
        gamutbpc = <optimized out>
        gamutintent = <optimized out>
        monitor = 0x1f5558d0
#4  0x0000000000932199 in rtengine::ImProcCoordinator::updatePreviewImage (
    this=this@entry=0x1f799fd0, todo=todo@entry=16384,
    cropCall=cropCall@entry=0x0)
    at D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtengine/improccoordinator.cc:828
        processingLock = {<rtengine::NonCopyable> = {<No data fields>},
          mutex = @0x1f79c0d8, locked = true}
        readyphase = <optimized out>
        highDetailNeeded = <optimized out>
        rp = {bayersensor = {method = {static npos = 18446744073709551615,
              string_ = {static npos = 18446744073709551615,
                _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                  _M_p = 0x3831fb60 "amaze"}, _M_string_length = 5, {
                  _M_local_buf = "amaze\000\000\000\a\000\000\000\000\000\000", _M_allocated_capacity = 435844902241}}}, imageNum = 0, ccSteps = 0,
            black0 = 0, black1 = 0, black2 = 0, black3 = 0, twogreen = true,
            linenoise = 0, greenthresh = 0, dcb_iterations = 2,
            lmmse_iterations = 2, pixelShiftMotion = 0,
            pixelShiftMotionCorrection = rtengine::procparams::RAWParams::BayerSensor::PSMotionCorrection::GRID_3X3_NEW,
            pixelShiftMotionCorrectionMethod = rtengine::procparams::RAWParams::BayerSensor::PSMotionCorrectionMethod::AUTO,
            pixelShiftStddevFactorGreen = 5, pixelShiftStddevFactorRed = 5,
            pixelShiftStddevFactorBlue = 5, pixelShiftEperIso = 0,
            pixelShiftNreadIso = 0, pixelShiftPrnu = 1, pixelShiftSigma = 1,
            pixelShiftSum = 3, pixelShiftRedBlueWeight = 0.69999999999999996,
            pixelShiftShowMotion = false,
            pixelShiftShowMotionMaskOnly = false, pixelShiftAutomatic = true,
            pixelShiftNonGreenHorizontal = false,
            pixelShiftNonGreenVertical = false, pixelShiftHoleFill = true,
            pixelShiftMedian = false, pixelShiftMedian3 = false,
            pixelShiftGreen = true, pixelShiftBlur = true,
            pixelShiftSmoothFactor = 0.69999999999999996,
            pixelShiftExp0 = false, pixelShiftLmmse = false,
            pixelShiftOneGreen = false, pixelShiftEqualBright = false,
            pixelShiftEqualBrightChannel = false,
            pixelShiftNonGreenCross = true, pixelShiftNonGreenCross2 = false,
            pixelShiftNonGreenAmaze = false, dcb_enhance = true},
          xtranssensor = {method = {static npos = 18446744073709551615,
              string_ = {static npos = 18446744073709551615,
                _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                  _M_p = 0x3831fc38 "3-pass (best)"}, _M_string_length = 13, {
                  _M_local_buf = "3-pass (best)\000\000",
                  _M_allocated_capacity = 2891437900165033267}}},
            ccSteps = 0, blackred = 0, blackgreen = 0, blackblue = 0},
          dark_frame = {static npos = 18446744073709551615, string_ = {
              static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x3831fc78 "/szeva"}, _M_string_length = 6, {
                _M_local_buf = "/szeva\000\000`▒▒/\000\000\000",
                _M_allocated_capacity = 107161136558895}}},
          df_autoselect = false, ff_file = {
            static npos = 18446744073709551615, string_ = {
              static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x3831fca0 "/szeva"}, _M_string_length = 6, {
                _M_local_buf = "/szeva\000\000▒2_h\000\000\000",
                _M_allocated_capacity = 107161136558895}}},
          ff_AutoSelect = false, ff_BlurRadius = 32, ff_BlurType = {
            static npos = 18446744073709551615, string_ = {
              static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x3831fcc8 "Area Flatfield"}, _M_string_length = 14, {
                _M_local_buf = "Area Flatfield\000",
                _M_allocated_capacity = 7020063024050696769}}},
          ff_AutoClipControl = false, ff_clipControl = 0,
          ca_autocorrect = true, cared = 0, cablue = 0, expos = 1,
          preser = 0, hotPixelFilter = false, deadPixelFilter = false,
          hotdeadpix_thresh = 100}
        cmp = {input = {static npos = 18446744073709551615, string_ = {
              static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x3831fab0 "(cameraICC)"}, _M_string_length = 11, {
                _M_local_buf = "(cameraICC)\000\000\000\000",
                _M_allocated_capacity = 5287633217462035240}}},
          toneCurve = false, applyLookTable = true,
          applyBaselineExposureOffset = true, applyHueSatMap = true,
          dcpIlluminant = 0, working = {static npos = 18446744073709551615,
            string_ = {static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x3831fad8 "Rec2020"}, _M_string_length = 7, {
                _M_local_buf = "Rec2020\000▒\000\000\000\000\000\000",
                _M_allocated_capacity = 13565981467305298}}}, output = {
            static npos = 18446744073709551615, string_ = {
              static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x150d8b40 "No ICM: sRGB output"},
              _M_string_length = 19, {
                _M_local_buf = "\023", '\000' <repeats 14 times>,
                _M_allocated_capacity = 19}}},
          outputIntent = rtengine::RI_RELATIVE, outputBPC = true, gamma = {
            static npos = 18446744073709551615, string_ = {
              static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0x3831fb20 "default"}, _M_string_length = 7, {
                _M_local_buf = "default\000@\004p\037\000\000\000",
                _M_allocated_capacity = 32770348699510116}}},
          gampos = 2.2200000000000002, slpos = 4.5, freegamma = false,
          static NoICMString = {static npos = 18446744073709551615,
            string_ = {static npos = 18446744073709551615,
              _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
                _M_p = 0xd05ff60 "No ICM: sRGB output"},
              _M_string_length = 19, {
                _M_local_buf = "\023", '\000' <repeats 14 times>,
                _M_allocated_capacity = 19}}}}
        lcur = {enabled = false,
          lcurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x2bb12b90,
                _M_finish = 0x2bb12b98,
                _M_end_of_storage = 0x2bb12b98}}, <No data fields>},
          acurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x2ff7af10,
                _M_finish = 0x2ff7af18,
                _M_end_of_storage = 0x2ff7af18}}, <No data fields>},
          bcurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x2ff7e500,
                _M_finish = 0x2ff7e508,
                _M_end_of_storage = 0x2ff7e508}}, <No data fields>},
          cccurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x1e4cb4c0,
                _M_finish = 0x1e4cb4c8,
                _M_end_of_storage = 0x1e4cb4c8}}, <No data fields>},
          chcurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x2fc74e20,
                _M_finish = 0x2fc74e28,
                _M_end_of_storage = 0x2fc74e28}}, <No data fields>},
          lhcurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x2fd413c0,
                _M_finish = 0x2fd413c8,
                _M_end_of_storage = 0x2fd413c8}}, <No data fields>},
          hhcurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x1557acf0,
                _M_finish = 0x1557acf8,
                _M_end_of_storage = 0x1557acf8}}, <No data fields>},
          lccurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x12fa9130,
                _M_finish = 0x12fa9138,
                _M_end_of_storage = 0x12fa9138}}, <No data fields>},
          clcurve = {<std::_Vector_base<double, std::allocator<double> >> = {
              _M_impl = {<std::allocator<double>> = {<__gnu_cxx::new_allocator<double>> = {<No data fields>}, <No data fields>}, _M_start = 0x10787a30,
                _M_finish = 0x10787a38,
                _M_end_of_storage = 0x10787a38}}, <No data fields>},
          brightness = 0, contrast = 0, chromaticity = 0,
          avoidcolorshift = false, rstprotection = 0, lcredsk = true}
        needstransform = <optimized out>
#5  0x0000000000932b14 in rtengine::ImProcCoordinator::process (
    this=0x1f799fd0)
    at D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtengine/improccoordinator.cc:1356
        change = 16384
#6  0x00000000009c1372 in sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>::operator() (this=<optimized out>)
    at C:/msys64/mingw64/include/sigc++-2.0/sigc++/functors/mem_fun.h:1991
No locals.
#7  sigc::adaptor_functor<sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator> >::operator() (this=<optimized out>)
    at C:/msys64/mingw64/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
No locals.
#8  sigc::internal::slot_call0<sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>, void>::call_it (rep=<optimized out>)
    at C:/msys64/mingw64/include/sigc++-2.0/sigc++/functors/slot.h:114
        typed_rep = <optimized out>
#9  0x00000000664ce691 in ?? ()
   from D:\rawtherapee\rtinstall\improved-gamut-warningrelease64\libglibmm-2.4-1.dll
No symbol table info available.
#10 0x0000000068619449 in ?? ()
   from D:\rawtherapee\rtinstall\improved-gamut-warningrelease64\libglib-2.0-0.dll
No symbol table info available.
#11 0x0000000064944ab4 in ?? ()
   from D:\rawtherapee\rtinstall\improved-gamut-warningrelease64\libwinpthread-1.dll
No symbol table info available.
#12 0x00007ffd789da8e6 in msvcrt!_beginthreadex ()
   from C:\WINDOWS\System32\msvcrt.dll
No symbol table info available.
#13 0x00007ffd789da9bc in msvcrt!_endthreadex ()
   from C:\WINDOWS\System32\msvcrt.dll
No symbol table info available.
#14 0x00007ffd79341fe4 in KERNEL32!BaseThreadInitThunk ()
   from C:\WINDOWS\System32\kernel32.dll
No symbol table info available.
#15 0x00007ffd7947efc1 in ntdll!RtlUserThreadStart ()
   from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#16 0x0000000000000000 in ?? ()
No symbol table info available.
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)
Floessie commented 6 years ago

@gaaned92 Does this help?

diff --git a/rtengine/improcfun.cc b/rtengine/improcfun.cc
index 44edd8b17..19d6cb6c1 100644
--- a/rtengine/improcfun.cc
+++ b/rtengine/improcfun.cc
@@ -377,7 +377,7 @@ void ImProcFunctions::updateColorProfiles (const Glib::ustring& monitorProfile,
             monitorTransform = cmsCreateTransform (iprof, TYPE_Lab_FLT, monitor, TYPE_RGB_8, monitorIntent, flags);
         }

-        if (gamutCheck) {
+        if (gamutCheck && gamutprof) {
             gamutWarning.reset(new GamutWarning(iprof, gamutprof, gamutintent, gamutbpc));
         }
gaaned92 commented 6 years ago

@Floessie It no longer crashes, but when there is no ICM, sRGB is assumed. So the OOG indication should be the same as for RTsRGB, but there is no OOG indicated.

agriggio commented 6 years ago

@gaaned92 I think this is the right thing to do

gaaned92 commented 6 years ago

@agriggio perhaps but not intuitive. In this case OOG should be deactivated

agriggio commented 6 years ago

In this case OOG should be deactivated

That's a good point, thanks!

agriggio commented 6 years ago

@aferrero2707 I think there's a bug in the logic that selects which path to take for the gamut warning (i.e. either L*a*b* and deltaE comparison or ACES and absolute comparison). In particular, using cmsIsMatrixShaper alone seems to be not sufficient to distinguish between a matrix and LUT profile. In RT, I'm now using this:

if (cmsIsMatrixShaper(gamutprof) && !cmsIsCLUT(gamutprof, intent, LCMS_USED_AS_OUTPUT))

Example of the problem. Current PhotoFlow, using my monitor profile (which is pretty close to sRGB) for proofing:

pf-bug

RawTherapee:

rt