Expand pixelref to return SkImageInfo and rowbytes

PixelRef now returns (nearly) everything that is currently in SkBitmap. The goal is to refactor bitmap later to remove redundancy, and more interestingly, remove the chance for a disconnect between the actual (pixelref) rowbytes and config, and the one claimed by the bitmap.

R=mtklein@google.com, scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12537

[reed1, 2013-11-19 16:37:10]

not ready for primetime, but getting there.

[scroggo, 2013-11-19 18:17:08]

https://codereview.chromium.org/68973005/diff/30001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/68973005/diff/30001/include/core/SkImageInfo....
include/core/SkImageInfo.h:66: kARGB_4444_SkColorType,
Nooo!

https://codereview.chromium.org/68973005/diff/30001/include/core/SkImageInfo....
include/core/SkImageInfo.h:130: * @param length Amount of memory available in
the buffer
What happens if you pass a number that is not a multiple of 4?

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:24: SkMallocPixelRef(const SkImageInfo&, void*
addr, size_t rowBytes,
We'll need to make sure we update Android to call the new constructor.

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:25: SkColorTable* ctable, bool ownPixels);
This problem already existed, but it is strange to be able to pass NULL to addr
(so the SkMallocPixelRef does the allocation) and false to ownPixels (so it
doesn't delete it). Maybe that's a valid use case though? The caller wants
SkMallocPixelRef to perform the allocation, and then they want to hang on to it
via getAddr?

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:30: return fInfo.fHeight * fRB;
Instead of (fInfo.fHeight - 1) * fRB + fInfo.fWidth * fInfo.bytesPerPixel()?

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:47: size_t          fRB;
Can these be const?

https://codereview.chromium.org/68973005/diff/30001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/30001/include/core/SkPixelRef.h...
include/core/SkPixelRef.h:240: virtual void* onLockPixels(SkImageInfo*, size_t*
rowBytes, SkColorTable**) = 0;
Comments explaining the parameters would be nice.

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

https://codereview.chromium.org/68973005/diff/30001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:460: this->setPixelRef(new SkMallocPixelRef(info, p,
fRowBytes, ctable, false))->unref();
SkNEW_ARGS

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
src/core/SkMallocPixelRef.cpp:51: buffer.write32(fInfo.fWidth);
SkImageInfo::writeToMemory?

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
src/core/SkMallocPixelRef.cpp:52: buffer.write32(fInfo.fHeight);
This will require a change to the picture format.

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
src/core/SkMallocPixelRef.cpp:69: fInfo.fWidth = buffer.read32();
readFromMemory?

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

https://codereview.chromium.org/68973005/diff/30001/src/core/SkPixelRef.cpp#n...
src/core/SkPixelRef.cpp:163: SkImageInfo info;
Shouldn't lockPixels take an SkImageInfo* and a size_t* so we can get this
information? If that is left to a future CL, can you add a TODO?

https://codereview.chromium.org/68973005/diff/30001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:524: if (NULL == fCache32PixelRef) {
If fCache32PixelRef exists, should we get info from it?

https://codereview.chromium.org/68973005/diff/30001/src/gpu/SkGrPixelRef.cpp
File src/gpu/SkGrPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/gpu/SkGrPixelRef.cpp#...
src/gpu/SkGrPixelRef.cpp:39: *ctable = NULL;
Why did you remove the check for NULL here, but preserve it for SkImageRef?

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef.cpp
File src/image/SkDataPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef....
src/image/SkDataPixelRef.cpp:38: void
SkDataPixelRef::flatten(SkFlattenableWriteBuffer& buffer) const {
We need to flatten (and unflatten) fInfo.

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef.h
File src/image/SkDataPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef....
src/image/SkDataPixelRef.h:31: SkImageInfo fInfo;
Can these be const?

https://codereview.chromium.org/68973005/diff/30001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/lazy/SkLazyPixelRef.c...
src/lazy/SkLazyPixelRef.cpp:151: return pixels;
I thought we had merged these two return cases (since the comments and return,
and now rowbytes, are the same)?

[reed1, 2013-11-20 20:21:30]

https://codereview.chromium.org/68973005/diff/30001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/68973005/diff/30001/include/core/SkImageInfo....
include/core/SkImageInfo.h:130: * @param length Amount of memory available in
the buffer
On 2013/11/19 18:17:09, scroggo wrote:
> What happens if you pass a number that is not a multiple of 4?

I have removed these methods.

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:24: SkMallocPixelRef(const SkImageInfo&, void*
addr, size_t rowBytes,
On 2013/11/19 18:17:09, scroggo wrote:
> We'll need to make sure we update Android to call the new constructor.

The compiler will help :)

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:25: SkColorTable* ctable, bool ownPixels);
On 2013/11/19 18:17:09, scroggo wrote:
> This problem already existed, but it is strange to be able to pass NULL to
addr
> (so the SkMallocPixelRef does the allocation) and false to ownPixels (so it
> doesn't delete it). Maybe that's a valid use case though? The caller wants
> SkMallocPixelRef to perform the allocation, and then they want to hang on to
it
> via getAddr?

I have an assert in the impl, requiring false for that case. I think now we
should perhaps offer two constructors, one explicitly for allocating storage,
and one explicitly for being passed storage. Or some other way to clarify this.

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:30: return fInfo.fHeight * fRB;
On 2013/11/19 18:17:09, scroggo wrote:
> Instead of (fInfo.fHeight - 1) * fRB + fInfo.fWidth * fInfo.bytesPerPixel()?

Good comment. Bitmap has both sizes. Not sure if we need to have both here or
not. I'll survey the callers for this call and see.

https://codereview.chromium.org/68973005/diff/30001/include/core/SkMallocPixe...
include/core/SkMallocPixelRef.h:47: size_t          fRB;
On 2013/11/19 18:17:09, scroggo wrote:
> Can these be const?

Good idea.

[reed1, 2013-11-20 20:56:56]

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

https://codereview.chromium.org/68973005/diff/30001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:460: this->setPixelRef(new SkMallocPixelRef(info, p,
fRowBytes, ctable, false))->unref();
On 2013/11/19 18:17:09, scroggo wrote:
> SkNEW_ARGS

Done.

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
src/core/SkMallocPixelRef.cpp:51: buffer.write32(fInfo.fWidth);
On 2013/11/19 18:17:09, scroggo wrote:
> SkImageInfo::writeToMemory?

removed (for now at least. only one caller)

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
src/core/SkMallocPixelRef.cpp:52: buffer.write32(fInfo.fHeight);
On 2013/11/19 18:17:09, scroggo wrote:
> This will require a change to the picture format.

indeed. I'll update the picvers code next.

https://codereview.chromium.org/68973005/diff/30001/src/core/SkMallocPixelRef...
src/core/SkMallocPixelRef.cpp:69: fInfo.fWidth = buffer.read32();
On 2013/11/19 18:17:09, scroggo wrote:
> readFromMemory?

removed api

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

https://codereview.chromium.org/68973005/diff/30001/src/core/SkPixelRef.cpp#n...
src/core/SkPixelRef.cpp:163: SkImageInfo info;
On 2013/11/19 18:17:09, scroggo wrote:
> Shouldn't lockPixels take an SkImageInfo* and a size_t* so we can get this
> information? If that is left to a future CL, can you add a TODO?

good idea

https://codereview.chromium.org/68973005/diff/30001/src/effects/gradients/SkG...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/effects/gradients/SkG...
src/effects/gradients/SkGradientShader.cpp:524: if (NULL == fCache32PixelRef) {
On 2013/11/19 18:17:09, scroggo wrote:
> If fCache32PixelRef exists, should we get info from it?

not sure it matters, since it is always the same.

https://codereview.chromium.org/68973005/diff/30001/src/gpu/SkGrPixelRef.cpp
File src/gpu/SkGrPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/gpu/SkGrPixelRef.cpp#...
src/gpu/SkGrPixelRef.cpp:39: *ctable = NULL;
On 2013/11/19 18:17:09, scroggo wrote:
> Why did you remove the check for NULL here, but preserve it for SkImageRef?

no good reason. I'll remove it from imageref too.

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef.cpp
File src/image/SkDataPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef....
src/image/SkDataPixelRef.cpp:38: void
SkDataPixelRef::flatten(SkFlattenableWriteBuffer& buffer) const {
On 2013/11/19 18:17:09, scroggo wrote:
> We need to flatten (and unflatten) fInfo.

doh!

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef.h
File src/image/SkDataPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/30001/src/image/SkDataPixelRef....
src/image/SkDataPixelRef.h:31: SkImageInfo fInfo;
On 2013/11/19 18:17:09, scroggo wrote:
> Can these be const?

nope, since they get set late in the unflattening constructor.

https://codereview.chromium.org/68973005/diff/30001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/30001/src/lazy/SkLazyPixelRef.c...
src/lazy/SkLazyPixelRef.cpp:151: return pixels;
On 2013/11/19 18:17:09, scroggo wrote:
> I thought we had merged these two return cases (since the comments and return,
> and now rowbytes, are the same)?

sgtm

[reed1, 2013-11-20 21:01:53]

I think I addressed leon's initial comments

[scroggo, 2013-11-20 21:04:27]

Do the A1 changes need to be included here?

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:24: *  specified colortable (if not NULL).
Can you add a comment that this does not take ownership of (i.e. delete) the
pixels?

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:28: static SkMallocPixelRef* Create(const
SkImageInfo&, void* addr,
Maybe a better name for this would be Wrap or WrapPixels?

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

https://codereview.chromium.org/68973005/diff/150001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1073: case kA1_Config:
This change seems to be unrelated.

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:82: {
Brace should be on previous line.

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:140: {
brace should be on previous line.

https://codereview.chromium.org/68973005/diff/150001/src/effects/gradients/Sk...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/src/effects/gradients/Sk...
src/effects/gradients/SkGradientShader.cpp:516: SkImageInfo info;
FIXME that info can be retrieved from fCache32PixelRef if non NULL?

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:229: { SkBitmap::kA1_Config,         "00000000"  },
Seems like part of a separate change.

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapGetColorTest...
File tests/BitmapGetColorTest.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapGetColorTest...
tests/BitmapGetColorTest.cpp:30: return;
This doesn't hit any of our warnings?

[reed1, 2013-11-20 21:27:17]

removing A1 support is (I think) a prerequisite for this CL to land.

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:24: *  specified colortable (if not NULL).
On 2013/11/20 21:04:28, scroggo wrote:
> Can you add a comment that this does not take ownership of (i.e. delete) the
> pixels?

"The caller is responsible for managing the  lifetime of the pixel storage
buffer."

This was not clear?

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:28: static SkMallocPixelRef* Create(const
SkImageInfo&, void* addr,
On 2013/11/20 21:04:28, scroggo wrote:
> Maybe a better name for this would be Wrap or WrapPixels?

Hmmm

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

https://codereview.chromium.org/68973005/diff/150001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1073: case kA1_Config:
On 2013/11/20 21:04:28, scroggo wrote:
> This change seems to be unrelated.

It was needed to keep unittests passing, given that with this CL, A1 is no
longer supported.

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:82: {
On 2013/11/20 21:04:28, scroggo wrote:
> Brace should be on previous line.

I am increasingly finding that much harder to read than this style.

    : initializer(value {
    first line of code

I would like to experiment with this variant.

https://codereview.chromium.org/68973005/diff/150001/src/effects/gradients/Sk...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/src/effects/gradients/Sk...
src/effects/gradients/SkGradientShader.cpp:516: SkImageInfo info;
On 2013/11/20 21:04:28, scroggo wrote:
> FIXME that info can be retrieved from fCache32PixelRef if non NULL?

? I'm not sure why that would be better?

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:229: { SkBitmap::kA1_Config,         "00000000"  },
On 2013/11/20 21:04:28, scroggo wrote:
> Seems like part of a separate change.

Needed since w/ this change, A1 is no longer supported.

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapGetColorTest...
File tests/BitmapGetColorTest.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapGetColorTest...
tests/BitmapGetColorTest.cpp:30: return;
On 2013/11/20 21:04:28, scroggo wrote:
> This doesn't hit any of our warnings?

A1 no longer supported

[scroggo, 2013-11-20 21:35:18]

lgtm

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/150001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:24: *  specified colortable (if not NULL).
On 2013/11/20 21:27:18, reed1 wrote:
> On 2013/11/20 21:04:28, scroggo wrote:
> > Can you add a comment that this does not take ownership of (i.e. delete) the
> > pixels?
> 
> "The caller is responsible for managing the  lifetime of the pixel storage
> buffer."
> 
> This was not clear?

D'oh! No, that's clear.

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

https://codereview.chromium.org/68973005/diff/150001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1073: case kA1_Config:
On 2013/11/20 21:27:18, reed1 wrote:
> On 2013/11/20 21:04:28, scroggo wrote:
> > This change seems to be unrelated.
> 
> It was needed to keep unittests passing, given that with this CL, A1 is no
> longer supported.

sgtm. Can you add the deprecation of A1 to the change description?

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:82: {
On 2013/11/20 21:27:18, reed1 wrote:
> On 2013/11/20 21:04:28, scroggo wrote:
> > Brace should be on previous line.
> 
> I am increasingly finding that much harder to read than this style.
> 
>     : initializer(value {
>     first line of code
> 
> I would like to experiment with this variant.

Agreed! I think we should update the style guide immediately.

https://codereview.chromium.org/68973005/diff/150001/src/effects/gradients/Sk...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/src/effects/gradients/Sk...
src/effects/gradients/SkGradientShader.cpp:516: SkImageInfo info;
On 2013/11/20 21:27:18, reed1 wrote:
> On 2013/11/20 21:04:28, scroggo wrote:
> > FIXME that info can be retrieved from fCache32PixelRef if non NULL?
> 
> ? I'm not sure why that would be better?

I guess not...

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapGetColorTest...
File tests/BitmapGetColorTest.cpp (right):

https://codereview.chromium.org/68973005/diff/150001/tests/BitmapGetColorTest...
tests/BitmapGetColorTest.cpp:30: return;
On 2013/11/20 21:27:18, reed1 wrote:
> On 2013/11/20 21:04:28, scroggo wrote:
> > This doesn't hit any of our warnings?
> 
> A1 no longer supported

Can we go ahead and remove this whole function?

I was thinking that once this gets submitted it might give us a warning on the
bots for unused code.

[scroggo, 2013-11-20 21:44:15]

https://codereview.chromium.org/68973005/diff/380001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/380001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:30: static SkMallocPixelRef* NewDirect(const
SkImageInfo&, void* addr,
Nit on the names:

Direct is an adjective, whereas Allocate is a verb.

I'm also not sure what "direct" means in this case. I like NewWrap because while
the other version allocates its pixels, this one wraps existing pixels.

[reed1, 2013-12-04 15:23:25]

Patch #7

Renamed onLockPixels to onNewLockPixels, so we can stage the change in Chrome,
which has existing subclasses of pixelref. There is a plan to obsolete those.
If/when that happens, and all subclasses are just in skia, we can rename it back
to onLockPixels.

[reed1, 2013-12-04 16:41:24]

Reviewers, please look at least at the new signature in SkPixelRef.h for
onNewLockPixels and onGetInfo

[scroggo, 2013-12-04 18:14:02]

lgtm at patchset 8, with some nits.

https://codereview.chromium.org/68973005/diff/560001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/68973005/diff/560001/include/core/SkImageInfo...
include/core/SkImageInfo.h:132: void write(SkFlattenableWriteBuffer&) const;
In SkBitmap we call these flatten() and unflatten().

In SkFlattenables we call this function flatten(), and use a constructor to
unflatten.

In SkColorTable and SkAnnotation we call this writeToBuffer(), and also use a
constructor for unflattening.

I'd suggest we use flatten() and unflatten() (or use a constructor) to be
consistent with most callers.

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

https://codereview.chromium.org/68973005/diff/560001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:78: static void mark_invalid(SkImageInfo* info) {
Any reason to put this here instead of on SkImageInfo?

https://codereview.chromium.org/68973005/diff/560001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:82: static bool is_invalid(const SkImageInfo& info) {
What's the reason for calling this instead of !SkImageInfo::isValid()?

[hal.canary, 2013-12-04 19:07:09]

https://codereview.chromium.org/68973005/diff/560001/src/core/SkMallocPixelRe...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/560001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:108: bool SkMallocPixelRef::onGetInfo(SkImageInfo*
info) {
Is this ever called if these PixelRefs are always pre-locked?

[reed1, 2013-12-04 19:50:58]

On 2013/12/04 19:07:09, Hal Canary wrote:
>
https://codereview.chromium.org/68973005/diff/560001/src/core/SkMallocPixelRe...
> File src/core/SkMallocPixelRef.cpp (right):
> 
>
https://codereview.chromium.org/68973005/diff/560001/src/core/SkMallocPixelRe...
> src/core/SkMallocPixelRef.cpp:108: bool
SkMallocPixelRef::onGetInfo(SkImageInfo*
> info) {
> Is this ever called if these PixelRefs are always pre-locked?

I don't think it is, but that is just the current optimization. I think they
need to implement it regardless.

[reed1, 2013-12-04 21:12:53]

https://codereview.chromium.org/68973005/diff/560001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/68973005/diff/560001/include/core/SkImageInfo...
include/core/SkImageInfo.h:132: void write(SkFlattenableWriteBuffer&) const;
On 2013/12/04 18:14:03, scroggo wrote:
> In SkBitmap we call these flatten() and unflatten().
> 
> In SkFlattenables we call this function flatten(), and use a constructor to
> unflatten.
> 
> In SkColorTable and SkAnnotation we call this writeToBuffer(), and also use a
> constructor for unflattening.
> 
> I'd suggest we use flatten() and unflatten() (or use a constructor) to be
> consistent with most callers.

Done.

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

https://codereview.chromium.org/68973005/diff/560001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:78: static void mark_invalid(SkImageInfo* info) {
On 2013/12/04 18:14:03, scroggo wrote:
> Any reason to put this here instead of on SkImageInfo?

ImageInfo's valid is possibly what I should use, but it was originally written
just for the valid-deserialization-security code. Perhaps it should be made
private to that logic for now, since it doesn't really check for all possible
"logical" invalid combinations, just checking for out-of-range values.

I'm just doing it here to avoid having a bool field, since I am "caching" the
ImageInfo.

https://codereview.chromium.org/68973005/diff/560001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:82: static bool is_invalid(const SkImageInfo& info) {
On 2013/12/04 18:14:03, scroggo wrote:
> What's the reason for calling this instead of !SkImageInfo::isValid()?

see above

[mtklein, 2013-12-04 22:45:59]

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPicture.h...
include/core/SkPicture.h:227: static const uint32_t PICTURE_VERSION = 17;
You may want to check in with Robert.  I think you might be able to call this
16.  If I understood correctly, we're sort of straddling 15 and 16 right now
with a backward compatibility bit, and Robert was going to make some bigger
changes before settling on a non-forward-compatible change from 15 to 16.

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef....
include/core/SkPixelRef.h:82: /** Call to access the pixel memory, which is
returned. Balance with a call
Just happened to notice that this comment is _somewhat_ misleading.

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef....
include/core/SkPixelRef.h:112: bool getInfo(SkImageInfo*);
Don't forget how you, Hal, and I decided we should probably do this all up front
and make this const SkImageInfo& getInfo() const;

https://codereview.chromium.org/68973005/diff/600001/src/core/SkMallocPixelRe...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/600001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:35: SkMallocPixelRef*
SkMallocPixelRef::NewDirect(const SkImageInfo& info,
I would assert that this may be better thought of as a different type than
SkMallocPixelRef.  There's not really any Mallociness to it.

[scroggo, 2013-12-04 22:47:37]

Still lgtm

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef....
include/core/SkPixelRef.h:58: struct LockRec {
Maybe add a comment that these are unowned pointers that are only guaranteed to
be valid while the pixel ref is locked (except for fRowBytes)?

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

https://codereview.chromium.org/68973005/diff/600001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:169: SkASSERT(!is_invalid(fInfo));
This seems redundant.

https://codereview.chromium.org/68973005/diff/600001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:178: SkImageInfo tmpInfo;
This one is just in case the implementation of onGetInfo does not honor the
contract and modifies the passed in info? I think that will still only be a
problem if it sets it to something valid and returns false.

[reed1, 2013-12-05 13:32:38]

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef....
include/core/SkPixelRef.h:82: /** Call to access the pixel memory, which is
returned. Balance with a call
On 2013/12/04 22:46:00, mtklein wrote:
> Just happened to notice that this comment is _somewhat_ misleading.

Done.

https://codereview.chromium.org/68973005/diff/600001/include/core/SkPixelRef....
include/core/SkPixelRef.h:112: bool getInfo(SkImageInfo*);
On 2013/12/04 22:46:00, mtklein wrote:
> Don't forget how you, Hal, and I decided we should probably do this all up
front
> and make this const SkImageInfo& getInfo() const; 

Working on it

https://codereview.chromium.org/68973005/diff/600001/src/core/SkMallocPixelRe...
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/600001/src/core/SkMallocPixelRe...
src/core/SkMallocPixelRef.cpp:35: SkMallocPixelRef*
SkMallocPixelRef::NewDirect(const SkImageInfo& info,
On 2013/12/04 22:46:00, mtklein wrote:
> I would assert that this may be better thought of as a different type than
> SkMallocPixelRef.  There's not really any Mallociness to it.

Agreed. May move this kind of factory out to SkPixelRef

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

https://codereview.chromium.org/68973005/diff/600001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:169: SkASSERT(!is_invalid(fInfo));
On 2013/12/04 22:47:38, scroggo wrote:
> This seems redundant.

I don't think we assert elsewhere that prelocked means the info is valid.

https://codereview.chromium.org/68973005/diff/600001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:178: SkImageInfo tmpInfo;
On 2013/12/04 22:47:38, scroggo wrote:
> This one is just in case the implementation of onGetInfo does not honor the
> contract and modifies the passed in info? I think that will still only be a
> problem if it sets it to something valid and returns false.

Exactly.

Also, we are experimenting with making this yet-more-thread-safe, and I will
need to separate out calling onGetInfo from writing the answer into fInfo, for
memory barrier persnicketiness

[scroggo, 2013-12-05 13:45:34]

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

https://codereview.chromium.org/68973005/diff/600001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:169: SkASSERT(!is_invalid(fInfo));
On 2013/12/05 13:32:39, reed1 wrote:
> On 2013/12/04 22:47:38, scroggo wrote:
> > This seems redundant.
> 
> I don't think we assert elsewhere that prelocked means the info is valid.

Not directly, but just below this we assert that invalid means the pixel ref is
not prelocked. In what situation would we hit this assert, but would not have
hit the assert below?

[reed1, 2013-12-05 16:44:56]

#13 -- very good idea (requiring SkImageInfo) -- lots to review :)

[mtklein, 2013-12-05 17:43:03]

lgtm

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

https://codereview.chromium.org/68973005/diff/660001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:240: static bool Config2ColorType(Config, SkColorType*);
Seems like this guy can be private or static in the cpp?

https://codereview.chromium.org/68973005/diff/660001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/660001/include/core/SkPixelRef....
include/core/SkPixelRef.h:55: explicit SkPixelRef(const SkImageInfo&,
SkBaseMutex* mutex = NULL);
You may want to take a pass over these constructors and update the explicits and
@params where documented?

[reed1, 2013-12-05 18:44:13]

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

https://codereview.chromium.org/68973005/diff/660001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:240: static bool Config2ColorType(Config, SkColorType*);
On 2013/12/05 17:43:04, mtklein wrote:
> Seems like this guy can be private or static in the cpp?

It is called by other sections of our code, not just SkBitmap.

https://codereview.chromium.org/68973005/diff/660001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/660001/include/core/SkPixelRef....
include/core/SkPixelRef.h:55: explicit SkPixelRef(const SkImageInfo&,
SkBaseMutex* mutex = NULL);
On 2013/12/05 17:43:04, mtklein wrote:
> You may want to take a pass over these constructors and update the explicits
and
> @params where documented?

Doh. Will do.

[reed1, 2013-12-05 22:13:41]

ptal -- known work-around that I will address -- SkImageRef needs to update its
constructor arguments to include SkImageInfo

[mtklein, 2013-12-06 14:44:34]

still lgtm

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

https://codereview.chromium.org/68973005/diff/720001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:540: ctable);
ctable looks so lonely on this line

[scroggo, 2013-12-06 14:50:32]

https://codereview.chromium.org/68973005/diff/720001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkImageInfo...
include/core/SkImageInfo.h:139: fWidth * SkColorTypeBytesPerPixel(fColorType);
Should this call bytesPerPixel?

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:54: virtual size_t getAllocatedSizeInBytes()
const SK_OVERRIDE;
I don't like that this returns something that may or may not be the same as what
was actually allocated. For NewDirect, we don't actually know what the allocated
size is (or maybe it's zero, since we allocated nothing?) but for NewAllocate
this returns a totally different number.

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:67: size_t computeMinSize() const {
Is this function needed?

https://codereview.chromium.org/68973005/diff/720001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkPixelRef....
include/core/SkPixelRef.h:306: *  SkBitmap::fPixelRefOffset +
SkBitmap::getSafeSize() should never overflow this value,
Would it make sense to enforce this? Instead of making this virtual, have the
following function:

size_t getAllocatedSizeInBytes() const {
  const size_t bytes = this->onGetAllocatedSizeInBytes(); // virtual
  SkASSERT(bytes >= SkBitmap::fPixelRefOffset + SkBitmap::getSafeSize());
  return bytes;
}

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

https://codereview.chromium.org/68973005/diff/720001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:364: static bool config2ColorType(SkBitmap::Config config,
SkColorType* ctOut) {
style nit: underscores for static functions.

https://codereview.chromium.org/68973005/diff/720001/src/images/SkImageRef.cpp
File src/images/SkImageRef.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/src/images/SkImageRef.cp...
src/images/SkImageRef.cpp:28: : SkPixelRef(dummy_info(), mutex),
fErrorInDecoding(false) {
Could you mark with a fixme or todo?

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkBitmapFactory...
File src/lazy/SkBitmapFactory.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkBitmapFactory...
src/lazy/SkBitmapFactory.cpp:16:
SkBitmapFactory::SkBitmapFactory(SkBitmapFactory::DecodeProc proc)
Hal deleted this file, correct?

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:16:
SkLazyCachingPixelRef::SkLazyCachingPixelRef(const SkImageInfo& info,
Deleted by Hal?

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:21: class SkLazyCachingPixelRef : public
SkCachingPixelRef {
I think Hal deleted this file?

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyPixelRef....
src/lazy/SkLazyPixelRef.cpp:23: SkLazyPixelRef::SkLazyPixelRef(const
SkImageInfo& info, SkData* data,
Deleted by Hal.

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyPixelRef.h
File src/lazy/SkLazyPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyPixelRef....
src/lazy/SkLazyPixelRef.h:31: class SkLazyPixelRef : public SkPixelRef {
This file was deleted by Hal.

https://codereview.chromium.org/68973005/diff/720001/tests/DrawBitmapRectTest...
File tests/DrawBitmapRectTest.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/tests/DrawBitmapRectTest...
tests/DrawBitmapRectTest.cpp:47: SkImageInfo info;
I don't see where this is used.

https://codereview.chromium.org/68973005/diff/720001/tests/SerializationTest.cpp
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/tests/SerializationTest....
tests/SerializationTest.cpp:321: info.fWidth = 32;
Is the hack that the SkMallocPixelRef's SkImageInfo disagrees with
invalidBitmap2's? Should we make that test for that in setPixelRef? We'd have to
make sure we allow a smaller width and/or height on the bitmap in case it
represents a subset.

[reed1, 2013-12-06 15:48:45]

https://codereview.chromium.org/68973005/diff/720001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkImageInfo...
include/core/SkImageInfo.h:139: fWidth * SkColorTypeBytesPerPixel(fColorType);
On 2013/12/06 14:50:33, scroggo wrote:
> Should this call bytesPerPixel?

Doh! Done.

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:54: virtual size_t getAllocatedSizeInBytes()
const SK_OVERRIDE;
On 2013/12/06 14:50:33, scroggo wrote:
> I don't like that this returns something that may or may not be the same as
what
> was actually allocated. For NewDirect, we don't actually know what the
allocated
> size is (or maybe it's zero, since we allocated nothing?) but for NewAllocate
> this returns a totally different number.

I agree. This virtual was not added as part of this CL. I think it should be
renamed to something about safeSize. Something to do outside of this CL I think.

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:67: size_t computeMinSize() const {
On 2013/12/06 14:50:33, scroggo wrote:
> Is this function needed?

It is called twice, but I may be able to call the new method on SkImageInfo
instead.

https://codereview.chromium.org/68973005/diff/720001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkPixelRef....
include/core/SkPixelRef.h:306: *  SkBitmap::fPixelRefOffset +
SkBitmap::getSafeSize() should never overflow this value,
On 2013/12/06 14:50:33, scroggo wrote:
> Would it make sense to enforce this? Instead of making this virtual, have the
> following function:
> 
> size_t getAllocatedSizeInBytes() const {
>   const size_t bytes = this->onGetAllocatedSizeInBytes(); // virtual
>   SkASSERT(bytes >= SkBitmap::fPixelRefOffset + SkBitmap::getSafeSize());
>   return bytes;
> }

Good idea. Next CL.

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

https://codereview.chromium.org/68973005/diff/720001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:364: static bool config2ColorType(SkBitmap::Config config,
SkColorType* ctOut) {
On 2013/12/06 14:50:33, scroggo wrote:
> style nit: underscores for static functions.

Done.

https://codereview.chromium.org/68973005/diff/720001/src/images/SkImageRef.cpp
File src/images/SkImageRef.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/src/images/SkImageRef.cp...
src/images/SkImageRef.cpp:28: : SkPixelRef(dummy_info(), mutex),
fErrorInDecoding(false) {
On 2013/12/06 14:50:33, scroggo wrote:
> Could you mark with a fixme or todo?

fixed

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkBitmapFactory...
File src/lazy/SkBitmapFactory.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkBitmapFactory...
src/lazy/SkBitmapFactory.cpp:16:
SkBitmapFactory::SkBitmapFactory(SkBitmapFactory::DecodeProc proc)
On 2013/12/06 14:50:33, scroggo wrote:
> Hal deleted this file, correct? 

Done.

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:21: class SkLazyCachingPixelRef : public
SkCachingPixelRef {
On 2013/12/06 14:50:33, scroggo wrote:
> I think Hal deleted this file?

Done.

https://codereview.chromium.org/68973005/diff/720001/tests/DrawBitmapRectTest...
File tests/DrawBitmapRectTest.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/tests/DrawBitmapRectTest...
tests/DrawBitmapRectTest.cpp:47: SkImageInfo info;
On 2013/12/06 14:50:33, scroggo wrote:
> I don't see where this is used.

bottom of the file

https://codereview.chromium.org/68973005/diff/720001/tests/SerializationTest.cpp
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/68973005/diff/720001/tests/SerializationTest....
tests/SerializationTest.cpp:321: info.fWidth = 32;
On 2013/12/06 14:50:33, scroggo wrote:
> Is the hack that the SkMallocPixelRef's SkImageInfo disagrees with
> invalidBitmap2's? Should we make that test for that in setPixelRef? We'd have
to
> make sure we allow a smaller width and/or height on the bitmap in case it
> represents a subset.

The hack is that the bitmap is LARGER than the pixelref. I will make that
clearer in the code.

[scroggo, 2013-12-06 15:55:19]

lgtm at patch set 17

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/68973005/diff/720001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:54: virtual size_t getAllocatedSizeInBytes()
const SK_OVERRIDE;
On 2013/12/06 15:48:46, reed1 wrote:
> On 2013/12/06 14:50:33, scroggo wrote:
> > I don't like that this returns something that may or may not be the same as
> what
> > was actually allocated. For NewDirect, we don't actually know what the
> allocated
> > size is (or maybe it's zero, since we allocated nothing?) but for
NewAllocate
> > this returns a totally different number.
> 
> I agree. This virtual was not added as part of this CL. I think it should be
> renamed to something about safeSize. Something to do outside of this CL I
think.

Haha - the danger of looking at the changes since the last patch is that I think
you added it.

[reed1, 2013-12-06 18:41:52]

Committed patchset #20 manually as r12537 (presubmit successful).