change offset to xy for pixelref subsetting

change offset to xy for pixelref subsetting

BUG=
R=scroggo@google.com

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

[scroggo, 2014-01-07 22:11:33]

https://codereview.chromium.org/105893012/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/gm/gmmain.cpp#newcode1016
gm/gmmain.cpp:1016: src.serialize(&storage, NULL);
Is this just temporary? If so, can you add a comment?

This probably makes little difference, actually, since I think it will only
affect gm/factory.cpp (I think that's the only one to use an SkPixelRef that
overrides onRefEncodedData)

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

https://codereview.chromium.org/105893012/diff/30001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:356: //    size_t pixelRefOffset() const { return
fPixelRefOffset; }
Any reason not to delete this outright?

https://codereview.chromium.org/105893012/diff/30001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:683: SkIPoint    fPixelRefOrigin;
Why was the old one mutable, and this one not? (My guess is that it shouldn't be
mutable, but it'd be nice to know why it used to be.)

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

https://codereview.chromium.org/105893012/diff/30001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1513: buffer.writeInt(fPixelRefOrigin.fX);
Should we update the PICTURE_VERSION? I think we never go through this path in
Chrome, since we use an encoding function.

https://codereview.chromium.org/105893012/diff/30001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1550: size_t offset = origin.fY * rowBytes + origin.fX *
fBytesPerPixel;
Thought for the future: it might be nice to have a have a helper function for
this calculation.

https://codereview.chromium.org/105893012/diff/30001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1554: offset = 0;
Setting the offset to 0 won't do anything (since we never look at it again). Set
origin to 0?

https://codereview.chromium.org/105893012/diff/30001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:165: // 2. If there is a function for encoding
bitmaps, use it to write an encoded version of the
Can you update the comments to match the new behavior (but see below - I think
we should change this behavior slightly).

https://codereview.chromium.org/105893012/diff/30001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:190: SkAutoDataUnref
data(bitmap.pixelRef()->refEncodedData());
I thought we discussed calling fBitmapEncoder first, so that we would allow the
caller to ignore the encoded data if desired. That would allow retaining the
desired behavior in https://code.google.com/p/skia/issues/detail?id=1239

https://codereview.chromium.org/105893012/diff/30001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:373: return SkIRect::MakeXYWH(origin.fX,
origin.fY, bm.width(), bm.height());
Thought for the future: might it be useful to have a function on SkBitmap that
does this?

[reed1, 2014-01-07 22:37:59]

https://codereview.chromium.org/105893012/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/gm/gmmain.cpp#newcode1016
gm/gmmain.cpp:1016: src.serialize(&storage, NULL);
On 2014/01/07 22:11:33, scroggo wrote:
> Is this just temporary? If so, can you add a comment?
> 
> This probably makes little difference, actually, since I think it will only
> affect gm/factory.cpp (I think that's the only one to use an SkPixelRef that
> overrides onRefEncodedData)

Are you looking for the following:

// we don't want to encode images during serialzation, so we pass null.

I can, though not sure that we have such a comment when we want "default"
behavior in other places.

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

https://codereview.chromium.org/105893012/diff/30001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:356: //    size_t pixelRefOffset() const { return
fPixelRefOffset; }
On 2014/01/07 22:11:33, scroggo wrote:
> Any reason not to delete this outright?

Done.

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

https://codereview.chromium.org/105893012/diff/30001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1513: buffer.writeInt(fPixelRefOrigin.fX);
On 2014/01/07 22:11:33, scroggo wrote:
> Should we update the PICTURE_VERSION? I think we never go through this path in
> Chrome, since we use an encoding function.

Yea, seems like I should bump the version.

https://codereview.chromium.org/105893012/diff/30001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1550: size_t offset = origin.fY * rowBytes + origin.fX *
fBytesPerPixel;
On 2014/01/07 22:11:33, scroggo wrote:
> Thought for the future: it might be nice to have a have a helper function for
> this calculation.

Agreed. I will look for that.

https://codereview.chromium.org/105893012/diff/30001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1554: offset = 0;
On 2014/01/07 22:11:33, scroggo wrote:
> Setting the offset to 0 won't do anything (since we never look at it again).
Set
> origin to 0?

Done.

https://codereview.chromium.org/105893012/diff/30001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:190: SkAutoDataUnref
data(bitmap.pixelRef()->refEncodedData());
On 2014/01/07 22:11:33, scroggo wrote:
> I thought we discussed calling fBitmapEncoder first, so that we would allow
the
> caller to ignore the encoded data if desired. That would allow retaining the
> desired behavior in https://code.google.com/p/skia/issues/detail?id=1239

I propose keeping the order as is today, as that is compatible with the current
callers, and when we "fix" the signature (get rid of the size_t offset), we
change to an object/interface, which can allow more sophisticated clients (e.g.
decided to re-encoded based on context).

https://codereview.chromium.org/105893012/diff/30001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:373: return SkIRect::MakeXYWH(origin.fX,
origin.fY, bm.width(), bm.height());
On 2014/01/07 22:11:33, scroggo wrote:
> Thought for the future: might it be useful to have a function on SkBitmap that
> does this?

yea, I wonder about that too. getPixelRefSubset()?

[scroggo, 2014-01-08 13:31:39]

lgtm, with a change to fix dm/DMSerializeTask.cpp (calls pixelRefOffset()) and a
change to the PICTURE_VERSION.

https://codereview.chromium.org/105893012/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/gm/gmmain.cpp#newcode1016
gm/gmmain.cpp:1016: src.serialize(&storage, NULL);
On 2014/01/07 22:37:59, reed1 wrote:
> On 2014/01/07 22:11:33, scroggo wrote:
> > Is this just temporary? If so, can you add a comment?
> > 
> > This probably makes little difference, actually, since I think it will only
> > affect gm/factory.cpp (I think that's the only one to use an SkPixelRef that
> > overrides onRefEncodedData)
> 
> Are you looking for the following:
> 
> // we don't want to encode images during serialzation, so we pass null.
> 
> I can, though not sure that we have such a comment when we want "default"
> behavior in other places.

Oh, no, I didn't realize that was the desired behavior. I thought you were
planning to use a new function once we had a new signature. No comment is
necessary for the default behavior.

https://codereview.chromium.org/105893012/diff/30001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/105893012/diff/30001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:373: return SkIRect::MakeXYWH(origin.fX,
origin.fY, bm.width(), bm.height());
On 2014/01/07 22:37:59, reed1 wrote:
> On 2014/01/07 22:11:33, scroggo wrote:
> > Thought for the future: might it be useful to have a function on SkBitmap
that
> > does this?
> 
> yea, I wonder about that too. getPixelRefSubset()?

sgtm

[reed1, 2014-01-08 15:42:12]

Committed patchset #8 manually as r12958 (presubmit successful).