[chromium] Use SkBitmap in ImageLayerChromium
Created attachment 147666 [details] Patch
I'm using pixelRef() to see if two SkBitmaps represent the same underlying thing - can you tell me if that's crazy or not, Stephen? It appears to work for animated .gifs which is the case that's most relevant here.
Comment on attachment 147666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147666&action=review > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:-147 > -void ImageLayerChromium::setContents(Image* contents) yay for removing the word "contents" :)
Comment on attachment 147666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147666&action=review > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:469 > + NativeImageSkia* nativeImage = image->nativeImageForCurrentFrame(); > + imageLayer->setBitmap(nativeImage->bitmap()); What's the lifetime of the pixels in image->nativeImageForCurrentFrame()->bitmap() with respect to ImageLayerChromium's use of the SkBitmap's pixels? I don't understand SkBitmap enough to know what happens when the source of a copy goes away.
SkBitmap holds a reference to the underlying data, I believe (right Stephen), so by holding an SkBitmap I believe I'm keeping the actual image data alive.
Comment on attachment 147666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147666&action=review >> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:469 >> + imageLayer->setBitmap(nativeImage->bitmap()); > > What's the lifetime of the pixels in image->nativeImageForCurrentFrame()->bitmap() with respect to ImageLayerChromium's use of the SkBitmap's pixels? I don't understand SkBitmap enough to know what happens when the source of a copy goes away. There are two cases: SkBitmaps can be exclusive owners of their pixels (where they have a non-NULL fPixels but a NULL fPixelRef), or they can have shared (non-exclusive) ownership, via fPixelRef. The SkBitmap copy constructor will simply incref the SkPixelRef in the latter case, which I believe is the case for all bitmaps owned by NativeImageSkia. > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:139 > + if (bitmap.pixelRef() == m_bitmap.pixelRef()) As above, this will work as long as the SkBitmaps have non-exclusive pixel ownership, which I believe is the case here. If you want to be conservative, you could do: if (bitmap.pixelRef() && bitmap.pixelRef() == m_bitmap.pixelRef()) return; > Source/WebCore/platform/graphics/chromium/PlatformImage.cpp:-60 > -} // namespace WebCore Yay for negative linecount patches. :)
(In reply to comment #6) > (From update of attachment 147666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147666&action=review > > >> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:469 > >> + imageLayer->setBitmap(nativeImage->bitmap()); > > > > What's the lifetime of the pixels in image->nativeImageForCurrentFrame()->bitmap() with respect to ImageLayerChromium's use of the SkBitmap's pixels? I don't understand SkBitmap enough to know what happens when the source of a copy goes away. > > There are two cases: SkBitmaps can be exclusive owners of their pixels (where they have a non-NULL fPixels but a NULL fPixelRef), or they can have shared (non-exclusive) ownership, via fPixelRef. > > The SkBitmap copy constructor will simply incref the SkPixelRef in the latter case, which I believe is the case for all bitmaps owned by NativeImageSkia. Thanks for the explanation. :)
(In reply to comment #6) > (From update of attachment 147666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147666&action=review > > >> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:469 > >> + imageLayer->setBitmap(nativeImage->bitmap()); > > > > What's the lifetime of the pixels in image->nativeImageForCurrentFrame()->bitmap() with respect to ImageLayerChromium's use of the SkBitmap's pixels? I don't understand SkBitmap enough to know what happens when the source of a copy goes away. > > There are two cases: SkBitmaps can be exclusive owners of their pixels (where they have a non-NULL fPixels but a NULL fPixelRef), or they can have shared (non-exclusive) ownership, via fPixelRef. > > The SkBitmap copy constructor will simply incref the SkPixelRef in the latter case, which I believe is the case for all bitmaps owned by NativeImageSkia. > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:139 > > + if (bitmap.pixelRef() == m_bitmap.pixelRef()) > > As above, this will work as long as the SkBitmaps have non-exclusive pixel ownership, which I believe is the case here. I believe that's the case for this as well, but I'll add the defensive check anyway (since I plan to expose this to the general compositor API and people might pass in other sorts of bitmaps). > > If you want to be conservative, you could do: > > if (bitmap.pixelRef() && bitmap.pixelRef() == m_bitmap.pixelRef()) > return; Will do. Mind setting the review flag if you're happy otherwise? :) > > > Source/WebCore/platform/graphics/chromium/PlatformImage.cpp:-60 > > -} // namespace WebCore > > Yay for negative linecount patches. :)
Created attachment 147914 [details] Patch
Comment on attachment 147914 [details] Patch R=me. I don't think there's anything controversial here. If senorblanco wants to change something about this, there can always be a follow-up patch.
Committed r120507: <http://trac.webkit.org/changeset/120507>