Reland "Fix genID cloning bugs."

Reland "Fix genID cloning bugs."

SkBitmap.cpp:
When copyTo calls readPixels, only clone the genID if the resulting
SkPixelRef has the same dimensions as the original. This catches a
bug where copying an SkBitmap representing the subset of an SkPixelRef
(which implements onReadPixels) would result in the copy sharing the
genID. (Thanks to r6710, this case can only happen using setPixelRef,
so the updated GpuBitmapCopyTest checks for that.)

Move some unnecessary NULL checks to asserts.

When copyTo performs a memcpy, only clone the genID if the resulting
SkPixelRef has the same dimensions as the original. This catches a bug
where copying an extracted SkBitmap with the same width as its original
SkPixelRef would incorrectly have the same genID.

Add a comment and assert in deepCopyTo, when cloning the genID, since
that case correctly clones it.

BitmapCopyTest.cpp:
Pull redundant work out of the inner loop (setting up the source bitmaps
and testing extractSubset). Create a new inner loop for extractSubset, to
test copying the result to each different config.

Extract a subset that has the same width as the original, to catch the
bug mentioned above.

Remove the reporter assert which checks for the resulting rowbytes.

Add checks to ensure that copying the extracted subset changes the genID.

GpuBitmapCopyTest:
Create an SkBitmap that shares an existing SkPixelRef, but only represents
a subset. This is to test the first call to cloneGenID in SkBitmap::copyTo.
In this case, the genID should NOT be copied, since only a portion of the
SkPixelRef was copied.

Also test deepCopy on this subset.

TestIndividualCopy now takes a parameter stating whether the genID should
change in the copy. It also does a read back using the appropriate subset.
It no longer differentiates between copyTo and deepCopyTo, since that
distinction was only necessary for copying from/to configs other than 8888
(which are no longer being tested), where copyTo did a read back in 8888 and
then drew the result to the desired config (resulting in an imperfect copy).

BUG=skia:1742
Committed: http://code.google.com/p/skia/source/detail?r=13021

R=mtklein@google.com

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

[scroggo, 2014-01-08 23:34:59]

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

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1104: // succeeded, the new pixel ref must be identical.
Currently, calling deepCopyTo copies the entire SkPixelRef, so we will always
clone the genID here.

We *could* pass the subset to deepCopy, in which case we would only call
cloneGenID if the whole thing was copied. Would that be desirable?

[reed1, 2014-01-09 13:04:50]

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

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:997: if (dst->pixelRef() && this->config() == dstConfig
do we need to compare config && info, or just info?

[mtklein, 2014-01-09 15:58:06]

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

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1045: SkASSERT(pixelRef != NULL);
I might even move this to right after the if(!tmpDst.readyToDraw()) { return
false; }, like we have for fPixelRef.

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1048: if (pixelRef->info() == fPixelRef->info()) {
Is it possible for this test to fail?  They've got the same config, width,
height, and size.  Doesn't that imply pixelRef->info() == fPixelRef->info()?

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

https://codereview.chromium.org/112113005/diff/60001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:244: // test extractSubset
A block like this makes me think you might want to extract a new
DEF_TEST(BitmapCopy_extractSubset, reporter) { ... }

https://codereview.chromium.org/112113005/diff/60001/tests/GpuBitmapCopyTest.cpp
File tests/GpuBitmapCopyTest.cpp (right):

https://codereview.chromium.org/112113005/diff/60001/tests/GpuBitmapCopyTest....
tests/GpuBitmapCopyTest.cpp:47: const bool expectChangedGenID) {
I think I might find this easier to follow inverted as expectSameGenID, but I
can go with the flow here.

https://codereview.chromium.org/112113005/diff/60001/tests/GpuBitmapCopyTest....
tests/GpuBitmapCopyTest.cpp:53: if (expectChangedGenID) {
Don't know if you find this clearer or not, but I think you can write this as

REPORTER_ASSERT(reporter, (src.getGenerationID() != dst.getGenerationID()) ==
expectChangedGenID));

[scroggo, 2014-01-09 20:45:23]

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

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:997: if (dst->pixelRef() && this->config() == dstConfig
On 2014/01/09 13:04:50, reed1 wrote:
> do we need to compare config && info, or just info?

Just info. Fixed.

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1045: SkASSERT(pixelRef != NULL);
On 2014/01/09 15:58:07, mtklein wrote:
> I might even move this to right after the if(!tmpDst.readyToDraw()) { return
> false; }, like we have for fPixelRef.

Done.

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1048: if (pixelRef->info() == fPixelRef->info()) {
On 2014/01/09 15:58:07, mtklein wrote:
> Is it possible for this test to fail?  They've got the same config, width,
> height, and size.  Doesn't that imply pixelRef->info() == fPixelRef->info()?

It is possible for this test to fail, but it's a little bit tricky. The
*bitmaps* have the same config, width, height, and size (rowbytes * height), but
the pixel refs do not necessarily match in all those dimensions. If this bitmap
has a width equal to its pixel ref's width (and the default rowbytes), but its
height is smaller, execution will still get here, but the new pixel ref is not a
complete copy (it's missing some rows).

That said, checking the height of the pixel refs should be sufficient. I've made
that change and added some comments and an assert.

Testing this case resulted in changing line 251 of BitmapCopyTest:

    r.set(0, 1, W, 3);

The original, line 290 (sorry for the move and change...), said:

    r.set(1, 1, 2, 2);

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

https://codereview.chromium.org/112113005/diff/60001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:244: // test extractSubset
On 2014/01/09 15:58:07, mtklein wrote:
> A block like this makes me think you might want to extract a new
> DEF_TEST(BitmapCopy_extractSubset, reporter) { ... }

Why so, for clarity? Or is there an advantage I'm not seeing? To me it makes
sense here because it shares some of the same code (setting up the bitmaps,
iterating through the configs) and it is testing copying the extracted subset,
but I could go either way.

https://codereview.chromium.org/112113005/diff/60001/tests/GpuBitmapCopyTest.cpp
File tests/GpuBitmapCopyTest.cpp (right):

https://codereview.chromium.org/112113005/diff/60001/tests/GpuBitmapCopyTest....
tests/GpuBitmapCopyTest.cpp:47: const bool expectChangedGenID) {
On 2014/01/09 15:58:07, mtklein wrote:
> I think I might find this easier to follow inverted as expectSameGenID, but I
> can go with the flow here.

Agreed. Inverted.

https://codereview.chromium.org/112113005/diff/60001/tests/GpuBitmapCopyTest....
tests/GpuBitmapCopyTest.cpp:53: if (expectChangedGenID) {
On 2014/01/09 15:58:07, mtklein wrote:
> Don't know if you find this clearer or not, but I think you can write this as
> 
> REPORTER_ASSERT(reporter, (src.getGenerationID() != dst.getGenerationID()) ==
> expectChangedGenID));

I find it clever but less clear.

[mtklein, 2014-01-09 21:29:18]

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

https://codereview.chromium.org/112113005/diff/60001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1048: if (pixelRef->info() == fPixelRef->info()) {
On 2014/01/09 20:45:23, scroggo wrote:
> On 2014/01/09 15:58:07, mtklein wrote:
> > Is it possible for this test to fail?  They've got the same config, width,
> > height, and size.  Doesn't that imply pixelRef->info() == fPixelRef->info()?
> 
> It is possible for this test to fail, but it's a little bit tricky. The
> *bitmaps* have the same config, width, height, and size (rowbytes * height),
but
> the pixel refs do not necessarily match in all those dimensions. If this
bitmap
> has a width equal to its pixel ref's width (and the default rowbytes), but its
> height is smaller, execution will still get here, but the new pixel ref is not
a
> complete copy (it's missing some rows).
> 
> That said, checking the height of the pixel refs should be sufficient. I've
made
> that change and added some comments and an assert.
> 
> Testing this case resulted in changing line 251 of BitmapCopyTest:
> 
>     r.set(0, 1, W, 3);
> 
> The original, line 290 (sorry for the move and change...), said:
> 
>     r.set(1, 1, 2, 2);
> 

Thanks, that's tricky.  The new comments help a lot.

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

https://codereview.chromium.org/112113005/diff/60001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:244: // test extractSubset
On 2014/01/09 20:45:23, scroggo wrote:
> On 2014/01/09 15:58:07, mtklein wrote:
> > A block like this makes me think you might want to extract a new
> > DEF_TEST(BitmapCopy_extractSubset, reporter) { ... }
> 
> Why so, for clarity? Or is there an advantage I'm not seeing? To me it makes
> sense here because it shares some of the same code (setting up the bitmaps,
> iterating through the configs) and it is testing copying the extracted subset,
> but I could go either way.

Yeah, clarity.  It's nearly-independent from the code below and it's a feature
that could fail independently of the other things we test below it.  If nothing
else, it'd help readability of that code below, in that we wouldn't have to
think about anything in the extractSubset block setting up state or
preconditions the test depends on.  Same reason we break up code into separate
functions even if they're only called from one place.

That shared setup code itself could be pulled out as a function too:
  static void setup_src_bitmaps(SkBitmap* opaque, SkBitmap* premul) { ... }

My rule of thumb is that, nearly any time you write a freestanding {} block and
it doesn't involve some sort of RAII scope trick, it wants to be its own
function, especially if it's got a comment like // test extractSubset above it. 
It's a major  step up in readability if you can get your functions to fit
entirely within a reasonable screen height, and I find it helps me get a better
handle on data dependencies too when I'm forced to think about which functions
need what.

[scroggo, 2014-01-09 21:47:29]

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

https://codereview.chromium.org/112113005/diff/60001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:244: // test extractSubset
On 2014/01/09 21:29:18, mtklein wrote:
> On 2014/01/09 20:45:23, scroggo wrote:
> > On 2014/01/09 15:58:07, mtklein wrote:
> > > A block like this makes me think you might want to extract a new
> > > DEF_TEST(BitmapCopy_extractSubset, reporter) { ... }
> > 
> > Why so, for clarity? Or is there an advantage I'm not seeing? To me it makes
> > sense here because it shares some of the same code (setting up the bitmaps,
> > iterating through the configs) and it is testing copying the extracted
subset,
> > but I could go either way.
> 
> Yeah, clarity.  It's nearly-independent from the code below and it's a feature
> that could fail independently of the other things we test below it.  If
nothing
> else, it'd help readability of that code below, in that we wouldn't have to
> think about anything in the extractSubset block setting up state or
> preconditions the test depends on.  Same reason we break up code into separate
> functions even if they're only called from one place.
> 
> That shared setup code itself could be pulled out as a function too:
>   static void setup_src_bitmaps(SkBitmap* opaque, SkBitmap* premul) { ... }
> 
> My rule of thumb is that, nearly any time you write a freestanding {} block
and
> it doesn't involve some sort of RAII scope trick, it wants to be its own
> function, especially if it's got a comment like // test extractSubset above
it. 
> It's a major  step up in readability if you can get your functions to fit
> entirely within a reasonable screen height, and I find it helps me get a
better
> handle on data dependencies too when I'm forced to think about which functions
> need what.

Thanks for the justification - you have me convinced! I have moved extractSubset
to its own test.

[mtklein, 2014-01-09 22:05:00]

lgtm

https://codereview.chromium.org/112113005/diff/200001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/112113005/diff/200001/tests/BitmapCopyTest.cp...
tests/BitmapCopyTest.cpp:221: {
These braces probably aren't useful any more. :)

[scroggo, 2014-01-10 14:45:57]

https://codereview.chromium.org/112113005/diff/200001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/112113005/diff/200001/tests/BitmapCopyTest.cp...
tests/BitmapCopyTest.cpp:221: {
On 2014/01/09 22:05:00, mtklein wrote:
> These braces probably aren't useful any more. :)

Removed.

[commit-bot: I haz the power, 2014-01-10 14:46:18]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/scroggo@google.com/112113005/250001

[commit-bot: I haz the power, 2014-01-10 16:43:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/scroggo@google.com/112113005/320001

[commit-bot: I haz the power, 2014-01-10 17:22:05]

Change committed as 13021

[caryclark, 2014-01-10 18:28:05]

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

The reason for reverting is: breaks unit tests.

[scroggo, 2014-01-10 20:56:33]

PTAL. I have included Mike's change from
https://codereview.chromium.org/134493002/

[scroggo, 2014-01-15 16:56:58]

Committed patchset #10 manually as r13090 (presubmit successful).