replace etc1_pixel_ref with skia version (SkMallocPixelRef)

replace etc1_pixel_ref with skia version (SkMallocPixelRef)

This is part of a larger effort to replace subclasses of pixelref in chrome/blink with equivalent skia versions, allowing faster iteration/evolution of bitmaps/pixelrefs in skia.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246469

[reed1, 2014-01-15 20:01:28]

need cc owner (+ cq request)

[ccameron, 2014-01-15 20:10:50]

lgtm (w very small nits)

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.cc:43: SkPixelRef* pr =
SkMallocPixelRef::NewAllocate(info, 0, 0);
nit (optional): maybe call this one pixel_ref and pass in the
skia::AdoptRef(pixel_ref) directly.

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.cc:52: const UIResourceBitmap& bitmap) :
bitmap_(bitmap) {
nit: indent 4 spaces

[enne (OOO), 2014-01-15 20:20:38]

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.cc:43: SkPixelRef* pr =
SkMallocPixelRef::NewAllocate(info, 0, 0);
No unwrapped raw Skia pointers, please.  Can you stick this in pixel_ref on the
same line that you allocate it?

[reed1, 2014-01-15 20:46:21]

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.cc:43: SkPixelRef* pr =
SkMallocPixelRef::NewAllocate(info, 0, 0);
On 2014/01/15 20:10:51, ccameron1 wrote:
> nit (optional): maybe call this one pixel_ref and pass in the
> skia::AdoptRef(pixel_ref) directly.

Done.

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.cc:43: SkPixelRef* pr =
SkMallocPixelRef::NewAllocate(info, 0, 0);
On 2014/01/15 20:20:38, enne wrote:
> No unwrapped raw Skia pointers, please.  Can you stick this in pixel_ref on
the
> same line that you allocate it?

Done.

https://codereview.chromium.org/138393002/diff/170001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.cc:52: const UIResourceBitmap& bitmap) :
bitmap_(bitmap) {
On 2014/01/15 20:10:51, ccameron1 wrote:
> nit: indent 4 spaces

Done.

[enne (OOO), 2014-01-15 21:04:50]

Thanks! lgtm2

[commit-bot: I haz the power, 2014-01-15 21:31:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/138393002/260001

[commit-bot: I haz the power, 2014-01-15 23:49:35]

Retried try job too often on linux_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[aelias_OOO_until_Jul13, 2014-01-16 00:40:23]

I like the switch to SkMallocRef but this patch also defeats some sharing
semantics that we need on Android.  We need to be able to create the SkPixelRef
earlier and share it with a UIResourceBitmap without incurring a copy.  Could
you make the UIResourceBitmap constructor take a const
skia::RefPtr<SkMallocRef>& similarly to how it was before?

[mikerreed, 2014-01-16 02:02:17]

Hal, we need to export SkMallocPixelRef so that sharedlib builds can work... I
presume just adding SK_API to the class.

[reed1, 2014-01-21 14:15:20]

On 2014/01/16 00:40:23, aelias wrote:
> I like the switch to SkMallocRef but this patch also defeats some sharing
> semantics that we need on Android.  We need to be able to create the
SkPixelRef
> earlier and share it with a UIResourceBitmap without incurring a copy.  Could
> you make the UIResourceBitmap constructor take a const
> skia::RefPtr<SkMallocRef>& similarly to how it was before?

I did not intend to introduce any additional copies. Can you show me how I did
that, and I will fix it.

[aelias_OOO_until_Jul13, 2014-01-21 19:33:17]

On 2014/01/21 14:15:20, reed1 wrote:
> On 2014/01/16 00:40:23, aelias wrote:
> > I like the switch to SkMallocRef but this patch also defeats some sharing
> > semantics that we need on Android.  We need to be able to create the
> SkPixelRef
> > earlier and share it with a UIResourceBitmap without incurring a copy. 
Could
> > you make the UIResourceBitmap constructor take a const
> > skia::RefPtr<SkMallocRef>& similarly to how it was before?
> 
> I did not intend to introduce any additional copies. Can you show me how I did
> that, and I will fix it.

We want to allocate the memory for the ETC1 bitmap before the UIResourceBitmap
object is created, and also continue to hold onto it after the UIResourceBitmap
is destroyed.  The plan was to allocate an SkPixelRef outside of it and pass a
RefPtr to the UIResourceBitmap.  With your changes, UIResourceBitmap allocates
pixels itself, so we would instead have to lock it and copy the pixels.  (Note
that the code I'm describing isn't landed yet, powei@ is working on it.)

[reed1, 2014-01-21 19:43:45]

On 2014/01/21 19:33:17, aelias wrote:
> On 2014/01/21 14:15:20, reed1 wrote:
> > On 2014/01/16 00:40:23, aelias wrote:
> > > I like the switch to SkMallocRef but this patch also defeats some sharing
> > > semantics that we need on Android.  We need to be able to create the
> > SkPixelRef
> > > earlier and share it with a UIResourceBitmap without incurring a copy. 
> Could
> > > you make the UIResourceBitmap constructor take a const
> > > skia::RefPtr<SkMallocRef>& similarly to how it was before?
> > 
> > I did not intend to introduce any additional copies. Can you show me how I
did
> > that, and I will fix it.
> 
> We want to allocate the memory for the ETC1 bitmap before the UIResourceBitmap
> object is created, and also continue to hold onto it after the
UIResourceBitmap
> is destroyed.  The plan was to allocate an SkPixelRef outside of it and pass a
> RefPtr to the UIResourceBitmap.  With your changes, UIResourceBitmap allocates
> pixels itself, so we would instead have to lock it and copy the pixels.  (Note
> that the code I'm describing isn't landed yet, powei@ is working on it.)

The mallocpixelref can be shared from the UIResourceBitmap without copying, just
call ref() on it and pass it to another SkBitmap. Perhaps I'm saying that we can
add whatever new constructor to UIResourceBitmap is needed when the time comes.
Do you think that would work?

[aelias_OOO_until_Jul13, 2014-01-21 21:49:35]

I suppose, but I don't see why we can't fix it now, as the new constructor
you're adding isn't going to be used.  Could you please replace it with:

UIResourceBitmap::UIResourceBitmap(const skia::RefPtr<SkMallocPixelRef>&
etc1_pixel_ref) {
  Create(etc1_pixel_ref, ETC1);
}

Then this patch would be fine by me.

[reed1, 2014-01-21 22:26:18]

On 2014/01/21 21:49:35, aelias wrote:
> I suppose, but I don't see why we can't fix it now, as the new constructor
> you're adding isn't going to be used.  Could you please replace it with:
> 
> UIResourceBitmap::UIResourceBitmap(const skia::RefPtr<SkMallocPixelRef>&
> etc1_pixel_ref) {
>   Create(etc1_pixel_ref, ETC1);
> }
> 
> Then this patch would be fine by me.

Done

[aelias_OOO_until_Jul13, 2014-01-21 22:34:04]

lgtm, thanks!

[commit-bot: I haz the power, 2014-01-22 14:15:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/138393002/590001

[commit-bot: I haz the power, 2014-01-22 14:44:35]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-22 15:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/138393002/600004

[commit-bot: I haz the power, 2014-01-22 17:18:54]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-22 20:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/138393002/700004

[commit-bot: I haz the power, 2014-01-23 01:15:28]

Change committed as 246469