| Summary: | [chromium] Set opaque flag on SkBitmap in per-tile layer updater | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dana Jansens <danakj> | ||||||
| Component: | New Bugs | Assignee: | Dana Jansens <danakj> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | backer, cc-bugs, jamesr, noel.gordon, piman, reed, reveman, senorblanco, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Dana Jansens
2012-02-13 08:05:16 PST
Created attachment 126773 [details]
Patch
Provide opaque information to skia so it can perform optimizations now or in the future. 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 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 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. 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. Would the bitmap not pass the current isOpaque() value on to the pixelref when the pixelref becomes available? it may. I am just thinking about future evolution of the API. Created attachment 126795 [details]
Patch
reversed the call order to skbitmap as per @reed's comments.
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 on attachment 126795 [details] Patch Clearing flags on attachment: 126795 Committed r107669: <http://trac.webkit.org/changeset/107669> All reviewed patches have been landed. Closing bug. 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. Yeah fun bug, and very glad to hear you don't have it :) |