Project

General

Profile

Bug #11787

profile/intent leak from print to export settings

Added by Dan Torop over 1 year ago. Updated over 1 year ago.

Status:
Fixed
Priority:
Low
Assignee:
-
Category:
Printing
Target version:
Start date:
10/25/2017
Due date:
% Done:

100%

Affected Version:
git master branch
System:
all
bitness:
64-bit
hardware architecture:
amd64/x86

Description

Altering the profile or intent in the "print settings" area in the print view changes the profile/intent used by export. The export panel provides no visual feedback that these settings have changed. Presumably settings also leak from export to print.

See discussion of this in #11783.

TEST CASE

  1. Switch to lighttable view.
  2. In export panel, set profile to sRGB.
  3. Switch to print view
  4. In print settings panel, set export profile (in the bottom-most section under "print settings") to Adobe RGB
  5. Switch to lighttable view.
  6. Verify that the export profile has remained sRGB.
  7. Export the current image.

EXPECTED BEHAVIOR

Image will be exported as sRGB.

ACTUAL BEHAVIOR

Image will be exported as Adobe RGB

Associated revisions

Revision 1425e4f3
Added by Tobias Ellinghaus over 1 year ago

Don't use export's conf to pass data to colorout

Abusing the configuration system by setting override profile/intent in
the export namespace from all over the code to control what colorout
does is bad style and prone to race conditions. It also messes with
exporting images.

Fixes #11787

Revision 8e401e7f
Added by Tobias Ellinghaus over 1 year ago

Pass overwrite profile to dt_*_get_ouput_profile

Fixes #11787. Again.

History

#1 Updated by Tobias Ellinghaus over 1 year ago

  • % Done changed from 0 to 100
  • Status changed from New to Fixed
  • Target version set to 2.4.0

I think I fixed this. Please test, I don't have a printer to try myself.

#2 Updated by Dan Torop over 1 year ago

I see one more export/print entanglement. The conf values plugins/lighttable/export/icctype and plugins/lighttable/export/iccprofile are still used in dt_colorspaces_get_output_profile() in colorspaces.c. This works when the various format/*.cc write_image() routines request the output profile during export. But it produces the wrong results when dt_apply_printer_profile() in printprof.c tries to get the intermediate profile to use as an input profile.

It's possible to test this (if one had a printer) by setting the profile in the export panel to "BRG (for testing)" and the profile at the bottom of the print settings panel to "sRGB (web-safe)". The print will come out with red/blue values flipped.

I put up a fix at https://github.com/dtorop/darktable/commit/12f1378d077aa8c45d453987ae0b426a44be802e. It takes advantage of _print_button_clicked() knowing the intermediate profile, but it duplicates some code. I'm not sure if there is a nicer way to do this, via getting the profile out of the export pipeline, or via making a smarter dt_colorspaces_get_output_profile()?

I'm also not sure if the call to dt_colorspaces_get_output_profile(() in dt_imageio_jpeg_write_with_icc_profile() causes trouble? That's deep in the cache world, and I don't understand the circumstances of its use.

Other than that, testing looks good.

One minor thing is that it looks like v_intent, v_icctype, and v_iccprofile in dt_lib_print_settings_t are set but never used (the actual information is read from conf values). That could be another patch, though, to get rid of those.

#3 Updated by Dan Torop over 1 year ago

Alas, the fix I put up breaks when the intermediate profile is a file. I added a commit to fix that, the whole patch is at https://github.com/darktable-org/darktable/compare/master...dtorop:print-export-profile-fix.

I'm still not sure if this is the best solution. It's awkward to figure out the intermediate profile both in the pixelpipe and in the print code. And now dt_colorspaces_get_output_profile() is muddled as to when it is useful.

#4 Updated by Dan Torop over 1 year ago

I'm wondering whether a good fix is to make dt_dev_pixelpipe_t simply take a cmsHPROFILE of any override profile (from export, print, or slideshow). (Or if we want a bit more information for diagnostics and a dt-specific type, a dt_colorspaces_color_profile_t.) If this is NULL, colorout takes the normal path.

Then it is the responsibility of the export, print, and slideshow code to create the proper profile (based on user choice or the display profile in the case of slideshow). Export and print can bail out if for some odd reason they can't find the profile. (As opposed to the regular pixelpipe, which does its best to produce some sort of output no matter what, hence falls back to sRGB.)

That would simplify the parameters to dt_imageio_export(), dt_imageio_export_with_flags(), dt_control_export(), etc. and add only one pointer to dt_dev_pixelpipe_t. And mean that print wouldn't have to duplicate work of colorout and figure out the profile separately before it calls dt_apply_printer_profile().

This would make a cleaner fix of buggy behavior of print in current master, which is now:

  • convert in colorout to plugins/print/print/icc{type,profile} with plugins/print/print/iccintent
  • take the exported image and assign it to profile plugins/lighttable/export/icc{type,profile}
  • convert to profile plugins/print/printer/icc{type,profile} with intent plugins/print/printer/iccintent

but should be:

  • convert in colorout to plugins/print/print/icc{type,profile} with plugins/print/print/iccintent
  • take the exported image and convert to profile plugins/print/printer/icc{type,profile} with intent plugins/print/printer/iccintent

#5 Updated by Dan Torop over 1 year ago

Uggh, tried working out using a profile in the pixelpipe. Not a good solution at all. Export and print can either override the icc profile or the intent while leaving the other as set in colorout. The only use of passing a cmsHPROFILE or dt_colorspaces_color_profile_t into the pixelpipe would be if always overriding both.

#6 Updated by Dan Torop over 1 year ago

Also I believe the problem I'm seeing only occurs when the user has set an output printer profile (via plugins/print/printer/icc{type,profile}) as otherwise there is no call to dt_apply_printer_profile() and no problem.

#7 Updated by Dan Torop over 1 year ago

What if the various implementations of dt_imageio_module_format_t took the override icc_type and icc_filename? And the various imageio/format/ writers passed that to dt_colorspaces_get_output_profile() which integrated that into its logic? I know this is getting very baroque, but essentially trying to figure out a way that the output of the pixelpipe is tagged with an ICC profile.

#8 Updated by Tobias Ellinghaus over 1 year ago

  • Status changed from Fixed to In Progress
  • % Done changed from 100 to 50

Right, I saw the code that tries to determine the current profile and totally forgot to change it afterwards.
Right now we are only trying to fix the bug with as little chance of breaking things as possible. Eventually that should be cleaned up and rewritten, but now is not the time for that.
I will have a look, but it can take a few days before I get the time.

#9 Updated by Dan Torop over 1 year ago

That sounds right about not breaking things... I made PR 1549, which fixes the print side of things, but doesn't mess with any of the internals beyond that -- and would be superseded once that happens.

#10 Updated by Tobias Ellinghaus over 1 year ago

  • Status changed from In Progress to Fixed
  • % Done changed from 50 to 100

I fixed it similar to what you did in the PR. I also changed some internal code as it was not doing the right thing in some cases. I still want to rewrite these things once we have the release out.

#11 Updated by Dan Torop over 1 year ago

One more question! DT_INTENT_LAST seems to signify not overriding the intent from colorout params. Yet _print_button_clicked() instead sets an intent of -1 if the user chooses "image settings" as the intent. In this case colorout still does the right thing (so far as I can tell) and uses the intent as set in colorout. I'm not sure if print needs to be fixed to pass in DT_INTENT_LAST, or if something else is making this work properly.

If this is a bug, a potential fix is at https://github.com/darktable-org/darktable/compare/master...dtorop:print-export-intent-fix?expand=1.

Also available in: Atom PDF