Bug 89134 – [chromium] Use SkBitmap in ImageLayerChromium

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

[James Robinson]

[chromium] Use SkBitmap in ImageLayerChromium

[James Robinson]

Created attachment 147666 [details]
Patch

[James Robinson]

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.

[Dana Jansens]

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" :)

[Adrienne Walker]

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.

[James Robinson]

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.

[Stephen White]

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.  :)

[Adrienne Walker]

(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.  :)

[James Robinson]

(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.  :)

[James Robinson]

Created attachment 147914 [details]
Patch

[Adrienne Walker]

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.

[James Robinson]

Committed r120507: <http://trac.webkit.org/changeset/120507>