add ways to peer into an image to get its pixels

add ways to peer into an image to get its pixels

BUG=skia:
R=bsalomon@google.com, scroggo@google.com

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

[reed1, 2014-02-05 17:06:48]

Consider image backends: cpu, gpu, encoded-bitmap, picture, (other?)

[reed1, 2014-02-05 17:08:02]

Related: is there real value in offering...

SkImage* SkImage::newSubset(const SkIRect& subset) const;

Where the new (subset) image may or may not be sharing explicit backend...

[bsalomon, 2014-02-05 17:13:36]

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h#newco...
include/core/SkImage.h:87: bool peekPixels(SkBitmap* bitmap) const;
Maybe this should return a const pointer, an image info, and a rowbytes? It
seems like SkBitmap's API is a mismatch for what the client is supposed to do
with the result.

[scroggo, 2014-02-05 17:16:06]

On 2014/02/05 17:08:02, reed1 wrote:
> Related: is there real value in offering...
> 
> SkImage* SkImage::newSubset(const SkIRect& subset) const;
> 
> Where the new (subset) image may or may not be sharing explicit backend...

I think it's probably just as valuable as SkBitmap::extractSubset

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h#newco...
include/core/SkImage.h:85: *  bitmap will unmodified.
nit: will be*

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h#newco...
include/core/SkImage.h:96: *  the bitmap's ColorType/AlphaType. Aany pixels in
the bitmap that do not
Any*

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h#newco...
include/core/SkImage.h:106: bool readPixels(SkBitmap* bitmap, const SkIRect*
subset = NULL);
Should this function be const also?

In the case of the GPU, where it *can* support these but it's slow (IIUC), will
it just return false? (Or, if I'm reading the comments correctly, it will return
false for peekPixels but true for readPixels?)

[scroggo, 2014-02-05 17:16:43]

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h#newco...
include/core/SkImage.h:87: bool peekPixels(SkBitmap* bitmap) const;
On 2014/02/05 17:13:37, bsalomon wrote:
> Maybe this should return a const pointer, an image info, and a rowbytes? It
> seems like SkBitmap's API is a mismatch for what the client is supposed to do
> with the result.

+1

[reed1, 2014-02-05 17:47:51]

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/155763004/diff/1/include/core/SkImage.h#newco...
include/core/SkImage.h:87: bool peekPixels(SkBitmap* bitmap) const;
On 2014/02/05 17:13:37, bsalomon wrote:
> Maybe this should return a const pointer, an image info, and a rowbytes? It
> seems like SkBitmap's API is a mismatch for what the client is supposed to do
> with the result.

Interesting. That is precisely what I first typed, but then changed it to
bitmap, so that I could know the lifetime of the client's window into the
pixels. Returning a void* means (which may be ok...)
- the image's pixels must be locked in ram (and decoded)
- they can't be unlocked/moved/purged

If we're fine with these constraints, I can switch back to that Raw version

[reed1, 2014-02-05 18:13:05]

revised signature for peekPixels uploaded

[scroggo, 2014-02-05 18:16:16]

On 2014/02/05 18:13:05, reed1 wrote:
> revised signature for peekPixels uploaded

New signature looks fine to me.

[bsalomon, 2014-02-05 18:26:47]

On 2014/02/05 17:16:06, scroggo wrote:
> On 2014/02/05 17:08:02, reed1 wrote:
> > Related: is there real value in offering...
> > 
> > SkImage* SkImage::newSubset(const SkIRect& subset) const;
> > 
> > Where the new (subset) image may or may not be sharing explicit backend...
> 
> I think it's probably just as valuable as SkBitmap::extractSubset
> 

I think it's more valuable because SkImage is immutable. SkBitmap's version is
ambiguous about whether a copy is made. That means it can't be used by clients
that must have a copy or clients who want to use it to modify a subset of a
bitmap and have that be reflected in the original bitmaps pixels.

new sig lgtm.

[reed1, 2014-02-05 21:00:55]

BTW -- in readPixels(), supporting pre-allocated bitmap is hard. Always
allocating the answer for them is a lot simpler.

[scroggo, 2014-02-05 21:04:40]

On 2014/02/05 21:00:55, reed1 wrote:
> BTW -- in readPixels(), supporting pre-allocated bitmap is hard. Always
> allocating the answer for them is a lot simpler.

I think it's fine to always allocate the answer for them. That goes along with
the new style allocation of bitmaps anyway (where setting its config and
allocating are done in one step).

[bsalomon, 2014-02-05 21:19:03]

On 2014/02/05 21:04:40, scroggo wrote:
> On 2014/02/05 21:00:55, reed1 wrote:
> > BTW -- in readPixels(), supporting pre-allocated bitmap is hard. Always
> > allocating the answer for them is a lot simpler.
> 
> I think it's fine to always allocate the answer for them. That goes along with
> the new style allocation of bitmaps anyway (where setting its config and
> allocating are done in one step).

It's a little sad if the client owns memory where the pixel data must go.

[reed1, 2014-02-05 21:23:58]

ptal

[bsalomon, 2014-02-05 21:36:20]

https://codereview.chromium.org/155763004/diff/210001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/155763004/diff/210001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:63: bounds.fRight = SkMin32(bounds.fRight,
bitmap->width());
does this mean that for a preallocated bitmap the upper left of the bmp is
mapped to the upper left of the image? It seems more intuitive to me to map it
to the upper left of the image subset. Or to just require that the bitmap's w/h
match the subset's w/h. Or to make the prealloc case have a different (public)
entry that isn't over-specified.

[scroggo, 2014-02-05 21:55:27]

https://codereview.chromium.org/155763004/diff/210001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/155763004/diff/210001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:112: if (canvas_supports(info)) {
Should this be

if(!canvas_supports(info))

?

https://codereview.chromium.org/155763004/diff/210001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:116: SkImageInfo info =
SkImageInfo::MakeN32Premul(subset.width(),
Are we okay with the side effect that readPixels returns premultiplied even if
the original SkImage was unpremultiplied?

That is, if the destination bitmap had no existing pixel ref. In that case, we
behave differently.

[reed1, 2014-02-05 22:06:27]

ptal -- hiding readPixels() for now. Have a known client for peekPixels...

[scroggo, 2014-02-05 22:27:23]

On 2014/02/05 22:06:27, reed1 wrote:
> ptal -- hiding readPixels() for now. Have a known client for peekPixels...

peekPixels lgtm. Do we need to check in readPixels yet? Perhaps we can ifdef it
out?

[reed1, 2014-02-06 13:56:16]

On 2014/02/05 22:27:23, scroggo wrote:
> On 2014/02/05 22:06:27, reed1 wrote:
> > ptal -- hiding readPixels() for now. Have a known client for peekPixels...
> 
> peekPixels lgtm. Do we need to check in readPixels yet? Perhaps we can ifdef
it
> out?

It is hidden, but I didn't want to delete the internal plumbing per-se, as I
think it will be useful when we agree upon the public api.

[reed1, 2014-02-06 13:56:44]

On 2014/02/06 13:56:16, reed1 wrote:
> On 2014/02/05 22:27:23, scroggo wrote:
> > On 2014/02/05 22:06:27, reed1 wrote:
> > > ptal -- hiding readPixels() for now. Have a known client for peekPixels...
> > 
> > peekPixels lgtm. Do we need to check in readPixels yet? Perhaps we can ifdef
> it
> > out?
> 
> It is hidden, but I didn't want to delete the internal plumbing per-se, as I
> think it will be useful when we agree upon the public api.

Which I want to work on right after landing peek (which I know I need for
chrome)

[scroggo, 2014-02-06 13:59:57]

On 2014/02/06 13:56:44, reed1 wrote:
> On 2014/02/06 13:56:16, reed1 wrote:
> > On 2014/02/05 22:27:23, scroggo wrote:
> > > On 2014/02/05 22:06:27, reed1 wrote:
> > > > ptal -- hiding readPixels() for now. Have a known client for
peekPixels...
> > > 
> > > peekPixels lgtm. Do we need to check in readPixels yet? Perhaps we can
ifdef
> > it
> > > out?
> > 
> > It is hidden, but I didn't want to delete the internal plumbing per-se, as I
> > think it will be useful when we agree upon the public api.
> 
> Which I want to work on right after landing peek (which I know I need for
> chrome)

Ok. lgtm

[reed1, 2014-02-06 14:12:02]

Committed patchset #7 manually as r13339 (presubmit successful).