Summary: | Add RGB -> CMYK conversion algorithm | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Scott Violet <sky> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Scott Violet <sky> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, mitz | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Windows XP | ||||||||||||||
Attachments: |
|
Description
Scott Violet
2009-01-21 14:32:59 PST
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? 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. 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. 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.
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.
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.
Created attachment 27105 [details]
Patch for RGB -> CMYK converter
Updated patch adding ASSERT and reviewd by nobody.
Thanks!
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!
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 @@ > +2009-01-21 Scott Violet <sky@google.com> > + > + 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. 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? Even if it is unused on some platforms at the moment I see no reason not to do in Color. Created attachment 27152 [details]
Adds Color(CMYKA) constructor
Created attachment 27360 [details]
Last patch had a bug, it used setFillColor when it should have used setStrokeColor
Extra fix landed as http://trac.webkit.org/changeset/40688. |