Always store pixels of mutable bitmaps when recording a SkPicture.

Always store pixels of mutable bitmaps when recording a SkPicture.

Prior to this CL mutable bitmaps only saved a copy of their pixels
if a flag was set at recording time.  That flag has been removed
and the default behavior when recording a mutable bitmap is to
make a copy of it's pixels. This is the only way to ensure that
the pixels are not manipulated before we playback their contents.

However, enabling this behavior breaks the recording of extracted
bitmaps in SkPicture. This is because we currently cache bitmaps
within a picture based only on their pixelRef. This results in
false positive cache hit when drawing an extracted bitmap as it
shares a pixelRef with its orginating bitmap.  Therefore we must
update the index of the bitmap cache to be both the pixelRef AND
the size and offset of the bitmap using those pixels.

BUG=
TEST=extractbitmap.cpp

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

[reed1, 2012-07-25 12:43:26]

pipe does this correctly I think. Can we be sharing more code?

[DerekS, 2012-07-25 12:57:05]

On 2012/07/25 12:43:26, reed1 wrote:
> pipe does this correctly I think. Can we be sharing more code?

actually looking at the pipe code I think it has the same problem that I just
fixed in SkPicture. The pipe only stores the pixels if it is cross process and
doesn't check to see if the bitmap is mutable or not.  I can work with Leon on a
follow up CL that fixes the issue in pipe in a way that we share as much code as
possible.

[junov1, 2012-07-25 13:04:06]

Actually, the pipe flattens bitmaps if it is cross process, and copies mutable
bitmaps when it is not cross process, which is more efficient than flattening. 
That behavior is in class SharedHeap in SkGPipeWrite.cpp. So, SkGPipe already
handles mutable bitmaps safely

[DerekS, 2012-07-25 13:25:44]

On 2012/07/25 13:04:06, junov1 wrote:
> Actually, the pipe flattens bitmaps if it is cross process, and copies mutable
> bitmaps when it is not cross process, which is more efficient than flattening.

> That behavior is in class SharedHeap in SkGPipeWrite.cpp. So, SkGPipe already
> handles mutable bitmaps safely

You are correct and it looks like SharedHeap has some similarities to what
SkPicture does for storage.  The pipe doesn't need to grow indefinitely like
SkPicture and can therefore do some LRU caching, but I do think we can share
some common code like comparing bitmaps for pixel equality.

[junov1, 2012-07-25 13:35:03]

http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp#new...
src/core/SkPictureRecord.cpp:644: return !bitmap.isImmutable() && NULL ==
bitmap.getTexture();
Why check bitmap.getTexture()?  Texture backed bitmaps are not any safer than
bitmaps stored in ram.

The original intent of that check was to serve the behavior of the
kFlattenMutableNonTexturePixelRefs_RecordingFlag, which was specific to
non-texture pixels refs.

By the way, removing this flag will break a performance optimization in the
SkPicture back-end of SkDeferredCanvas.  I intend to retire that implementation
soon, once the SkGPipe back-end is well proven.  I still think it is a little
early to break the SkPicture-based implementation.

http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp#new...
src/core/SkPictureRecord.cpp:658: entry.fHeight = bitmap.height();
So this makes it safe to index subset bitmaps in the index cache?  Should have a
test to verify this. For example a gm that draws several subsets from the same
source bitmap. 

This could be a separate CL

http://codereview.appspot.com/6439043/diff/1/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/6439043/diff/1/src/utils/SkDeferredCanvas.cpp#n...
src/utils/SkDeferredCanvas.cpp:650: if (bitmap.isImmutable() ||
fPicture.willFlattenPixelsOnRecord(bitmap)) {
This will no longer work like before.  The intended behavior was to flush and
render immediately rather than flatten when encountering a GPU bitmap.  Without
the kFlattenMutableNonTexturePixelRefs_RecordingFlag, the SkPicture will no
longer make that check. 
I have a CL in the works that will make this problem easy to workaround.  Let me
land it first.

[DerekS, 2012-07-25 14:01:36]

http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp#new...
src/core/SkPictureRecord.cpp:644: return !bitmap.isImmutable() && NULL ==
bitmap.getTexture();
On 2012/07/25 13:35:03, junov1 wrote:
> Why check bitmap.getTexture()?  Texture backed bitmaps are not any safer than
> bitmaps stored in ram.
> 
> The original intent of that check was to serve the behavior of the
> kFlattenMutableNonTexturePixelRefs_RecordingFlag, which was specific to
> non-texture pixels refs.
> 
> By the way, removing this flag will break a performance optimization in the
> SkPicture back-end of SkDeferredCanvas.  I intend to retire that
implementation
> soon, once the SkGPipe back-end is well proven.  I still think it is a little
> early to break the SkPicture-based implementation.

I talked with Brian about this and we both thought that if the bitmap had a
texture then we wouldn't have any pixels to flatten.  Without this texture check
we would go ahead and try to flatten the pixels which would do nothing as the
SkGrPixelRef is currently marked unflattenable.  However, upon thinking about it
I think putting the check here is a mistake and we should just let if fall
through, which for now will be a no-op, but could become something in the
future.

I guess I am confused as to how this breaks DeferredCanvas, which I thought it
always set that flag to true.  If that is the case then I don't think I changed
its behavior.  I was able to run all the gm's and I didn't break any of the
deferred canvas drawing passes.

http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp#new...
src/core/SkPictureRecord.cpp:658: entry.fHeight = bitmap.height();
On 2012/07/25 13:35:03, junov1 wrote:
> So this makes it safe to index subset bitmaps in the index cache?  Should have
a
> test to verify this. For example a gm that draws several subsets from the same
> source bitmap. 
> 
> This could be a separate CL

Agreed a separate GM would be good.  As a note these are the same fields that
the pipe's sharedHeap code uses to determine the equality of bitmap pixels.

http://codereview.appspot.com/6439043/diff/1/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/6439043/diff/1/src/utils/SkDeferredCanvas.cpp#n...
src/utils/SkDeferredCanvas.cpp:650: if (bitmap.isImmutable() ||
fPicture.willFlattenPixelsOnRecord(bitmap)) {
On 2012/07/25 13:35:03, junov1 wrote:
> This will no longer work like before.  The intended behavior was to flush and
> render immediately rather than flatten when encountering a GPU bitmap. 
Without
> the kFlattenMutableNonTexturePixelRefs_RecordingFlag, the SkPicture will no
> longer make that check. 
> I have a CL in the works that will make this problem easy to workaround.  Let
me
> land it first.

I believe that without that flag the SkPicture will always make the check.  How
is that not the case?

[junov, 2012-07-25 15:20:45]

On Wed, Jul 25, 2012 at 10:01 AM, <djsollen@google.com> wrote:

>
>>
> I talked with Brian about this and we both thought that if the bitmap
> had a texture then we wouldn't have any pixels to flatten.  Without this
> texture check we would go ahead and try to flatten the pixels which
> would do nothing as the SkGrPixelRef is currently marked unflattenable.
> However, upon thinking about it I think putting the check here is a
> mistake and we should just let if fall through, which for now will be a
> no-op, but could become something in the future.
>

In that case there should probably be an SkASSERT.


> I guess I am confused as to how this breaks DeferredCanvas, which I
> thought it always set that flag to true.  If that is the case then I
> don't think I changed its behavior.  I was able to run all the gm's and
> I didn't break any of the deferred canvas drawing passes.
>
>
Hehe, gm does verify SkDeferredCanvas, but not with a GPU device.
Furthermore, none of
the gm tests create source images that are mutable GPU-backed bitmaps.  Try
running the chrome layout test trybots with this change.
It should fail some of the canvas-to-canvas drawImage tests on the GPU.


>
> http://codereview.appspot.com/**6439043/diff/1/src/core/**
>
SkPictureRecord.cpp#newcode658<http://codereview.appspot.com/6439043/diff/1/src/core/SkPictureRecord.cpp#newcode658>
> src/core/SkPictureRecord.cpp:**658: entry.fHeight = bitmap.height();
> On 2012/07/25 13:35:03, junov1 wrote:
>
>> So this makes it safe to index subset bitmaps in the index cache?
>>
> Should have a
>
>> test to verify this. For example a gm that draws several subsets from
>>
> the same
>
>> source bitmap.
>>
>
>  This could be a separate CL
>>
>
> Agreed a separate GM would be good.  As a note these are the same fields
> that the pipe's sharedHeap code uses to determine the equality of bitmap
> pixels.
>
>
> http://codereview.appspot.com/**6439043/diff/1/src/utils/**
>
SkDeferredCanvas.cpp<http://codereview.appspot.com/6439043/diff/1/src/utils/SkDeferredCanvas.cpp>
> File src/utils/SkDeferredCanvas.cpp (right):
>
> http://codereview.appspot.com/**6439043/diff/1/src/utils/**
>
SkDeferredCanvas.cpp#**newcode650<http://codereview.appspot.com/6439043/diff/1/src/utils/SkDeferredCanvas.cpp#newcode650>
> src/utils/SkDeferredCanvas.**cpp:650: if (bitmap.isImmutable() ||
> fPicture.**willFlattenPixelsOnRecord(**bitmap)) {
> On 2012/07/25 13:35:03, junov1 wrote:
>
>> This will no longer work like before.  The intended behavior was to
>>
> flush and
>
>> render immediately rather than flatten when encountering a GPU bitmap.
>>
>  Without
>
>> the kFlattenMutableNonTexturePixel**Refs_RecordingFlag, the SkPicture
>>
> will no
>
>> longer make that check.
>> I have a CL in the works that will make this problem easy to
>>
> workaround.  Let me
>
>> land it first.
>>
>
> I believe that without that flag the SkPicture will always make the
> check.  How is that not the case?
>

Oh yeah, if you leave in the controversial check on bitmap.getTexture(),
then it is OK I think


>
>
http://codereview.appspot.com/**6439043/<http://codereview.appspot.com/6439043/>
>

[DerekS, 2012-07-27 15:08:04]

now that junov has removed the need for the deferred canvas to ask the picture
if it will flatten pixels, I think this CL is now safe to check in.

[reed1, 2012-07-27 15:15:05]

if juno agrees, I'm all for removing APIs

[junov1, 2012-07-27 15:18:18]

On 2012/07/27 15:15:05, reed1 wrote:
> if juno agrees, I'm all for removing APIs

Yep, we're good.