Bug 113184

Summary: Add webp image color profile support
Product: WebKit Reporter: noel gordon <noel.gordon>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch for landing
none
Patch fix const nit none

Description noel gordon 2013-03-25 03:36:39 PDT
Add webp image color profile support
Comment 1 noel gordon 2013-03-25 04:06:34 PDT
Created attachment 194821 [details]
Patch
Comment 2 Urvang Joshi 2013-03-25 11:10:50 PDT
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.
Comment 3 noel gordon 2013-03-26 01:07:26 PDT
(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 4 Eric Seidel (no email) 2013-03-26 01:12:56 PDT
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
Comment 5 Eric Seidel (no email) 2013-03-26 01:13:54 PDT
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.
Comment 6 Eric Seidel (no email) 2013-03-26 01:14:29 PDT
I believe you that you got the color management working. :)  My only review concern is if the code is safe and future/idiot-proof.
Comment 7 Urvang Joshi 2013-03-26 02:15:44 PDT
>> 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.
Comment 8 noel gordon 2013-03-26 20:25:34 PDT
(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.
Comment 9 Urvang Joshi 2013-03-26 22:28:25 PDT
@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.
Comment 10 noel gordon 2013-03-26 23:42:17 PDT
(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
Comment 11 noel gordon 2013-03-27 00:55:14 PDT
Created attachment 195236 [details]
Patch for landing
Comment 12 noel gordon 2013-03-27 02:07:36 PDT
(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 13 Pascal Massimino 2013-03-27 05:08:16 PDT
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
Comment 14 noel gordon 2013-03-27 05:19:47 PDT
(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.
Comment 15 noel gordon 2013-03-27 05:22:16 PDT
(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.
Comment 16 noel gordon 2013-03-27 17:27:24 PDT
Created attachment 195446 [details]
Patch fix const nit
Comment 17 Eric Seidel (no email) 2013-03-27 19:48:25 PDT
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 18 WebKit Review Bot 2013-03-27 20:03:06 PDT
Comment on attachment 195446 [details]
Patch fix const nit

Clearing flags on attachment: 195446

Committed r147048: <http://trac.webkit.org/changeset/147048>
Comment 19 WebKit Review Bot 2013-03-27 20:03:15 PDT
All reviewed patches have been landed.  Closing bug.