Bug 23462 – Add RGB -> CMYK conversion algorithm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Scott Violet]

CanvasStyle currently only handles RGB -> CMYK conversion for CG and QT. We should add a fallback algorithm for use on other platforms.

[Scott Violet]

Created attachment 26907 [details]
Patch for RGB -> CMYK converter

As this converter gives different results than the CG one, at least one layout test produces different results. I was tempted to modify shouldBe in fast/js/resources/js-test-pre.js to accept another parameter that gives another possible accepted value, but wanted to make sure you want that before writing it. Let me know if you want me to.

Comment on attachment 26907 [details]
Patch for RGB -> CMYK converter

> +    int r = static_cast<int>(255 * (colors * (1 - m_cyan)));

This is suspicious, as it maps only one value to 255. Shouldn't nextafterf(256, 0) be used instead of 255?

[Scott Violet]

I must be missing something. colors is only 0-1 so that I believe we want 255 here.

(In reply to comment #3)
> I must be missing something. colors is only 0-1 so that I believe we want 255
> here.

I think you want to divide the interval [0,1] into 255 equal intervals, and map the interval [254/255, 1] to 255. However, static_cast<int>(255 * (colors * (1 - m_cyan))) will map [254/255, 1) to 254 and only the value 1 will map to 255.

(In reply to comment #4)
> (In reply to comment #3)
> > I must be missing something. colors is only 0-1 so that I believe we want 255
> > here.
> 
> I think you want to divide the interval [0,1] into 255 equal intervals, and map
> the interval [254/255, 1] to 255. However, static_cast<int>(255 * (colors * (1
> - m_cyan))) will map [254/255, 1) to 254 and only the value 1 will map to 255.

Sorry, I meant to write:
You want to divide the interval [0, 1] into 256 equal intervals, and map the interval [255/256, 1] to 255.

[Scott Violet]

I'm still confused.

If I have a value x in the range [0-1] and need to map that to the range [0-n], then I multiply x by n. Perhaps I'm not understanding your syntax though. Could you post what you think the code should look like?

(In reply to comment #6)
> I'm still confused.
> 
> If I have a value x in the range [0-1] and need to map that to the range [0-n],
> then I multiply x by n. Perhaps I'm not understanding your syntax though. Could
> you post what you think the code should look like?

Multiplication by n is correct if you are mapping floating point numbers to floating point numbers, but your mapping is to the set of integers {0, 1, ..., n}. static_cast<int>(n * x) truncates the result of the multiplication, meaning the only x value mapping to n is 1. I think static_cast<int>(nextafter(n, 0) * x) would map intervals of equal length to each integer in the set.

[Scott Violet]

Created attachment 27080 [details]
Patch for RGB -> CMYK converter

macdome showed me the error of my ways. New patch is attached. I went with nextafter instead of nextafterf as nextafterf doesn't give the behavior you mentioned.

[Eric Seidel (no email)]

Comment on attachment 27080 [details]
Patch for RGB -> CMYK converter

The cmykToRGB function should at least ASSERT that m_type is CMYK.  Another way would be to make it a free function which took the 4 arguments, but this is probably less error-prone as is (if you add the ASSERT.

I'll mark your first patch as obsolete since this one supersedes it.

[Eric Seidel (no email)]

Comment on attachment 27080 [details]
Patch for RGB -> CMYK converter

Oh, you also can leave the "Reviewed by nobody (OOPS!). line in when posting for review.

[Scott Violet]

Created attachment 27105 [details]
Patch for RGB -> CMYK converter

Updated patch adding ASSERT and reviewd by nobody.

Thanks!

[Eric Seidel (no email)]

Comment on attachment 27105 [details]
Patch for RGB -> CMYK converter

Yeah, mitz should probably complete the review here.  I'm unsure if the alpha conversion is correct either.  I think that needs to use nextafter(256,0) as well.  If that's true, that's simple and can be corrected when landing.  Thanks for the update!

[Sam Weinig]

Comment on attachment 27105 [details]
Patch for RGB -> CMYK converter

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 40097)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Scott Violet  <[e-mail]>
> +
> +        Reviewed by nobody (OOPS!)
> +
> +	Bug 23462: Add RGB -> CMYK conversion algorithm
> +	<https://bugs.webkit.org/show_bug.cgi?id=23462>
> +
> +	Adds an RGB -> CMYK converter. This isn't perfect, but better than
> +	nothing.

Please remove tabs.  Doesn't this actually add a cmyk -> rgb conversion?

>  
> +Color CanvasStyle::cmykToRGB() const
> +{
> +    ASSERT(m_type == CMYKA);
> +    double colors = 1 - m_black;
> +    int r = static_cast<int>(nextafter(256, 0) * (colors * (1 - m_cyan)));
> +    int g = static_cast<int>(nextafter(256, 0) * (colors * (1 - m_magenta)));
> +    int b = static_cast<int>(nextafter(256, 0) * (colors * (1 - m_yellow)));
> +    return Color(r, g, b, static_cast<int>(m_alpha * 255));
> +}

This seems much more suited to be a Color constructor.

r-.  Please make the function a color constructor.

[Scott Violet]

Just to be sure I understand what you're after. You're asking for a

Color(float c, float m, float y, float k, float a)

constructor, right? And this would have the code that I currently have in CanvasStyle::cmykToRGB, right?

The CMYK color conversion seems platform specific though, are you sure you want to hoist this into Color?

[Sam Weinig]

Even if it is unused on some platforms at the moment I see no reason not to do in Color.

[Scott Violet]

Created attachment 27152 [details]
Adds Color(CMYKA) constructor

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/40673

[Scott Violet]

Created attachment 27360 [details]
Last patch had a bug, it used setFillColor when it should have used setStrokeColor

[Dimitri Glazkov (Google)]

Extra fix landed as http://trac.webkit.org/changeset/40688.