Bug 146464 - lighttable does not deal with colour management
Summary: lighttable does not deal with colour management
Status: RESOLVED FIXED
Alias: None
Product: digikam
Classification: Unclassified
Component: LightTable-Canvas (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Digikam Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-06-06 23:38 UTC by Jakob Østergaard
Modified: 2022-02-01 09:24 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 0.9.2


Attachments
Screenshot showing the problem (933.49 KB, image/png)
2007-06-06 23:43 UTC, Jakob Østergaard
Details
Patch 1/2 (1.59 KB, patch)
2007-06-07 01:43 UTC, Jakob Østergaard
Details
Patch 2/2 (3.90 KB, patch)
2007-06-07 01:47 UTC, Jakob Østergaard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jakob Østergaard 2007-06-06 23:38:03 UTC
Version:           Current SVN  (using KDE KDE 3.5.6)
Compiler:          gcc 4.1.2 
OS:                Linux

I have configured DigiKam to work with 16-bit editing and configured Colour management (everything works very well in this regard).

Using the lighttable, it seems that the left- and right-side images are affected by the 16-bit setting too. If I add a number of raw files to the lighttable, they show up much darker than they should.

I guess it would be ok for the lighttable to just use 8-bit conversion.
Comment 1 Jakob Østergaard 2007-06-06 23:43:37 UTC
Created attachment 20791 [details]
Screenshot showing the problem

Notice how the left-side display shows the typical non-colour-managed 16-bit
raw colours while the right-side display (and the small previews) are much
brighter (closer to proper brightness).
Comment 2 Jakob Østergaard 2007-06-07 00:07:31 UTC
I can confirm that if I disable 16-bit RAW in DigiKam (Settings, RAW decoding) then the problem in the lighttable disappears; all images have proper brightness.

Personally, I think it would be very reasonable to simply use 8-bit decoding in the lighttable - after all, one is not adjusting curves there... 16-bit makes sense in the editor, maybe not so much in the lighttable...

Well, that's my 0.02 Euro at least :)
Comment 3 caulier.gilles 2007-06-07 00:10:57 UTC
Well, this is really a bug because Light Table must normally always handle RAW image in 8 bits, not in 16 bits.

Marcel, any ideas ?

Gilles
Comment 4 Jakob Østergaard 2007-06-07 01:43:49 UTC
Created attachment 20794 [details]
Patch 1/2

Implements test for equality between raw decoding parameters
Comment 5 Jakob Østergaard 2007-06-07 01:47:37 UTC
Created attachment 20795 [details]
Patch 2/2

Implements stricter check for image loader jobs (don't ignore 8/16 bit setting
or raw settings in general).
Added 8-bit setting for all previews.
Added various comments which should definitely be reviewed by someone who
understands the code a lot better than me.
Added the 8/16 bit parameter as part of the cache file name; note that this is
not a complete solution to the problem: the entire set of parameters must be a
part of the filename - but this fix solves the current problem.
Comment 6 Jakob Østergaard 2007-06-07 01:49:15 UTC
Applying the two posted patches seems to solve the problem for me right now.
I have only tested this lightly, and from what I have seen, the patches are not a complete fix at all (but I made them and they work for me right now so I didn't want to just throw them away).

Hope it helps.
Comment 7 caulier.gilles 2007-06-07 09:00:50 UTC
SVN commit 672447 by cgilles:

libkdraw from KDE3 branches : patch from Jakob Stergaard to perform equality test between 2 rawdecoding settings.
CCBUGS: 146464


 M  +24 -0     rawdecodingsettings.h  


--- branches/extragear/kde3/libs/libkdcraw/rawdecodingsettings.h #672446:672447
@@ -80,6 +80,30 @@
         colorBalanceMultipliers[3] = 0.0;
     };
     
+    /** Compare for equality */
+    bool operator==(const RawDecodingSettings &o) const
+    {
+        return sixteenBitsImage == o.sixteenBitsImage
+            && brightness == o.brightness
+            && RAWQuality == o.RAWQuality
+            && outputColorSpace == o.outputColorSpace
+            && RGBInterpolate4Colors == o.RGBInterpolate4Colors  
+            && DontStretchPixels == o.DontStretchPixels  
+            && unclipColors == o.unclipColors  
+            && cameraColorBalance == o.cameraColorBalance  
+            && automaticColorBalance == o.automaticColorBalance  
+            && halfSizeColorImage == o.halfSizeColorImage  
+            && enableBlackPoint == o.enableBlackPoint  
+            && blackPoint == o.blackPoint  
+            && enableNoiseReduction == o.enableNoiseReduction  
+            && NRThreshold == o.NRThreshold  
+            && enableColorMultipliers == o.enableColorMultipliers  
+            && colorBalanceMultipliers[0] == o.colorBalanceMultipliers[0]  
+            && colorBalanceMultipliers[1] == o.colorBalanceMultipliers[1]  
+            && colorBalanceMultipliers[2] == o.colorBalanceMultipliers[2]  
+            && colorBalanceMultipliers[3] == o.colorBalanceMultipliers[3]  
+          ;
+    };
 
     /** Standard destructor */
     virtual ~RawDecodingSettings(){};
Comment 8 caulier.gilles 2007-06-07 09:03:17 UTC
SVN commit 672448 by cgilles:

libkdcraw from trunk (KDE) : backport KDE3 patch from Jakob 
CCBUGS: 146464


 M  +24 -0     rawdecodingsettings.h  


--- trunk/extragear/libs/libkdcraw/src/rawdecodingsettings.h #672447:672448
@@ -80,6 +80,30 @@
         colorBalanceMultipliers[3] = 0.0;
     };
     
+    /** Compare for equality */
+    bool operator==(const RawDecodingSettings &o) const
+    {
+        return sixteenBitsImage == o.sixteenBitsImage
+            && brightness == o.brightness
+            && RAWQuality == o.RAWQuality
+            && outputColorSpace == o.outputColorSpace
+            && RGBInterpolate4Colors == o.RGBInterpolate4Colors  
+            && DontStretchPixels == o.DontStretchPixels  
+            && unclipColors == o.unclipColors  
+            && cameraColorBalance == o.cameraColorBalance  
+            && automaticColorBalance == o.automaticColorBalance  
+            && halfSizeColorImage == o.halfSizeColorImage  
+            && enableBlackPoint == o.enableBlackPoint  
+            && blackPoint == o.blackPoint  
+            && enableNoiseReduction == o.enableNoiseReduction  
+            && NRThreshold == o.NRThreshold  
+            && enableColorMultipliers == o.enableColorMultipliers  
+            && colorBalanceMultipliers[0] == o.colorBalanceMultipliers[0]  
+            && colorBalanceMultipliers[1] == o.colorBalanceMultipliers[1]  
+            && colorBalanceMultipliers[2] == o.colorBalanceMultipliers[2]  
+            && colorBalanceMultipliers[3] == o.colorBalanceMultipliers[3]  
+          ;
+    };
 
     /** Standard destructor */
     virtual ~RawDecodingSettings(){};
Comment 9 caulier.gilles 2007-06-07 09:07:02 UTC
Jakob,

Patch 1 is applied on KDE3 and KDE4 libkdcraw implementation. In all cases this patch can be added to svn.

For patch2, the code fix come from Marcel. Patch sound fine for me, but i lets Marcel decide if all is fine.

Thanks for you help Jakob

Gilles Caulier
Comment 10 Jakob Østergaard 2007-06-07 09:28:15 UTC
>
> Patch 1 is applied on KDE3 and KDE4 libkdcraw implementation. In all
> cases this patch can be added to svn.


Great

> For patch2, the code fix come from Marcel. Patch sound fine for me,
> but i lets Marcel decide if all is fine.


There are some comments in the patch which probably shouldn't be added 
to SVN. They were "braindumps" from me when I was reading through the 
code trying to find out what was going on.

Thanks Gilles
Comment 11 Marcel Wiesweg 2007-06-07 16:29:44 UTC
SVN commit 672556 by mwiesweg:

Apply patch from Jakob Oestergaard to reflect 8/16 bit loading
in the cache key, needed to distinguish between lighttable, histogram
and image editor loading.

Adapt equalsOrBetterThan to only accept changes in the halfSizeColorImage field.
Adapt possibleCacheKeys.
Move constructors from .h to .cpp
Change some comments.

CCBUG: 146464


 M  +67 -19    loadingdescription.cpp  
 M  +14 -18    loadingdescription.h  
 M  +1 -0      previewloadthread.cpp  


--- branches/extragear/kde3/graphics/digikam/libs/threadimageio/loadingdescription.cpp #672555:672556
@@ -28,31 +28,70 @@
 namespace Digikam
 {
 
+bool LoadingDescription::PreviewParameters::operator==(const PreviewParameters &other) const
+{
+    return isPreview  == other.isPreview
+            && size       == other.size
+            && exifRotate == other.exifRotate;
+}
+
+LoadingDescription::LoadingDescription(const QString &filePath)
+    : filePath(filePath)
+{
+    rawDecodingSettings = KDcrawIface::RawDecodingSettings();
+}
+
+LoadingDescription::LoadingDescription(const QString &filePath, KDcrawIface::RawDecodingSettings settings)
+    : filePath(filePath), rawDecodingSettings(settings)
+{
+}
+
+LoadingDescription::LoadingDescription(const QString &filePath, int size, bool exifRotate)
+    : filePath(filePath)
+{
+    rawDecodingSettings = KDcrawIface::RawDecodingSettings();
+    previewParameters.isPreview  = false;
+    previewParameters.size       = size;
+    previewParameters.exifRotate = exifRotate;
+}
+
 QString LoadingDescription::cacheKey() const
 {
     // Here we have the knowledge which LoadingDescriptions / RawFileDecodingSettings
     // must be cached separately.
-    // When the Raw loading settings are changed in setup, the cache is cleaned,
-    // so we do not need to store check for every option here.
+    // Current assumption:
+    // Eight-bit images are needed for LightTable, and if 16-bit is enabled,
+    // 16-bit half size images for the histogram sidebar,
+    // and 16-bit full size images for the image editor.
+    // Use previewParameters.size, not isPreview - if it is 0, full loading is used.
+
+    QString suffix = rawDecodingSettings.sixteenBitsImage ? "-16" : "-8";
+
     if (rawDecodingSettings.halfSizeColorImage)
-        return filePath + "-halfSizeColorImage";
+        return filePath + suffix + "-halfSizeColorImage";
     else if (previewParameters.size)
-        return filePath + "-previewImage";
+        return filePath + suffix + "-previewImage";
     else
-        return filePath;
+        return filePath + suffix;
 }
 
 QStringList LoadingDescription::lookupCacheKeys() const
 {
     // Build a hierarchy which cache entries may be used for this LoadingDescription.
-    // Typically, the first is the best, but an actual loading operation will use a faster
-    // way and will effectively add the last entry of the list to the cache
+    // Typically, the first is the best, but an actual loading operation may use a
+    // lower-quality loading and will effectively only add the last entry of the
+    // list to the cache, although it can accept the first if already available.
+    // Sixteen-bit images cannot be used used instead of eight-bit ones because
+    // color management is needed to display them.
+
+    QString suffix = rawDecodingSettings.sixteenBitsImage ? "-16" : "-8";
+
     QStringList keys;
-    keys.append(filePath);
+    keys.append(filePath + suffix);
     if (rawDecodingSettings.halfSizeColorImage)
-        keys.append(filePath + "-halfSizeColorImage");
+        keys.append(filePath + suffix + "-halfSizeColorImage");
     if (previewParameters.size)
-        keys.append(filePath + "-previewImage");
+        keys.append(filePath + suffix + "-previewImage");
     return keys;
 }
 
@@ -65,11 +104,9 @@
 
 bool LoadingDescription::operator==(const LoadingDescription &other) const
 {
-    // NOTE: If we start loading RAW files with different loading settings in parallel,
-    //       this and the next methods must be better implemented!
     return filePath == other.filePath &&
-            rawDecodingSettings.halfSizeColorImage == other.rawDecodingSettings.halfSizeColorImage &&
-            previewParameters.size == other.previewParameters.size;
+            rawDecodingSettings == other.rawDecodingSettings &&
+            previewParameters == other.previewParameters;
 }
 
 bool LoadingDescription::equalsIgnoreReducedVersion(const LoadingDescription &other) const
@@ -79,10 +116,18 @@
 
 bool LoadingDescription::equalsOrBetterThan(const LoadingDescription &other) const
 {
+    // This method is similar to operator==. But it returns true as well if other
+    // Loads a "better" version than this.
+    // Preview parameters must have the same size, or other has no size restriction.
+    // All raw decoding settings must be equal, only the half size parameter is allowed to vary.
+
+    KDcrawIface::RawDecodingSettings fullSize = other.rawDecodingSettings;
+    fullSize.halfSizeColorImage = false;
+
     return filePath == other.filePath &&
             (
-             (rawDecodingSettings.halfSizeColorImage == other.rawDecodingSettings.halfSizeColorImage) ||
-             other.rawDecodingSettings.halfSizeColorImage
+             rawDecodingSettings == other.rawDecodingSettings ||
+             rawDecodingSettings == fullSize
             ) &&
             (
              (previewParameters.size == other.previewParameters.size) ||
@@ -93,9 +138,12 @@
 QStringList LoadingDescription::possibleCacheKeys(const QString &filePath)
 {
     QStringList keys;
-    keys.append(filePath);
-    keys.append(filePath + "-halfSizeColorImage");
-    keys.append(filePath + "-previewImage");
+    keys.append(filePath + "-16");
+    keys.append(filePath + "-16-halfSizeColorImage");
+    keys.append(filePath + "-16-previewImage");
+    keys.append(filePath + "-8");
+    keys.append(filePath + "-8-halfSizeColorImage");
+    keys.append(filePath + "-8-previewImage");
     return keys;
 }
 
--- branches/extragear/kde3/graphics/digikam/libs/threadimageio/loadingdescription.h #672555:672556
@@ -45,9 +45,12 @@
             size       = 0;
             exifRotate = false;
         }
+
         bool isPreview;
         int  size;
         bool exifRotate;
+
+        bool operator==(const PreviewParameters &other) const;
     };
 
     /**
@@ -61,34 +64,26 @@
      * Use this for files that are not raw files.
      * Stores only the filePath.
      */
-    LoadingDescription(const QString &filePath)
-        : filePath(filePath)
-        {
-            rawDecodingSettings = KDcrawIface::RawDecodingSettings();
-        };
+    LoadingDescription(const QString &filePath);
 
     /**
      * For raw files:
      * Stores filePath and RawDecodingSettings
      */
-    LoadingDescription(const QString &filePath, KDcrawIface::RawDecodingSettings settings)
-        : filePath(filePath), rawDecodingSettings(settings)
-        {};
+    LoadingDescription(const QString &filePath, KDcrawIface::RawDecodingSettings settings);
 
     /**
      * For preview jobs:
      * Stores preview max size and exif rotation.
-     * The exif rotation is only a hint.
-     * Call LoadSaveThread::exifRotate to make sure that the image is really
-     * rotated. It is safe to call this method even if the image is rotated.
+     * Exif Rotation:
+     *    The exif rotation is only a hint.
+     *    Call LoadSaveThread::exifRotate to make sure that the image is really
+     *    rotated. It is safe to call this method even if the image is rotated.
+     * Raw files:
+     *    If size is not 0, the embedded preview will be loaded if available.
+     *    If size is 0, DImg based loading will be used with default raw decoding settings.
      */
-    LoadingDescription(const QString &filePath, int size, bool exifRotate)
-        : filePath(filePath)
-        {
-            previewParameters.isPreview  = false;
-            previewParameters.size       = size;
-            previewParameters.exifRotate = exifRotate;
-        };
+    LoadingDescription(const QString &filePath, int size, bool exifRotate);
 
     QString                          filePath;
     KDcrawIface::RawDecodingSettings rawDecodingSettings;
@@ -121,6 +116,7 @@
      * ignoring parameters used to specify a reduced version.
      */
     bool equalsIgnoreReducedVersion(const LoadingDescription &other) const;
+
     /**
      * Returns whether this loading task equals the other one
      * or is superior to it, if the other one is a reduced version
--- branches/extragear/kde3/graphics/digikam/libs/threadimageio/previewloadthread.cpp #672555:672556
@@ -37,6 +37,7 @@
 
 void PreviewLoadThread::load(LoadingDescription description)
 {
+    description.rawDecodingSettings.sixteenBitsImage = false;
     ManagedLoadSaveThread::loadPreview(description);
 }
 
Comment 12 caulier.gilles 2007-06-07 16:33:00 UTC
Ok, this is want mean than a new release of libkdcraw need to be done accordinly with 0.9.2 digiKam release

Gilles
Comment 13 Jakob Østergaard 2007-06-07 16:36:42 UTC
Thanks Marcel

I see that I completely misunderstood the possibleCacheKeys function :)

I'll try and test the changes tonight.
Comment 14 caulier.gilles 2007-06-07 16:42:10 UTC
Thanks Jakob.

We will wait your feedback before to release digiKam 0.9.2-final and close this B.K.O file

Gilles

Comment 15 Jakob Østergaard 2007-06-07 18:47:55 UTC
>
> We will wait your feedback before to release digiKam 0.9.2-final and close
> this B.K.O file


Just tested; It works for me! :)
Comment 16 caulier.gilles 2007-06-07 19:04:49 UTC
tested too here. Work fine. I close this file.

Gilles