Bug 113184 – Add webp image color profile support

• Component: New Bugs
• Status: RESOLVED FIXED

[noel gordon]

Add webp image color profile support

[noel gordon]

Created attachment 194821 [details]
Patch

[Urvang Joshi]

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.

[noel gordon]

(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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I believe you that you got the color management working. :)  My only review concern is if the code is safe and future/idiot-proof.

[Urvang Joshi]

>> 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.

[noel gordon]

(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.

[Urvang Joshi]

@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.

[noel gordon]

(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

[noel gordon]

Created attachment 195236 [details]
Patch for landing

[noel gordon]

(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.

[Pascal Massimino]

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

[noel gordon]

(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.

[noel gordon]

(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.

[noel gordon]

Created attachment 195446 [details]
Patch fix const nit

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 195446 [details]
Patch fix const nit

Clearing flags on attachment: 195446

Committed r147048: <http://trac.webkit.org/changeset/147048>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.