Bug 25303 - Skia image decoder does not need to ref its internal buffer
Summary: Skia image decoder does not need to ref its internal buffer
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Peter Kasting
Depends on:
Reported: 2009-04-20 16:38 PDT by Peter Kasting
Modified: 2009-05-05 10:46 PDT (History)
1 user (show)

See Also:

patch v1 (8.63 KB, patch)
2009-04-20 16:42 PDT, Peter Kasting
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-04-20 16:38:24 PDT
This is a cleanup bug; for details see http://code.google.com/p/chromium/issues/detail?id=4679 .

Patch coming shortly.
Comment 1 Peter Kasting 2009-04-20 16:42:48 PDT
Created attachment 29629 [details]
patch v1

Though he's not a WebKit reviewer, brettw@chromium.org wrote this code originally and should probably give an opinion on it.
Comment 2 Brett Wilson (Google) 2009-04-22 10:34:39 PDT
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?
Comment 3 Peter Kasting 2009-04-22 14:01:45 PDT
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.
Comment 4 Brett Wilson (Google) 2009-05-04 19:23:54 PDT
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.
Comment 5 Eric Seidel (no email) 2009-05-04 20:20:41 PDT
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.
Comment 6 Peter Kasting 2009-05-05 10:46:29 PDT
Fixed Brett's comments and committed in r43244.