Bug 99326 – In Skia's ImageFrame, only set the isOpaque flag when the frame is complete

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Reed]

In Skia's ImageFrame, only set the isOpaque flag when the frame is complete

[Mike Reed]

Created attachment 168712 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 168712 [details]
Patch

Attachment 168712 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14292761

[Stephen White]

Looks like this is missing some .h changes?

+abarth for his image decoder expertise.

[Mike Reed]

Created attachment 168724 [details]
Patch

[Mike Reed]

forgot to upload change to ImageDecoder.h

[Stephen White]

Comment on attachment 168724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168724&action=review

This looks reasonable to me, but I'll leave for Adam in case he has any comments.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:55
> +    setHasAlpha(other.hasAlpha());

Is it important that you do the new special logic here? If so, it seems that m_status must be initialized before this is called.  Perhaps you should add a comment to that effect.

[Adam Barth]

> This looks reasonable to me, but I'll leave for Adam in case he has any comments.

Looks reasonable to me too.

[Mike Reed]

Created attachment 168731 [details]
Patch

[Mike Reed]

patch #3 adds comment about calling setHasAlpha after calling setStatus.

[Stephen White]

Comment on attachment 168731 [details]
Patch

OK (bots willing).  r=me

[WebKit Review Bot]

Comment on attachment 168731 [details]
Patch

Rejecting attachment 168731 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14291783

[Mike Reed]

Created attachment 168776 [details]
Patch

[Mike Reed]

reloaded with OOPS. PTAL

[Stephen White]

Comment on attachment 168776 [details]
Patch

Once more, with feeling.  r=me  (Sadly, I think you might need the exclamation mark too).

[Mike Reed]

Created attachment 168777 [details]
Patch

[Mike Reed]

OOPS!

[Stephen White]

Comment on attachment 168777 [details]
Patch

<insert Bill Murray here>  r=me

[WebKit Review Bot]

Comment on attachment 168777 [details]
Patch

Clearing flags on attachment: 168777

Committed r131370: <http://trac.webkit.org/changeset/131370>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #19)
> All reviewed patches have been landed.  Closing bug.

And landed by the time this got to me.

[noel gordon]

Comment on attachment 168777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168777&action=review

> Source/WebCore/ChangeLog:12
> +

Really don't understand these comments (lines 8-11); the tests are passing last I looked.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:74
> +    m_hasAlpha = true;

So if we returned and had to paint the image buffer here, would the opaque setting of m_bitmap be correct given you set m_hasAlpha = true here, viz., would m_bitmap.bitmap().isOpaque() return false at this time.  It's not clear to me where the opaque value was initialized for the m_bitmap, but one of this function or the class constructor seems wrong.

[Mike Reed]

Comment on attachment 168777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168777&action=review

>> Source/WebCore/ChangeLog:12
>> +
> 
> Really don't understand these comments (lines 8-11); the tests are passing last I looked.

This is a chicken-egg problem. We have a hack in skia that works around this bug. If I remove that hack, DRT has tests that will fail, so I have to first fix the webkit code and then do the 2nd half of the fix in skia. If I did the changes in the other order, the tests would fail.. until I also landed the webkit change.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:74
>> +    m_hasAlpha = true;
> 
> So if we returned and had to paint the image buffer here, would the opaque setting of m_bitmap be correct given you set m_hasAlpha = true here, viz., would m_bitmap.bitmap().isOpaque() return false at this time.  It's not clear to me where the opaque value was initialized for the m_bitmap, but one of this function or the class constructor seems wrong.

We will *never* set skia's opaque bit until the image-decode statis FrameComplete, so yes, isOpaque will be false if we stopped right after zeroing, but didn't complete the decode.

I guess we could call setHasAlpha(true) instead of setting this field directly. Might be clearer. I'm not sure that there is a real bug, but I'd be fine with making that change as a new CL.