Bug 78498 - [chromium] Set opaque flag on SkBitmap in per-tile layer updater
Summary: [chromium] Set opaque flag on SkBitmap in per-tile layer updater
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 08:05 PST by Dana Jansens
Modified: 2012-02-14 18:04 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2012-02-13 08:06 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2012-02-13 10:47 PST, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-02-13 08:05:16 PST
[chromium] Set opaque flag on SkBitmap in per-tile layer updater
Comment 1 Dana Jansens 2012-02-13 08:06:21 PST
Created attachment 126773 [details]
Patch
Comment 2 Dana Jansens 2012-02-13 08:58:43 PST
Provide opaque information to skia so it can perform optimizations now or in the future.
Comment 3 Stephen White 2012-02-13 09:00:49 PST
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.
Comment 4 Dana Jansens 2012-02-13 09:03:00 PST
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.
Comment 5 Stephen White 2012-02-13 09:41:00 PST
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.
Comment 6 Mike Reed 2012-02-13 10:10:36 PST
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.
Comment 7 Dana Jansens 2012-02-13 10:20:53 PST
Would the bitmap not pass the current isOpaque() value on to the pixelref when the pixelref becomes available?
Comment 8 Mike Reed 2012-02-13 10:29:21 PST
it may. I am just thinking about future evolution of the API.
Comment 9 Dana Jansens 2012-02-13 10:47:39 PST
Created attachment 126795 [details]
Patch

reversed the call order to skbitmap as per @reed's comments.
Comment 10 noel gordon 2012-02-13 21:24:48 PST
Kinda reminds me of http://crbug.com/113171, where calling setIsOpaque() on bitmap lead to all sorts of trouble.  Maybe I worry too much?
Comment 11 WebKit Review Bot 2012-02-13 21:57:42 PST
Comment on attachment 126795 [details]
Patch

Clearing flags on attachment: 126795

Committed r107669: <http://trac.webkit.org/changeset/107669>
Comment 12 WebKit Review Bot 2012-02-13 21:57:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Dana Jansens 2012-02-14 07:54:12 PST
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.
Comment 14 noel gordon 2012-02-14 18:04:21 PST
Yeah fun bug, and very glad to hear you don't have it :)