add SkPixmap and external locking to bitmaps

add SkPixmap and external locking to bitmaps

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/92fc2ae58331662ec411a048686cb4801e0a909a

[reed1, 2015-04-09 20:57:16]

reed@google.com changed reviewers:
+ robertphillips@google.com, scroggo@google.com

[scroggo, 2015-04-10 13:17:44]

On 2015/04/09 20:57:17, reed1 wrote:

Seems like the right track to me.

[enne (OOO), 2015-04-17 17:57:54]

I think if this only keeps the scaled version locked, then that'd be perfect for
a warmup API.

Do you see some way to say "use this lock request" for a given drawBitmap or
SkPaint with a bitmap?

[reed1, 2015-05-18 21:24:55]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-18 21:25:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074983003/100001

[reed1, 2015-05-18 21:25:37]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-05-18 21:25:47]

reed@google.com changed reviewers:
+ caryclark@google.com

[reed1, 2015-05-19 14:55:56]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-19 14:56:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074983003/120001

[commit-bot: I haz the power, 2015-05-19 15:02:12]

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

[commit-bot: I haz the power, 2015-05-19 15:02:13]

Dry run: This issue passed the CQ dry run.

[scroggo, 2015-05-19 20:11:42]

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkBitmap....
include/core/SkBitmap.h:432: class LockResult {
inherit from SkNoncopyable?

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkBitmap....
include/core/SkBitmap.h:441: void unlock();
Should this get called by a destructor, if it wasn't called before?

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
include/core/SkImageInfo.h:120: default: return 0;
Should this SkASSERT(false)?

https://codereview.chromium.org/1074983003/diff/120001/include/core/SkPixmap.h
File include/core/SkPixmap.h (right):

https://codereview.chromium.org/1074983003/diff/120001/include/core/SkPixmap....
include/core/SkPixmap.h:15: class SkPixmap {
Comments?

It looks like this is designed to be a short-lived object whose members (fPixels
and fCTable) are unowned.

Shouldn't this be SkNoncopyable, so no one tries to copy it and use it after the
associated pixel ref is unlocked?

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:433: static bool copy_pixels_to(const SkPixmap& pmap,
void* const dst, size_t dstSize,
Maybe rename pmap to something that specifies that it is the source?

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:757: return; // can't erase. Should we bzero so the memory
is not uninitialized?
Maybe SkASSERT(false)? Otherwise it seems like something incorrect is going to
happen later...

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:923: const SkImageInfo srcInfo =
pmap.info().makeWH(dstInfo.width(), dstInfo.height());
You could have used fInfo instead of pmap.info(). (It seems weird that we have
duplicate data here, but I don't think there's any way around it.)

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1126: SkAutoBitmapLockResult apl;
Auto Pixel (?) Lock?

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1408: SkPixelRef* pr = fPixelRef;
SkASSERT(result)

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1424: new (result) LockResult(SkPixmap(this->info(), addr,
res.fRowBytes, res.fCTable),
This seems a little sneaky. Would it make more sense to call a private
initialize function?

Also, should this use SkNEW_PLACEMENT_ARGS?

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap_scro...
File src/core/SkBitmap_scroll.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap_scro...
src/core/SkBitmap_scroll.cpp:55: bool SkBitmap::scrollRect(const SkIRect*
subset, int dx, int dy, SkRegion* inval) const {
It appears that neither Android nor Chromium use this function. Remove it?

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkPixelRef.cp...
src/core/SkPixelRef.cpp:155: bool SkPixelRef::lockPixelsInsideMutex(LockRec*
rec) {
fMutex->assertHeld();

[reed1, 2015-05-21 20:02:00]

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
include/core/SkImageInfo.h:120: default: return 0;
On 2015/05/19 20:11:42, scroggo wrote:
> Should this SkASSERT(false)?

kUnknown_SkColorType correctly should return 0

[reed1, 2015-05-21 20:02:39]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-21 20:03:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074983003/180001

[scroggo, 2015-05-21 20:20:55]

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
include/core/SkImageInfo.h:120: default: return 0;
On 2015/05/21 20:02:00, reed1 wrote:
> On 2015/05/19 20:11:42, scroggo wrote:
> > Should this SkASSERT(false)?
> 
> kUnknown_SkColorType correctly should return 0

Right, but it seems like that is correct but not interesting. If someone called
this with kUnknown_SkColorType, aren't they going to run into trouble later, if
they were going to do anything with the information?

[reed1, 2015-05-21 20:59:36]

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkBitmap....
include/core/SkBitmap.h:432: class LockResult {
On 2015/05/19 20:11:41, scroggo wrote:
> inherit from SkNoncopyable?

Done.

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkBitmap....
include/core/SkBitmap.h:441: void unlock();
On 2015/05/19 20:11:42, scroggo wrote:
> Should this get called by a destructor, if it wasn't called before?

Done.

https://codereview.chromium.org/1074983003/diff/120001/include/core/SkPixmap.h
File include/core/SkPixmap.h (right):

https://codereview.chromium.org/1074983003/diff/120001/include/core/SkPixmap....
include/core/SkPixmap.h:15: class SkPixmap {
On 2015/05/19 20:11:42, scroggo wrote:
> Comments?
> 
> It looks like this is designed to be a short-lived object whose members
(fPixels
> and fCTable) are unowned.
> 
> Shouldn't this be SkNoncopyable, so no one tries to copy it and use it after
the
> associated pixel ref is unlocked?

Done.

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:433: static bool copy_pixels_to(const SkPixmap& pmap,
void* const dst, size_t dstSize,
On 2015/05/19 20:11:42, scroggo wrote:
> Maybe rename pmap to something that specifies that it is the source?

Done.

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:757: return; // can't erase. Should we bzero so the memory
is not uninitialized?
On 2015/05/19 20:11:42, scroggo wrote:
> Maybe SkASSERT(false)? Otherwise it seems like something incorrect is going to
> happen later...

As the old code did this, I suggest I leave it as is but add a TODO (in case the
assert fires I don't want that to invalidate this CL).

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:923: const SkImageInfo srcInfo =
pmap.info().makeWH(dstInfo.width(), dstInfo.height());
On 2015/05/19 20:11:42, scroggo wrote:
> You could have used fInfo instead of pmap.info(). (It seems weird that we have
> duplicate data here, but I don't think there's any way around it.)

Acknowledged.

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1126: SkAutoBitmapLockResult apl;
On 2015/05/19 20:11:42, scroggo wrote:
> Auto Pixel (?) Lock?

SkAutoPixmapUnlock

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1408: SkPixelRef* pr = fPixelRef;
On 2015/05/19 20:11:42, scroggo wrote:
> SkASSERT(result)

Done.

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1424: new (result) LockResult(SkPixmap(this->info(), addr,
res.fRowBytes, res.fCTable),
On 2015/05/19 20:11:42, scroggo wrote:
> This seems a little sneaky. Would it make more sense to call a private
> initialize function?
> 
> Also, should this use SkNEW_PLACEMENT_ARGS?

Done.

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap_scro...
File src/core/SkBitmap_scroll.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap_scro...
src/core/SkBitmap_scroll.cpp:55: bool SkBitmap::scrollRect(const SkIRect*
subset, int dx, int dy, SkRegion* inval) const {
On 2015/05/19 20:11:42, scroggo wrote:
> It appears that neither Android nor Chromium use this function. Remove it?

I'll try that in an (earlier?) CL.

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkPixelRef.cp...
src/core/SkPixelRef.cpp:155: bool SkPixelRef::lockPixelsInsideMutex(LockRec*
rec) {
On 2015/05/19 20:11:42, scroggo wrote:
> fMutex->assertHeld();

Done.

[reed1, 2015-05-21 20:59:45]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-21 21:00:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074983003/200001

[reed1, 2015-05-21 21:19:02]

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/1074983003/diff/100001/include/core/SkImageIn...
include/core/SkImageInfo.h:120: default: return 0;
On 2015/05/21 20:20:55, scroggo wrote:
> On 2015/05/21 20:02:00, reed1 wrote:
> > On 2015/05/19 20:11:42, scroggo wrote:
> > > Should this SkASSERT(false)?
> > 
> > kUnknown_SkColorType correctly should return 0
> 
> Right, but it seems like that is correct but not interesting. If someone
called
> this with kUnknown_SkColorType, aren't they going to run into trouble later,
if
> they were going to do anything with the information? 

This is calling SkColorTypeBytesPerPixels, which already behaves this same-way :
it returns a 0 for these configs. If we want to make this sort of logical
change, I'd like to make it across the board (in this and the other method) to
keep them mutually consistent. If we choose to do that, I'd prefer to do it in a
separate CL.

[commit-bot: I haz the power, 2015-05-21 22:43:27]

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

[commit-bot: I haz the power, 2015-05-21 22:43:29]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-22 12:13:55]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-05-22 12:14:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074983003/220001

[commit-bot: I haz the power, 2015-05-22 12:14:09]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-05-22 18:14 UTC

[scroggo, 2015-05-22 15:04:14]

lgtm

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:757: return; // can't erase. Should we bzero so the memory
is not uninitialized?
On 2015/05/21 20:59:35, reed1 wrote:
> On 2015/05/19 20:11:42, scroggo wrote:
> > Maybe SkASSERT(false)? Otherwise it seems like something incorrect is going
to
> > happen later...
> 
> As the old code did this, I suggest I leave it as is but add a TODO (in case
the
> assert fires I don't want that to invalidate this CL).

sgtm

https://codereview.chromium.org/1074983003/diff/120001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1126: SkAutoBitmapLockResult apl;
On 2015/05/21 20:59:35, reed1 wrote:
> On 2015/05/19 20:11:42, scroggo wrote:
> > Auto Pixel (?) Lock?
> 
> SkAutoPixmapUnlock

Oh, haha, I was trying to understand the name "apl". I'm fine with either name
for the class.

https://codereview.chromium.org/1074983003/diff/220001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/1074983003/diff/220001/src/core/SkPixelRef.cp...
src/core/SkPixelRef.cpp:357: pr->unref();    // balancing the Ref in
onRequestLoc
onRequestLock*

[commit-bot: I haz the power, 2015-05-22 15:06:28]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/92fc2ae58331662ec411a048686cb4801e0a909a

[scroggo, 2015-05-22 18:42:33]

Late addition to the review:

https://codereview.chromium.org/1074983003/diff/220001/include/core/SkPixmap.h
File include/core/SkPixmap.h (right):

https://codereview.chromium.org/1074983003/diff/220001/include/core/SkPixmap....
include/core/SkPixmap.h:150: };
Should this be SK_REQUIRE_LOCAL_VAR?