Bug 25303 – Skia image decoder does not need to ref its internal buffer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Peter Kasting]

This is a cleanup bug; for details see http://code.google.com/p/chromium/issues/detail?id=4679 .

Patch coming shortly.

[Peter Kasting]

Created attachment 29629 [details]
patch v1

Though he's not a WebKit reviewer, [e-mail] wrote this code originally and should probably give an opinion on it.

[Brett Wilson (Google)]

The original reason this code exists is that these are put into a Vector as described in the big comment above RGBA32Buffer. This vector still exists. Can you describe what changed that makes it no longer necessary?

[Peter Kasting]

The reason this code existed is not just because these are put into a vector, but because they are put into a vector _and we used to return raw pointers to the underlying Skia objects_, which could become invalid when the vector copied on resize.

After landing the patch for http://code.google.com/p/chromium/issues/detail?id=4298 , we no longer deal with these raw objects, so it's safe to put things in vectors and have them copied, as other consumers of the image data won't suddenly find their pointers invalidated.

[Brett Wilson (Google)]

Comment on attachment 29629 [details]
patch v1

> -            m_bitmapRef = other.m_bitmapRef;
> +            m_bitmap = other.m_bitmap;
> +            m_bitmap.lockPixels();  // Without this, getAddr32() returns NULL.

This comment reflects the symptom rather than the cause. I would say instead "Always keep the pixels locked since we will be writing directly into it." or something like that.

> -            m_bitmapRef = RefCountedNativeImageSkia::create();
> -            SkBitmap& bmp = bitmap();
> -            const SkBitmap& otherBmp = other.bitmap();
> -            bmp.setConfig(SkBitmap::kARGB_8888_Config, other.width(),
> -                          other.height(), otherBmp.rowBytes());
> -            bmp.allocPixels();
> +            m_bitmap.reset();
> +            const NativeImageSkia& otherBitmap = other.bitmap();
> +            m_bitmap.setConfig(SkBitmap::kARGB_8888_Config, otherBitmap.width(),
> +                              otherBitmap.height(), otherBitmap.rowBytes());
> +            m_bitmap.allocPixels();
>              if (width() > 0 && height() > 0) {
> -                memcpy(bmp.getAddr32(0, 0),
> -                       otherBmp.getAddr32(0, 0),
> -                       otherBmp.rowBytes() * height());
> +                memcpy(m_bitmap.getAddr32(0, 0),
> +                       otherBitmap.getAddr32(0, 0),
> +                       otherBitmap.rowBytes() * height());
>              }
>          }


I think you can replace this function with SkBitmap::copyTo, which didn't exist when we first wrote this, it was added in response to our complaints of having to do this kind of thing :)


Otherwise, this Looks Good To Me.

[Eric Seidel (no email)]

Comment on attachment 29629 [details]
patch v1

You have commit bit.  This looks fine from a WebKit perspective.  Assuming you address Brett's comments when landing, this is r=me.

[Peter Kasting]

Fixed Brett's comments and committed in r43244.