Bug 146464

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).

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 :)

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

Created attachment 20794 [details]
Patch 1/2

Implements test for equality between raw decoding parameters

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.

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.

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(){};

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(){};

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

>
> 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

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);
 }
 

Ok, this is want mean than a new release of libkdcraw need to be done accordinly with 0.9.2 digiKam release

Gilles

Thanks Marcel

I see that I completely misunderstood the possibleCacheKeys function :)

I'll try and test the changes tonight.

Thanks Jakob.

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

Gilles

>
> 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! :)

tested too here. Work fine. I close this file.

Gilles