Summary: | Add webp image color profile support | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | noel gordon <noel.gordon> | ||||||||
Component: | New Bugs | Assignee: | noel gordon <noel.gordon> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, eric, gyuyoung.kim, rakuco, urvang, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 112761 | ||||||||||
Bug Blocks: | 113124 | ||||||||||
Attachments: |
|
Description
noel gordon
2013-03-25 03:36:39 PDT
Created attachment 194821 [details]
Patch
It doesn't seem to let me add comments, so writing here directly (for patch ) > #if USE(QCMSLIB) && (WEBP_DECODER_ABI_VERSION > 0x200) BUG! Should be 0x201 > if ((m_formatFlags & ICCP_FLAG) && !ignoresGammaAndColorProfile()) Refactor this inside applycolorprofile() > 232 if (state <= WEBP_DEMUX_PARSING_HEADER) > 233 return false; Move to 225, as width/height don't need to be read if we haven't parsed the header. > int m_decodedHeight; Pls put a comment explaining this variable. (In reply to comment #2) > It doesn't seem to let me add comments, so writing here directly (for patch ) > > > #if USE(QCMSLIB) && (WEBP_DECODER_ABI_VERSION > 0x200) > > BUG! Should be 0x201 Nope, no bug. The code says "greater than" 0x200. The tests are passing. > > if ((m_formatFlags & ICCP_FLAG) && !ignoresGammaAndColorProfile()) > > Refactor this inside applycolorprofile() I don't see why. applyColorProfile() is not called "maybeApplyColorProfile()", and writing the code this way is consistent with the uses elsewhere in the decode(onlySize) routine. It also allows the other webp-using WebKit ports with earlier libwebp versions to optimize out (remove) the if test and the function call code. > > 232 if (state <= WEBP_DEMUX_PARSING_HEADER) > > 233 return false; > > Move to 225, as width/height don't need to be read if we haven't parsed the header. Nor are they used unless we parse the header viz., I've coded here for the common case. > > int m_decodedHeight; > > Pls put a comment explaining this variable. Somewhat vague. Maybe you mean "// The decoded height of the image." or something, but it's not webkit style to comment on member variables this way. The use of WebPIDecGetRGB() and decoded height is covered in detail in bug 74062 (there is already a comment in my patch referring to that bug), and then again in the libwebp *.h files describing the WebPIDecGetRGB API. I'm not sure why it needs more explanation. Comment on attachment 194821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194821&action=review I'll have to read this again when it's not 1am and I'm getting up in 6 hours. :) > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:140 > + qcms_profile_release(inputProfile); We can use a ReleasePtr<> specialization if we do this often. > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:180 > + return; // https://bugs.webkit.org/show_bug.cgi?id=74062 Thsi is a FIXME I assume? > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:198 > + buffer.setRGBA(x, y, pixel[0], pixel[1], pixel[2], pixel[3]); We don't have a setRGBA which takes uint8_t[4]? > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:63 > + qcms_transform* colorTransform() { return m_transform; } > + void createColorTransform(const char* data, size_t); > + void readColorProfile(const uint8_t* data, size_t); > + void applyColorProfile(const uint8_t* data, size_t, ImageFrame&); Bleh. I assume these are called from QCMS code and thus need to use ugly pointer/length pairs? :p My main concern is going to be... well, the same as it is for every patch which touches C-APIs: did we get the manual memory management right, and written in a sane fashion such that future folks in this code won't shoot themselves in the foot. I believe you that you got the color management working. :) My only review concern is if the code is safe and future/idiot-proof. >> It doesn't seem to let me add comments, so writing here directly (for patch ) >> >> > #if USE(QCMSLIB) && (WEBP_DECODER_ABI_VERSION > 0x200) >> >> BUG! Should be 0x201 > Nope, no bug. The code says "greater than" 0x200. The tests are passing. The problem case is == 0x200 case --> QCMS_COLOR_CORRECTION should NOT be defined in that case. The patch would NOT build for the earlier libwebp 0.2.1 (where DECODER_ABI_VERSION was 0x200 and color profile support wasn't present) for that reason. > >> int m_decodedHeight; > > > > Pls put a comment explaining this variable. > Somewhat vague. Maybe you mean "// The decoded height of the image." or > something, but it's not webkit style to comment on member variables this way. Sure, the meaning & purpose of most of the variables is clear by name. I was just suggesting that the reason for this may be non-trivial, and worth a comment. I'll leave that to you. (In reply to comment #7) > >> It doesn't seem to let me add comments, so writing here directly (for patch ) > >> > >> > #if USE(QCMSLIB) && (WEBP_DECODER_ABI_VERSION > 0x200) > >> > >> BUG! Should be 0x201 > > > Nope, no bug. The code says "greater than" 0x200. The tests are passing. > > The problem case is == 0x200 case --> QCMS_COLOR_CORRECTION should NOT be defined in that case. What does (WEBP_DECODER_ABI_VERSION > 0x200) evaluate to in this case? How would QCMS_COLOR_CORRECTION be defined therefore? > The patch would NOT build for the earlier libwebp 0.2.1 (where DECODER_ABI_VERSION was 0x200 and color profile support wasn't present) for that reason. USE(QCMSLIB) is false on the GTK and QT ports so QCMS_COLOR_CORRECTION is not defined for them. They use earlier versions of libwebp (0.1.x, 0.2.0) and the green bubbles above show they do in fact build. @Noel: I was thinking about the case wehre ABI_VERSION == 0x200 and USE(QCMSLIB) is true. But that's handled correctly. Pardon my ignorance: should have seen the sign is strict greater than, after ur last comment Anyway, LGTM from my side. (In reply to comment #4, #5, #6) > (From update of attachment 194821 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194821&action=review > > I'll have to read this again when it's not 1am and I'm getting up in 6 hours. :) > > > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:140 > > + qcms_profile_release(inputProfile); > > We can use a ReleasePtr<> specialization if we do this often. Perhaps you meant deleteOwnPtr, but we don't do this often enough to warrant it I believe. > > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:180 > > + return; // https://bugs.webkit.org/show_bug.cgi?id=74062 > > This is a FIXME I assume? Not a FIXME. Will change it to // See also https://bugs ... it's a reference to usage discussion re: the WebPIDecGetRGB() api. > > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:198 > > + buffer.setRGBA(x, y, pixel[0], pixel[1], pixel[2], pixel[3]); > > We don't have a setRGBA which takes uint8_t[4]? Nope. > > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:63 > > + qcms_transform* colorTransform() { return m_transform; } > > + void createColorTransform(const char* data, size_t); > > + void readColorProfile(const uint8_t* data, size_t); > > + void applyColorProfile(const uint8_t* data, size_t, ImageFrame&); > > Bleh. I assume these are called from QCMS code and thus need to use ugly pointer/length pairs? :p One third party library wants uint8_t, another wants const char*, and I'm in the middle of said parties putting more party into their party :) (In reply to comment #5) > My main concern is going to be... well, the same as it is for every patch which touches C-APIs: did we get the manual memory management right, and written in a sane fashion such that future folks in this code won't shoot themselves in the foot. I believe the mem management is correct, it follows what we do in JPEG and PNG decoders. (In reply to comment #6) > I believe you that you got the color management working. :) The new layout tests I've added seem to confirm it. > My only review concern is if the code is safe and future/idiot-proof. Safe: sec-review for webp 0.3.0 is a chromium concern, and we have chromium ASAN bots for memory issues if any. Idiot-proof? Dunno. The libwebp API lacks a row callback during decoding. That makes the current task (color profiles and pixel color correction) way more complicated than it need be IMHO. See https://bugs.webkit.org/show_bug.cgi?id=93430#c17 Created attachment 195236 [details]
Patch for landing
(In reply to comment #10) > Safe: sec-review for webp 0.3.0 is a chromium concern, and we have chromium ASAN bots for memory issues if any. ... and valgrind bots as well. Comment on attachment 195236 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=195236&action=review LGTM. > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:60 > + qcms_transform* colorTransform() { return m_transform; } nit: is a 'const' method (In reply to comment #13) > > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:60 > > + qcms_transform* colorTransform() { return m_transform; } > > nit: is a 'const' method Agree, will fix that in a follow-up if this lands before I'm awake. (In reply to comment #7) > > > Pls put a comment explaining this variable. > > > Somewhat vague. Maybe you mean "// The decoded height of the image." or > > something, but it's not webkit style to comment on member variables this way. > > Sure, the meaning & purpose of most of the variables is clear by name. > I was just suggesting that the reason for this may be non-trivial, and worth a comment. > I'll leave that to you. Added some comments about this in the patch-for-landing ChangeLog. Created attachment 195446 [details]
Patch fix const nit
Comment on attachment 195446 [details] Patch fix const nit View in context: https://bugs.webkit.org/attachment.cgi?id=195446&action=review This is fine. No worse than the rest of the code in this file. C-memory management is poop. Thanks for following up. > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:83 > + if (m_transform) > + qcms_transform_release(m_transform); Ick. I could have sworn we had a nice way to do this with RetainPtr specializations, but I don't see it now. > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:124 > + if (m_transform) > + qcms_transform_release(m_transform); > + m_transform = 0; Perhaps we should have made this a releaseTransform() call at least. Comment on attachment 195446 [details] Patch fix const nit Clearing flags on attachment: 195446 Committed r147048: <http://trac.webkit.org/changeset/147048> All reviewed patches have been landed. Closing bug. |