add readPixels() to SkBitmap

add readPixels() to SkBitmap

BUG=chromium:390782
Committed: https://skia.googlesource.com/skia/+/c4f216151b6ade70c35fade09a353052f40236b1

[scroggo, 2014-07-09 15:22:02]

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

https://codereview.chromium.org/377303002/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:612: bool readPixels(const SkImageInfo&, void* pixels,
size_t rowBytes, int x, int y) const;
I take it the intent to perform the work done in copyTo here? Might there be a
better name that makes it more clear that it might end up doing conversions
(e.g. SkDitherARGB32To4444)? copyTo also does more than a simple "copy", but I
think it makes more sense because it means copying a bitmap (which in turn does
more work on the pixels), but this function does more work on the pixels
themselves.

It looks like SkCanvas::readPixels behaves similarly, so it's at least
consistent.

Should we make the names of the parameters explicit about what they refer to?
Maybe it's obvious enough that x and y refer to the source, and the others refer
to the destination.

Otherwise this seems like a good API to me.

Will it work for texture-backed bitmaps? I'm guessing not, since those won't
always exist and it allows us to keep this simpler. Maybe we should document
that?

[reed1, 2014-07-10 14:10:40]

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

https://codereview.chromium.org/377303002/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:612: bool readPixels(const SkImageInfo&, void* pixels,
size_t rowBytes, int x, int y) const;
On 2014/07/09 15:22:02, scroggo wrote:
> I take it the intent to perform the work done in copyTo here? Might there be a
> better name that makes it more clear that it might end up doing conversions
> (e.g. SkDitherARGB32To4444)? copyTo also does more than a simple "copy", but I
> think it makes more sense because it means copying a bitmap (which in turn
does
> more work on the pixels), but this function does more work on the pixels
> themselves.

This function is intended to be a mirror of canvas::readPixels, which already
does pixel transformations, so I think that's consistent.

It will be the underside impl for copyTo, but will not perform any memory
allocations.

> 
> It looks like SkCanvas::readPixels behaves similarly, so it's at least
> consistent.
> 
> Should we make the names of the parameters explicit about what they refer to?
> Maybe it's obvious enough that x and y refer to the source, and the others
refer
> to the destination.

Good idea, will try renaming them.

> 
> Otherwise this seems like a good <span id="abbrex-spnAbbr_43633"
class="abbrex-Abbr abbrex-Abbr_API abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">API</span> to me.
> 
> Will it work for texture-backed bitmaps? I'm guessing not, since those won't
> always exist and it allows us to keep this simpler. Maybe we should document
> that?

It will, as long as bitmaps can be texture backed. This is consistent with
canvas::readPixels, which works if the canvas is texture/gpu backed.

[reed1, 2014-07-10 15:09:38]

ptal

[scroggo, 2014-07-10 15:09:49]

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

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:860: if (0 == dstInfo.width() || 0 == dstInfo.height()) {
Arguably this could return true.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:872: info.fHeight = srcR.height();
You never use info after this (except to check bytesPerPixel, which is unchanged
from dstInfo). I'm guessing you meant to use it down below when you use dstInfo.

It might be clearer if you change the parameter from dstInfo to requestedDstInfo
and this one to dstInfo.

That brings up the larger question though: if the caller tried to copy a
rectangle that is not a subset of pixels, why did we choose to copy a subset,
rather than report failure? Is it possible we're covering up their mistake?

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:894: srcInfo.fWidth = dstInfo.width();
shouldn't this be:

    srcInfo.fWidth = info.width()

and same for height? They could have been shrunk by the intersection. (or just
look at srcR)

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:981: //  The old copyTo impl did this, so we continue it
for now.
I don't agree that this is a hack. The point of the genID is to show that the
pixels are the same, which they are.

You mention that we do it for now; what would be a better long term solution?

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (left):

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmapDevice....
src/core/SkBitmapDevice.cpp:227: // perhaps can relax these in the future
Why did we have these restrictions in the past? Did we change something so
they're no longer necessary?

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cpp
File src/core/SkConfig8888.cpp (right):

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cp...
src/core/SkConfig8888.cpp:1: #include "SkBitmap.h"
Could you add the copyright header as well?

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cp...
src/core/SkConfig8888.cpp:160: // If they agree on colorType, then we ignore
alphaType and just memcpy.
I'm not sure this is the correct behavior (it's also not true in the case of
4444). If we do behave this way, can you add a comment in the declaration?

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cp...
src/core/SkConfig8888.cpp:185: // Can no longer draw directly into 4444, but we
can manually wack it for a few combinations
whack*

https://codereview.chromium.org/377303002/diff/60001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/377303002/diff/60001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:272: DEF_TEST(BitmapCopy, reporter) {
Should we add tests that check for copying a subset using readPixels?

[reed1, 2014-07-10 15:32:16]

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

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:860: if (0 == dstInfo.width() || 0 == dstInfo.height()) {
On 2014/07/10 15:09:48, scroggo wrote:
> Arguably this could return true.

Possibly. Below we return false if the srcR doesn't intersect the bounds, which
would trigger if srcR were empty. I guess I could go either way. If we change
this, I would update canvas::readpixels as well.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:872: info.fHeight = srcR.height();
On 2014/07/10 15:09:48, scroggo wrote:
> You never use info after this (except to check bytesPerPixel, which is
unchanged
> from dstInfo). I'm guessing you meant to use it down below when you use
dstInfo.

Good catch.

> 
> It might be clearer if you change the parameter from dstInfo to
requestedDstInfo
> and this one to dstInfo.

Agreed.

> 
> That brings up the larger question though: if the caller tried to copy a
> rectangle that is not a subset of pixels, why did we choose to copy a subset,
> rather than report failure? Is it possible we're covering up their mistake?

Not necessarily a mistake, but who knows. This allows the caller to have us
write into a subset of their buffer easily. Again, canvas::readpixels has
supported this all along, but we can always rethink this (but possibly in a
different CL).

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:894: srcInfo.fWidth = dstInfo.width();
On 2014/07/10 15:09:48, scroggo wrote:
> shouldn't this be:
> 
>     srcInfo.fWidth = info.width()
> 
> and same for height? They could have been shrunk by the intersection. (or just
> look at srcR)

Agreed. I will make the suggested change above, and that will trickle down and
fix this.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:981: //  The old copyTo impl did this, so we continue it
for now.
On 2014/07/10 15:09:48, scroggo wrote:
> I don't agree that this is a hack. The point of the genID is to show that the
> pixels are the same, which they are.
> 
> You mention that we do it for now; what would be a better long term solution?

I don't agree with the hack either, but BitmapHeap and Pipe will need to be
changed if we remove this. Sounds to me like a separate CL, since this is the
current behavior, and not related to this change.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (left):

https://codereview.chromium.org/377303002/diff/60001/src/core/SkBitmapDevice....
src/core/SkBitmapDevice.cpp:227: // perhaps can relax these in the future
On 2014/07/10 15:09:48, scroggo wrote:
> Why did we have these restrictions in the past? Did we change something so
> they're no longer necessary?

copy_pixels only handled 4byte src/dst. Now CopyPixels does all the work that
was in copyTo, so we can remove this restriction. This brings bitmapdevice more
inline with gpudevice, which could do more swizzles.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cpp
File src/core/SkConfig8888.cpp (right):

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cp...
src/core/SkConfig8888.cpp:1: #include "SkBitmap.h"
On 2014/07/10 15:09:48, scroggo wrote:
> Could you add the copyright header as well?

Done.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cp...
src/core/SkConfig8888.cpp:160: // If they agree on colorType, then we ignore
alphaType and just memcpy.
On 2014/07/10 15:09:48, scroggo wrote:
> I'm not sure this is the correct behavior (it's also not true in the case of
> 4444). If we do behave this way, can you add a comment in the declaration?

This is what we did in the old code for both copyTo and readPixels. However, I
agree it might not be strictly correct.

I will update the above comment to reflect 4444's check, and/or handle this more
non-blindly.

https://codereview.chromium.org/377303002/diff/60001/src/core/SkConfig8888.cp...
src/core/SkConfig8888.cpp:185: // Can no longer draw directly into 4444, but we
can manually wack it for a few combinations
On 2014/07/10 15:09:48, scroggo wrote:
> whack*

Done.

https://codereview.chromium.org/377303002/diff/60001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/377303002/diff/60001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:272: DEF_TEST(BitmapCopy, reporter) {
On 2014/07/10 15:09:48, scroggo wrote:
> Should we add tests that check for copying a subset using readPixels?

Done.

[reed1, 2014-07-10 18:19:44]

ptal

[scroggo, 2014-07-10 20:01:40]

LGTM

nit: Could you put BUG=chromium:390782 in the description?

(I'd be fine without that since it has larger effects, but it is what motivated
this, and I think it will make that bug get updated automagically with this CL.)

https://codereview.chromium.org/377303002/diff/100001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/377303002/diff/100001/tests/BitmapCopyTest.cp...
tests/BitmapCopyTest.cpp:596: // If fExpectedSucceds, check these, otherwise
ignore
nit: fExpectedSuccess*

[reed1, 2014-07-10 20:06:09]

https://codereview.chromium.org/377303002/diff/100001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/377303002/diff/100001/tests/BitmapCopyTest.cp...
tests/BitmapCopyTest.cpp:596: // If fExpectedSucceds, check these, otherwise
ignore
On 2014/07/10 20:01:40, scroggo wrote:
> nit: fExpectedSuccess*

Done.

[reed1, 2014-07-10 20:06:26]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-07-10 20:07:16]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/377303002/120001

[commit-bot: I haz the power, 2014-07-10 21:16:02]

Change committed as c4f216151b6ade70c35fade09a353052f40236b1

[jcgregorio, 2014-07-11 14:33:12]

On 2014/07/10 21:16:02, I haz the power (commit-bot) wrote:
> Change committed as c4f216151b6ade70c35fade09a353052f40236b1

This is the last change to SkBitmap which seems to be where bench is now
crashing:

  
http://108.170.220.21:10117/builders/Test-Android-Xoom-Tegra2-Arm7-Debug/buil...

[jcgregorio, 2014-07-11 14:47:58]

A revert of this CL has been created in
https://codereview.chromium.org/382243003/ by jcgregorio@google.com.

The reason for reverting is: Maybe causing crashes in bench.

http://108.170.220.120:10117/builders/Test-ChromeOS-Daisy-MaliT604-Arm7-Debug....

[scroggo, 2014-07-11 15:40:15]

On 2014/07/11 14:47:58, jcgregorio wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/382243003/ by mailto:jcgregorio@google.com.
> 
> The reason for reverting is: Maybe causing crashes in bench.
> 
>
http://108.170.220.120:10117/builders/Test-ChromeOS-Daisy-MaliT604-Arm7-Debug....

I find it weird that this doesn't seem to be happening consistently. I am unable
to reproduce locally. That said, it
looks like we always crash in the test premul_and_unpremul_alpha_BGRA_8888, with
the assertion:

  SkAssert(r <= a)

inside SkColorPriv.h's SkPMColorAssert. This would mean we're trying to use a
color that is not premultiplied. The only
place this function is called is in various SkBlitRow files, leading me to
believe we're attempting to draw with an
unpremultiplied color. Looking at the test a little bit, it appears we use
SkColorSetARGB to set colors in an SkBitmap.
This seems wrong, given that the function/macro returns an SkColor, whereas the
address it is assigned to expects an
SkPMColor. Unlike SkPackARGB32, this function does not assert that the color is
a valid premultiplied color (since
the expectation is that it will be converted from an SkColor later). I've made
some comments in the CL that
introduced that part of the test (see
https://codereview.chromium.org/199413013/#msg20). Brian pointed out that the
test actually wants unpremultiplied data, in which case I think we should be
setting the colortype properly. I'm
guessing that this CL resulted in attempting to draw that data. The fact that it
happens inconsistently is also
worrying though.