Bug 78498 – [chromium] Set opaque flag on SkBitmap in per-tile layer updater

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Set opaque flag on SkBitmap in per-tile layer updater

[Dana Jansens]

Created attachment 126773 [details]
Patch

[Dana Jansens]

Provide opaque information to skia so it can perform optimizations now or in the future.

[Stephen White]

Comment on attachment 126773 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.h:52
> +    virtual void setOpaque(bool);
> +

Out of curiosity, why does this need to be public now?  It doesn't seem to be called from anywhere new.

[Dana Jansens]

Comment on attachment 126773 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.h:52
>> +
> 
> Out of curiosity, why does this need to be public now?  It doesn't seem to be called from anywhere new.

It should have been public to start with. The function its overriding in LayerTextureUpdater.h is public.

[Stephen White]

Comment on attachment 126773 [details]
Patch

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

Looks good.  r=me

>>> Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.h:52
>>> +
>> 
>> Out of curiosity, why does this need to be public now?  It doesn't seem to be called from anywhere new.
> 
> It should have been public to start with. The function its overriding in LayerTextureUpdater.h is public.

Ah, thanks for the explanation.

[Mike Reed]

probably better to set the opaqueness *after* the call to allocPixels(). I think this may work today, but it is likely we'll want to pass this hint onto the pixelref, which we don't have until the alloc call.

[Dana Jansens]

Would the bitmap not pass the current isOpaque() value on to the pixelref when the pixelref becomes available?

[Mike Reed]

it may. I am just thinking about future evolution of the API.

[Dana Jansens]

Created attachment 126795 [details]
Patch

reversed the call order to skbitmap as per @reed's comments.

[noel gordon]

Kinda reminds me of http://crbug.com/113171, where calling setIsOpaque() on bitmap lead to all sorts of trouble.  Maybe I worry too much?

[WebKit Review Bot]

Comment on attachment 126795 [details]
Patch

Clearing flags on attachment: 126795

Committed r107669: <http://trac.webkit.org/changeset/107669>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

Looks like a fun bug. I think we'll be okay. We fill the bitmap with things before using the pixels here, as they are used for texture upload.

[noel gordon]

Yeah fun bug, and very glad to hear you don't have it :)