Bug 281761 - 16 -> 8 bit CMYK conversion demonstrably broken
Summary: 16 -> 8 bit CMYK conversion demonstrably broken
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: General (show other bugs)
Version: git master (please specify the git hash!)
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Krita Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-10 16:59 UTC by Sergei Lewis
Modified: 2011-11-11 13:30 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Lewis 2011-09-10 16:59:32 UTC
Version:           svn trunk (using Devel) 
OS:                Linux

Converting between different bit depth CMYK images, changing only the bit depth, completely changes relative proportions of inks in the image, visibly incorrectly.

Reproducible: Always

Steps to Reproduce:
Create a CMYK 16 bits/channel image. Flood fill with pure black (0 cyan / 0 magenta / 0 yellow / 65535 black). Use image... convert image type to change depth to 8 bits/channel. Use colour picker to observe that image is now filled with a rich grey (131C/119M/113Y/204K) instead of pure black!

Actual Results:  
Image filled with a rich grey (131C/119M/113Y/204K)

Expected Results:  
Image filled with a pure black (0C/0M/0Y/255K)
Comment 1 Sven Langkamp 2011-09-10 17:26:42 UTC
Not necessarily a bug. The image is converted to another color profile, depending on the profile the conversion might still be correct.
Comment 2 Sergei Lewis 2011-09-10 18:42:47 UTC
Not (deliberately) changing the colour profile, rendering intent or the colour model - *just* dropping the bit depth to 8 bits per pixel per channel from 16. If this operation results in any changes to the colour data other than quantising it, can that really be correct?
Comment 3 Sven Langkamp 2011-09-12 16:07:31 UTC
Could be, but that needs to be checked in the code.
Comment 4 Cyrille Berger 2011-09-12 18:34:30 UTC
Well I guess the story here is that we create a lcms transformation to go from CMYK 16bits to CMYK 8bits, no matter what. And I would suspect that lcms does not check that source and destination have the same profile, meaning that the image is transformed to an intermediary color space, which would explain the result.
Comment 5 Halla Rempt 2011-09-16 06:07:19 UTC
Hm, I tried to make sure we scale only if colormodel and profile are the same and it's just the depth differing, but I haven't got the solution yet. This patch crashes:

diff --git a/libs/pigment/KoColorSpaceAbstract.h b/libs/pigment/KoColorSpaceAbstract.h
index fb5d56b..6a1a285 100644
--- a/libs/pigment/KoColorSpaceAbstract.h
+++ b/libs/pigment/KoColorSpaceAbstract.h
@@ -25,6 +25,7 @@
 #include <klocale.h>
 
 #include <KoColorSpace.h>
+#include <KoColorProfile.h>
 #include "KoColorSpaceConstants.h"
 #include <KoColorSpaceMaths.h>
 #include <KoColorSpaceRegistry.h>
@@ -100,6 +101,7 @@ public:
         typename _CSTrait::channels_type c = _CSTrait::nativeArray(srcPixel)[channelIndex];
         return KoColorSpaceMaths<typename _CSTrait::channels_type, quint16>::scaleToA(c);
     }
+
     virtual void singleChannelPixel(quint8 *dstPixel, const quint8 *srcPixel, quint32 channelIndex) const {
         _CSTrait::singleChannelPixel(dstPixel, srcPixel, channelIndex);
     }
@@ -149,7 +151,49 @@ public:
     virtual KoID mathToolboxId() const {
         return KoID("Basic");
     }
-};
 
 
+    // Scale the specified channel from the source pixel to T and copy it into the destination pixel.
+    // the template parameter is the type of the source channel
+    template<typename T>
+    void scaleTo(const quint8 *srcPixel, quint8 *dstPixel, qint32 srcChannelIndex, qint32 dstChannelIndex) const {
+        T srcChannel = _CSTrait::nativeArray(srcPixel)[srcChannelIndex];
+        typename _CSTrait::channels_type dstChannel = KoColorSpaceMaths<typename _CSTrait::channels_type, T>::scaleToA(srcChannel);
+        memcpy(dstPixel + (dstChannelIndex * sizeof(srcChannel)), &dstChannel, sizeof(dstChannel));
+    }
+
+    virtual bool convertPixelsTo(const quint8 *src,
+                                 quint8 *dst, const KoColorSpace *dstColorSpace,
+                                 quint32 numPixels,
+                                 KoColorConversionTransformation::Intent renderingIntent = KoColorConversionTransformation::IntentPerceptual) const {
+
+
+
+        // check whether we have the same profile and color model, but only a different bit
+        // depth; in that case we don't convert as such, but scale
+        bool scaleOnly = dstColorSpace->colorModelId().id() == colorModelId().id()
+                && dstColorSpace->colorDepthId().id() != colorDepthId().id()
+                && dstColorSpace->profile()->name() == profile()->name();
+
+        const KoColorSpaceAbstract *cs = static_cast<const KoColorSpaceAbstract*>(dstColorSpace);
+
+        if (scaleOnly && cs) {
+
+            // Use KoColorSpaceMaths and KoColorSpaceTraits to create the right scaling maths
+            // object and just scale the pixels.
+            int numberOfChannels = channelCount();
+            for (uint pixel = 0; pixel < numPixels; ++pixel) {
+                for (int channel = 0; channel < numberOfChannels; ++channel) {
+                    cs->scaleTo<typename _CSTrait::channels_type >(src + (pixel * pixelSize()), dst + (pixel * pixelSize()), channel, channel);
+                }
+            }
+            return true;
+        }
+        else {
+            return KoColorSpace::convertPixelsTo(src, dst, dstColorSpace, numPixels, renderingIntent);
+        }
+
+        return false;
+    }
+};
 #endif


And I don't know why, really...
Comment 6 plassy 2011-11-11 13:30:10 UTC
Git commit a201a7a59f9c8f94e469fbb198774b996c481aca by Silvio Heinrich.
Committed on 11/11/2011 at 14:22.
Pushed by heinrich into branch 'master'.

When converting between colorspaces make sure that a full conversion is only done when more than the bit depth canges.

When converting between two equal colorspaces where only the channel size is
different, the pixel values should only be scaled to the new channel size.
BUG:281761

M  +2    -1    krita/plugins/tools/tool_crop/kis_tool_crop.cc
M  +57   -7    libs/pigment/KoColorSpaceAbstract.h
M  +2    -1    libs/pigment/KoColorSpaceMaths.h

http://commits.kde.org/calligra/a201a7a59f9c8f94e469fbb198774b996c481aca