SkBitmapRegionCodec needs to use the rowBytes on the pixel ref

SkBitmapRegionCodec needs to use the rowBytes on the pixel ref

BUG=skia:4538
Committed: https://skia.googlesource.com/skia/+/fcff08c8301b1ca08eee20911d0df5fb9db41156

[msarett, 2015-11-04 22:43:05]

Patchset #1 (id:1) has been deleted

[msarett, 2015-11-05 20:02:30]

msarett@google.com changed reviewers:
+ scroggo@google.com

[msarett, 2015-11-05 20:02:30]

Forgot to send this.

Should make the change in Android make more sense.

[scroggo, 2015-11-05 20:20:48]

> SkBitmapRegionCodec needs to use the rowBytes on the pixel
> ref
>
> BUG=skia:

Maybe add 4538 to the description?

https://codereview.chromium.org/1422023006/diff/20001/tools/SkBitmapRegionCod...
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1422023006/diff/20001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:98: if (!bitmap->tryAllocPixels(allocator,
colorTable.get())) {
So if I understand correctly, the allocator is one in Android, which may be
using a different value for rowbytes than outInfo.minRowBytes? Maybe there
should also be a FIXME here that this method should be able to update the
rowbytes?

Come to think of it, we just used SkBitmap::Allocator as a convenience - we
could create our own class which handles this more cleanly...

No need to make that change now, but something to think about.

https://codereview.chromium.org/1422023006/diff/20001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:125: size_t rowBytes =
bitmap->pixelRef()->rowBytes();
Should we update the bitmap so that it matches?

That has a weird side effect - we can only update the bitmap's rowBytes by
calling setInfo, which unref's the pixel ref. In order to make this code robust,
we need to do the following:

  SkPixelRef* pr = SkRef(bitmap->pixelRef());
  size_t rowBytes = pr->rowBytes();
  bitmap->setInfo(outInfo, rowBytes);
  bitmap->setPixelRef(pr)->unref();

Probably makes sense to make that change (along with a FIXME).

[msarett, 2015-11-05 22:14:54]

Description was changed from

==========
SkBitmapRegionCodec needs to use the rowBytes on the pixel ref

BUG=skia:
==========

to

==========
SkBitmapRegionCodec needs to use the rowBytes on the pixel ref

BUG=skia:4538
==========

[msarett, 2015-11-05 22:34:59]

https://codereview.chromium.org/1422023006/diff/20001/tools/SkBitmapRegionCod...
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1422023006/diff/20001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:98: if (!bitmap->tryAllocPixels(allocator,
colorTable.get())) {
On 2015/11/05 20:20:47, scroggo wrote:
> So if I understand correctly, the allocator is one in Android, which may be
> using a different value for rowbytes than outInfo.minRowBytes? Maybe there
> should also be a FIXME here that this method should be able to update the
> rowbytes?
> 
> Come to think of it, we just used SkBitmap::Allocator as a convenience - we
> could create our own class which handles this more cleanly...
> 
> No need to make that change now, but something to think about.

I'm glad you pointed this out.  We should not be trying to set the rowBytes
here.  We should accept whatever the allocator (from Android) gives us - we
can't know what it is because a bitmap may be recycled.  I am removing rowBytes
from this call.

Also, I agree that we may want to think about creating our own class.

https://codereview.chromium.org/1422023006/diff/20001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:125: size_t rowBytes =
bitmap->pixelRef()->rowBytes();
On 2015/11/05 20:20:48, scroggo wrote:
> Should we update the bitmap so that it matches?
> 
> That has a weird side effect - we can only update the bitmap's rowBytes by
> calling setInfo, which unref's the pixel ref. In order to make this code
robust,
> we need to do the following:
> 
>   SkPixelRef* pr = SkRef(bitmap->pixelRef());
>   size_t rowBytes = pr->rowBytes();
>   bitmap->setInfo(outInfo, rowBytes);
>   bitmap->setPixelRef(pr)->unref();
> 
> Probably makes sense to make that change (along with a FIXME).

Done.

Yeah we are fine without this change, but I agree it's dangerous to leave the
rowBytes mismatched.

[scroggo, 2015-11-05 22:45:56]

lgtm

https://codereview.chromium.org/1422023006/diff/40001/tools/SkBitmapRegionCod...
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1422023006/diff/40001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:127: SkPixelRef* pr = SkRef(bitmap->pixelRef());
I just want to double check that the ref counting is correct here...

Say pixelRef has a refCount of RC. After this call, it is RC + 1

https://codereview.chromium.org/1422023006/diff/40001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:129: bitmap->setInfo(outInfo, rowBytes);
Behind our backs, this decrements the refCount, so it is back to RC

https://codereview.chromium.org/1422023006/diff/40001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:130: bitmap->setPixelRef(pr)->unref();
setPixelRef increments it again (RC + 1) and then we immediately unref it (RC).

lgtm

[msarett, 2015-11-05 22:49:05]

https://codereview.chromium.org/1422023006/diff/40001/tools/SkBitmapRegionCod...
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1422023006/diff/40001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:130: bitmap->setPixelRef(pr)->unref();
On 2015/11/05 22:45:56, scroggo wrote:
> setPixelRef increments it again (RC + 1) and then we immediately unref it
(RC).
> 
> lgtm

Looks correct to me as well.

[msarett, 2015-11-05 22:49:13]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-05 22:49:18]

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

[commit-bot: I haz the power, 2015-11-05 23:01:01]

Committed patchset #2 (id:40001) as
https://skia.googlesource.com/skia/+/fcff08c8301b1ca08eee20911d0df5fb9db41156