Add monitor profile and softproofing

[Hombre57]

No description provided.

[adamreichold]

I'll try to continue where we left off:

• I still strongly believe that we should either store monitor profile/intent in the processing parameters and notify the processing pipeline using the change events, or we should store it with in the coordinator and trigger the necessary pipeline steps directly from there. Not a mixture of both which nobody will be able to follow in the future. IMHO the first alternative is preferable.
• As indicated in the gtkmm documentation, it is perfectly fine to have class scope widgets, so I still think there is no need to new and Gtk::manage the widgets of the monitor profile selector.
• Concerning a comment in the code, I don't think we should update the options when the users selects a monitor profile/intent in the editor and only use the options as a default to initialize an editor as it currently is.
• Concerning another comment in the code, I think it is necessary to reset the monitor profile selector if the profile and intent are stored in the processing parameters since they will be changed when loading the new image and the selector should reflect that.
• @Beep6581 suggested to limit the intents to relative colorimetric and perceptual. Does that apply to the output intent as well?

And some small and more or less inconsequential stuff:

• typedef enum RenderingIntent { /* ... */ } eRenderingIntent should just be enum eRenderingIntent. (If we are not prefixing or suffixing member variables, prefixing enumerations also seems unnecessary.)
• getDefaultMonitorProfileName seems to be called getDefaultMonitorProfileStr still. ;-)
• As suggested in the issue, the last two commits (and the above changes if you take them up) should probably be squashed into a single commit.

[adamreichold]

Just something you might want to use in the future: If you want to temporarily block signals without the possibility to forget unblocking them, using a RAII helper like MyMutex::Lock seems advisable, e.g.

class ConnectionBlocker
{
public:
    ConnectionBlocker(sigc::connection& connection)
        : connection(connection)
    {
        wasBlocked = connection.block(true);
    }
    ~ConnectionBlocker()
    {
        connection.block(wasBlocked);
    }
private:
    sigc::connection& connection;
    bool wasBlocked;
};

which can then be used like

{
    ConnectionBlocker blocker(someConnection);
    /* Do stuff here, may throw or return... */
}

[Hombre57]

Nice idea, I'll add it to guiutils.cc.

[adamreichold]

I'll add the Saturation entry for the monitor profile intent, but for the scroll wheel feature, I think it's gonna be complicated because the base object is a button, not a combobox and I don't want to loose too much time for this convenience.

Maybe you could use this commit instead of the wheel function, i.e. make the pop-up button switch the selected entry cyclically when clicked, for similarly simple access to this? (The commit is larger than necessary since it begins to battle long compile times by reducing unnecessary header inter-dependencies and tries to resolve some of the open points in the common pop-up base.)

[Hombre57]

The softproofing branch has been updated:

• patch from @adamreichold merged
• reworked icons
• "Absolute" rendering intent added to the monitor profile intent, as suggested by @Beep6581

I'll make a separate patch in master for the ConnectionBlocker class.

If the changes are fine for you, I'd like to commit ASAP.

[adamreichold]

@Hombre57 If you add the absolute rendering intent to the pop-up button, it should probably also be added to the preferences dialog? I also don't think any of the issues I raised in my first comment are addressed, either by implemented changes or by rejecting the suggestions?

And one question concerning the basic work flow: Why do we need the separate soft proof button? What is the difference to choosing (None) as the output profile? The flag itself does not seem to do anything internally besides deactivating the colour profile setup if disabled? Should maybe just be a check box that enables the cms_SOFTPROOF flag when creating the output transform? If so, why not just add it as an extra checkbox to the ICM panel?

So I am sorry, but I don't think this should be merged yet. Not because of technicalities that probably just bug me and nobody else, but because I am not yet convinced that the pieces fit together. Maybe I just don't understand how the soft proof button is intended, but then could you try to explain? Thanks.

[adamreichold]

@Hombre57 Also, as I understand, an output profile is very often chosen (and embedded in the output file) even if no soft proofing in the sense of previewing the effect of printer technology on a computer monitor is done. This is necessary to have predictable results after passing on developed images and will usually be one of the standard RGB profiles like sRGB, Adobe RGB or eciRGBv2.

[Hombre57]

If you add the absolute rendering intent to the pop-up button, it should probably also be added to the preferences dialog?

I'll add that.

And one question concerning the basic work flow: Why do we need the separate soft proof button? What is the difference to choosing (None) as the output profile?

The workflow is :

• only enable softproofing if your monitor is calibrated and a monitor profile is provided. Otherwise, make no sense
• when soft-proofing button is off, the image is converted to the monitor using monitorTransform
• when soft-proofing button is on, the image is converted to the output profile first using lab2outputTransform, then to the monitor using output2monitorTransform.

Before, this branch, RT did theoretically work as if the soft-proof button was always on. Other software have a similar button to activate soft-proofing on demand, so here it is, but we could revert to the previous version, where it's always on.

The flag itself does not seem to do anything internally besides deactivating the colour profile setup if disabled?

By toggling the soft-proof button, it'll use the rtengine::setSoftProofing method to set a flag (as you noticed) which will be used in ImProcFunctions::lab2monitorRgb.

Should maybe just be a check box that enables the cms_SOFTPROOF flag when creating the output transform?

I don't know how to use the cms_SOFTPROOF, I'll let you investigate. But I know that LCMS can create special profiles for soft-proofing using cmsCreateProofingTransform.

If so, why not just add it as an extra checkbox to the ICM panel?

The ICM panel, as all tool panel, should be used to set parameters that will have a consequence on the output image. The soft-proof button, monitor profile and monitor rendering intent will only affect the preview when editing the image. But I agree that we should maybe have soft-proofing enabled as before, this has to be discussed.

I still strongly believe that we should either store monitor profile/intent in the processing parameters and notify the processing pipeline using the change events, or we should store it with in the coordinator and trigger the necessary pipeline steps directly from there. Not a mixture of both which nobody will be able to follow in the future. IMHO the first alternative is preferable.

See above.
Parameters not related to the output image should not be added to the procparams, and all parameters that have an influence on the output image should be added in it. Some devs made the choice to put some of those "result changing" parameters in options.rtSettings, contrary to my advice, I can't stand responsible for that. But you get the point I think.

As indicated in the gtkmm documentation, it is perfectly fine to have class scope widgets, so I still think there is no need to new and Gtk::manage the widgets of the monitor profile selector.

Okay, will revert that.

@Beep6581 suggested to limit the intents to relative colorimetric and perceptual. Does that apply to the output intent as well?

Don't really know, I'll let @Beep6581 answer to this.

typedef enum RenderingIntent { /* ... */ } eRenderingIntent should just be enum eRenderingIntent. (If we are not prefixing or suffixing member variables, prefixing enumerations also seems unnecessary.)

Because I'm an old style programmer, and thought that making a typdef was mandatory to use the contracted syntax (i.e. "RenderingIntent" instead of "enum RenderingIntent"). But I was wrong. Was it possible before C++11 ?

getDefaultMonitorProfileName seems to be called getDefaultMonitorProfileStr still.

Ah, missed that too :). I'll fix that.

As suggested in the issue, the last two commits (and the above changes if you take them up) should probably be squashed into a single commit.

I'll follow the advice given in the link pointed out by @Beep6581 and only do that locally. I would be very grateful if you could to that as well to avoid problem on other's side. Hard reseting seem to be a last resort solution, after some tweaking because of a bad move (i.e. pulling a rebased branch). I can also delete your branches locally and recreate it instead of pulling or resetting, but others may not have the info.

Also, as I understand, an output profile is very often chosen (and embedded in the output file) even if no soft proofing in the sense of previewing the effect of printer technology on a computer monitor is done. This is necessary to have predictable results after passing on developed images and will usually be one of the standard RGB profiles like sRGB, Adobe RGB or eciRGBv2.

What do you recommend then ?

[adamreichold]

Before, this branch, RT did theoretically work as if the soft-proof button was always on. Other software have a similar button to activate soft-proofing on demand, so here it is, but we could revert to the previous version, where it's always on.

What do you recommend then ?

I think the current way of doing things w.r.t. to the output profile is fine, we just do not need the soft proof button or internal flag IMHO, at least not in its current form. Soft proofing is a strict subset of applying an output profile AFAIU, it really means to use a profile to emulate the effect of printer technology (subtractive colour mixture) using a computer monitor (additive colour mixture).

Hence, also to keep this PR smaller, I would recommend to just remove it. (Later on we can add it as an option to enable LCMS' internal soft proof flag, but then it will also influence the output AFAIU (since it is applied to the output transform) and should hence be stored persistently in the processing parameters and be handled in the ICM panel.

Because I'm an old style programmer, and thought that making a typdef was mandatory to use the contracted syntax (i.e. "RenderingIntent" instead of "enum RenderingIntent"). But I was wrong. Was it possible before C++11 ?

Yes, it is definitely the same in C++98 (C++11 only added enum classes to the mix with stricter scoping and conversion rules). Developers coming from a C background often typedef, but for C++ the idiom is enum foo { /*...*/ }; and then foo bar;.

Parameters not related to the output image should not be added to the procparams, and all parameters that have an influence on the output image should be added in it. Some devs made the choice to put some of those "result changing" parameters in options.rtSettings, contrary to my advice, I can't stand responsible for that. But you get the point I think.

The ICM panel, as all tool panel, should be used to set parameters that will have a consequence on the output image. The soft-proof button, monitor profile and monitor rendering intent will only affect the preview when editing the image. But I agree that we should maybe have soft-proofing enabled as before, this has to be discussed.

I am perfectly fine with not putting it in the processing parameters, but then we should not abuse the processing parameter change events to signal that these properties have been changed. We should just trigger the necessary computations in the setter methods of the image processing coordinators.

Also since the soft proof button in its current form, disables the output profile completely it does affect the final image output. As indicated above, I think there is more to the output profile than soft proofing, I'd even say its canonical use is not soft proofing, but rather to properly convert from a larger working profile to smaller output profile, e.g. to sRGB.

[Beep6581]

Regarding soft-proofing. We must make sure that we implement things in a way which makes sense from a user's perspective. User interface items should do what they say they do and should not magically change roles which the user can only learn about from some hidden paragraph in RawPedia.

I have mulled this over in my head for a while and it basically comes down to the fact that the output profile should not be used for the printer profile because the printer profile used for soft-proofing must not contribute to the saved image and so it is not an output profile, it is not part of the output image.

1. The image is always transformed into the gamut of the color space defined by the output profile, is it not? This should be done to both the saved image and the preview.
2. After being transformed into the output color space, the image should then optionally be transformed into the monitor color profile, to look accurate on a profiled screen. This should only affect the preview.
3. If we want to let the user soft-proof for a printer (i.e. to transform the image into the printer's color space), this step should happen after the image is transformed into the output color space (see point 4) but before being subsequently transformed into the monitor color space if a monitor profile is being used, to make sure that the soft-proofed colors we see on screen are accurate. This means that being able to choose a printer profile should be possible additionally to choosing an output and a monitor profile, i.e. we shouldn't hijack the output profile functionality for this! Soft-proofing (i.e. the printer profile) should only affect the preview, not the saved image.
4. We could treat the printer profile as being the output profile. It sounds at first like doing this is a good idea because there is one less choice on screen and one less transform in the pipeline, it is not a good idea. The point of soft-proofing is that you can see what the print result will look like on screen so that you can make the necessary adjustments to correct the image and get an instant preview of the printed result. If the output profile becomes the soft-proofing profile then we have a problem because it is not used in the saved image, so it is not an output profile, just a preview one.

If we agree on that, there remains the question: where does the user specify the printer profile? Well it should not be in the toolbox because it does not affect the saved image. That leaves us three choices:

1. We have it under the preview next to the monitor profile.
   Pro: Easily accessible.
   Con: Few people need it, and we are running out of space there.
2. We have it in Preferences.
   Pro: Out of the way.
   Con: Too much out of the way, hard to access for people who have several printer profiles and need to change them frequently.
3. We make a notebook out of the left part of the Editor and put it there, beneath the always-visible Navigator.
   Pro: We don't clutter the bottom panel which has limited room, we keep this easily accessible.
   Con: None.
   The notebook would then have one tab with the current history and snapshots, one tab for these color-management settings which do not affect the saved image and so do not belong in the toolbox on the right, and plenty of room for other tabs, e.g. see issue Processing profile selector as a tree in a vertical tab #3043
   I need the monitor profile selection to be easily accessible, not in Preferences. With such a tab on the left, we could move the monitor profile and intent selection out of the bottom panel and into this tab, if anyone wants to do that - I won't object.

[Hombre57]

I'll make something that is not elegant, but I'll let someone else finish this branch ( @adamreichold ? ). More on the reasons later on.

[adamreichold]

@Beep6581 @Hombre57 I agree with the previous comment on a separate tab for monitor and printer profile. However, since this is getting very much out of hand, I think we should try to wrap something useful up and push that out to our users before we make more plans. Hence, I would suggest we strictly limit this issue and pull request to

• Add a rendering intent settings for the output profile to the processing parameters.
• Add the on-the-fly selection of monitor profile and intent to the editor panel.

Anything else should be tackled in separate issues and pull requests IMHO. This way, users get a chance to tell us what they actually want and we get the chance to actually finish parts of this without getting bogged down by ever more complicated reviews that touch ever more issues.

[Beep6581]

I wanted easy monitor profile and rendering intent selection, and I'd be very happy if that got committed to master (along with the icons from #2744 (comment) ) and if master was merged into gtk3. I do think subsequently moving these things to a new notebook tab as I wrote above is a necessity, but it would be nice to be able to use this feature in the meanwhile.

As a side-note, once this is committed and master is merged into gtk3, I would like to upload a new build to Coverity to scan for issues in light of recent changes.

[adamreichold]

@Hombre57 Does this mean you want this adopted? If so, I would offer to try to boil it down to the reduced scope and make it ready for merging. Or do you want to finish it yourself with the reduced scope? We could then merge this and begin to experiment on a more complete solution for soft proofing using the separate ICM preview properties tab?

[Hombre57]

I mean that you can take it over and make what you want with it. I'm also "plussing" the idea of a dedicated tab for the monitor profile and printer profile.

[adamreichold]

I went through it again and removed the soft proof button and flag. Hence, this is now reduced to having the output profile intent stored persistently in the processing parameters and modifiable via the ICM panel and the monitor profile and intent being selectable in the preferences and editor panel.

I did change the various technicalities I was anxious about and tried to trigger the monitor transform changes directly instead of via processing parameter change events but failed so far to implement this cleanly. I also left the current change events as is, even though I think we do not need separate events for output profile and intent.

Personally, I think we should now merge this to begin testing and gathering user feedback. Adding the preview ICM tab seems like a logical next step that can build upon this after merging. Cleaning up the event handling is probably something best done after implementing full soft proofing when we know exactly which kinds of events with which effects on the pipeline we need.

@Beep6581 Can you give the branch another test and merge if you are satisfied?

@Hombre57 Can you explain why you wanted to give away the branch in the end? It seems like the review was quite frustrating for you? Is there something that you think that we generally or I specifically should do different in such situations?

[Beep6581]

I tested preview, saving and PP3, and it appears to be working correctly with one bug. When I open a previously edited photo (i.e. one with a PP3 assigned), save a partial PP3 of the "Color management settings", then change just the output profile's rendering intent and save a new partial PP3, the saved PP3s are identical. They lack the OutputProfileIntent key. If I clear a photo's PP3 and "Clear from cache - full", then open it and do the same steps as above, the saved PP3s do contain that key.

[adamreichold]

@Beep6581 Seems like the output intent was just missing from ParamsEdited::set. I also tried to make sure that the default output profile intent is relative colorimetric as this was always used before?

[Beep6581]

Fix confirmed, branch merged. Feel free to delete it if it's not needed.