Use RefPtr for SkBitmap::Allocator in ImageDecoder

Use RefPtr for SkBitmap::Allocator in ImageDecoder

ImageFrame:
- Store m_allocator in a RefPtr. (This fixes a potential leak, since we
previously did not ref/deref it. In practice, we always removed it
beforehand anyway.)
- Remove accessor for m_allocator, which was only used by operator=
(which has access to member variables)

ImageDecoder:
- Use a PassRefPtr<SkBitmap::Allocator> in setMemoryAllocator, instead
of a raw pointer.
- Make setMemoryAllocator no longer virtual, since it has no overrides.

ImageFrameGenerator:
- Use a PassRefPtr for methods taking an SkBitmap::Allocator, and
release the ExternalMemoryAllocator to them.

No change in behavior, no new tests

[scroggo_chromium, 2016-04-14 18:22:30]

scroggo@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2016-04-14 19:52:54]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 19:53:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880993003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880993003/1

[commit-bot: I haz the power, 2016-04-14 19:55:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 19:55:44]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[f(malita), 2016-04-14 20:27:54]

Description was changed from

==========
Use RefPtr for SkBitmap::Allocator in ImageDecoder

ImageFrame:
- Store m_allocator in a RefPtr. (This fixes a potential leak, since we
previously did not ref/deref it. In practice, we always removed it
beforehand anyway.)
- Remove accessor for m_allocator, which was only used by operator=
(which has access to member variables)

ImageDecoder:
- Use a PassRefPtr<SkBitmap::Allocator> in setMemoryAllocator, instead
of a raw pointer.
- Make setMemoryAllocator no longer virtual, since it has no overrides.

ImageFrameGenerator:
- Use a PassRefPtr for methods taking an SkBitmap::Allocator, and
release the ExternalMemoryAllocator to them.

No change in behavior, no new tests
==========

to

==========
Use RefPtr for SkBitmap::Allocator in ImageDecoder

ImageFrame:
- Store m_allocator in a RefPtr. (This fixes a potential leak, since we
previously did not ref/deref it. In practice, we always removed it
beforehand anyway.)
- Remove accessor for m_allocator, which was only used by operator=
(which has access to member variables)

ImageDecoder:
- Use a PassRefPtr<SkBitmap::Allocator> in setMemoryAllocator, instead
of a raw pointer.
- Make setMemoryAllocator no longer virtual, since it has no overrides.

ImageFrameGenerator:
- Use a PassRefPtr for methods taking an SkBitmap::Allocator, and
release the ExternalMemoryAllocator to them.

No change in behavior, no new tests
==========

[scroggo_chromium, 2016-04-14 20:33:46]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 20:34:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880993003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880993003/40001

[f(malita), 2016-04-14 20:37:06]

The change LGTM, but I find the SkBitmap::Allocator semantics and ownership
confusing :)

While I requested this in http://crrev.com/1812273003, I don't feel as strongly
now: I realized ImageFrame::setMemoryAllocator is not really ref-ing the
allocator, but saving a raw pointer temporarily (to be immediately cleared in
ImageFrameGenerator::decode).  Skia (SkBitmap::tryAlloc) doesn't ref the
allocator either - so I think the only reason for having setMemoryAllocator is
to temporarily plumb the allocator arg down to the lower layers.

If that matches your understanding, it should be OK to go back to your initial
form and use a stack allocated, err, allocator (plus document why it's safe to
do so).  I'll leave it up to you if you want to do that instead.


A couple of follow-up ideas:

1) Do we ever pass/share ownership of SkBitmap::Allocator in Skia's public APIs?
 If not, can it stop inheriting from SkRefCnt?  Part of my confusion is related
to the allocator being ref-counted - which seems to indicate sharing, but I
couldn't find evidence of that.

2) If the only reason for setMemoryAllocator is arg passing, maybe the allocator
should be plumbed explicitly; that's a lot of plumbing, but I think in the end
it'd be worth it (much easier to reason/follow method arguments than track
transient state bits, plus it may solve the multi-frame limitation).

[scroggo_chromium, 2016-04-14 20:59:32]

On 2016/04/14 20:37:06, f(malita) wrote:
> The change LGTM, but I find the SkBitmap::Allocator semantics and ownership
> confusing :)
> 
> While I requested this in http://crrev.com/1812273003, I don't feel as
strongly
> now: I realized ImageFrame::setMemoryAllocator is not really ref-ing the
> allocator, but saving a raw pointer temporarily (to be immediately cleared in
> ImageFrameGenerator::decode).  Skia (SkBitmap::tryAlloc) doesn't ref the
> allocator either - so I think the only reason for having setMemoryAllocator is
> to temporarily plumb the allocator arg down to the lower layers.
> 
> If that matches your understanding, it should be OK to go back to your initial
> form and use a stack allocated, err, allocator (plus document why it's safe to
> do so).  I'll leave it up to you if you want to do that instead.

Yes, that is my understanding. Should've fought harder for my original form :)

> 
> 
> A couple of follow-up ideas:
> 
> 1) Do we ever pass/share ownership of SkBitmap::Allocator in Skia's public
APIs?
>  If not, can it stop inheriting from SkRefCnt?  Part of my confusion is
related
> to the allocator being ref-counted - which seems to indicate sharing, but I
> couldn't find evidence of that.

Not that I have found. Generally it is used for one shot calls. We do have some
classes that own one, but they do not do any sharing. Android always puts theirs
on the stack.
(I think we used to more freely inherit from SkRefCnt. If we wrote the class
today, I don't think we would've written it that way.)

> 
> 2) If the only reason for setMemoryAllocator is arg passing, maybe the
allocator
> should be plumbed explicitly; that's a lot of plumbing, but I think in the end
> it'd be worth it (much easier to reason/follow method arguments than track
> transient state bits, plus it may solve the multi-frame limitation).

That sounds reasonable to me. I don't think it solves the multi-frame limitation
though. Here is my understanding of that limitation:
- We only use the allocator to make an ImageFrame use discardable memory.
- A multi-frame image may have one frame that depends on another.

Let's say that frame B depends on frame A. We've decoded frame A, but not B. A
is in discardable memory. We decide to decode frame B, but we've discarded frame
A! So we now need to decode frame A again.

Keeping frames out of discardable memory means that A will stick around long
enough for us to copy it into frame B. (Maybe, it's possible the whole
ImageDecoder will get thrown out of the ImageDecodingStore :P

[commit-bot: I haz the power, 2016-04-14 23:03:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 23:03:10]

Dry run: This issue passed the CQ dry run.