When copying a bitmap, copy the generation ID.

When copying a bitmap, copy the generation ID.

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

[Leon, 2012-08-20 18:18:39]

Still need to modify the test so that it tests gpu bitmaps, but I wanted to get
this up so Derek can take a look/use it.

[DerekS, 2012-08-20 19:08:39]

pending the GPU test it lgtm

[bsalomon, 2012-08-20 19:12:44]

http://codereview.appspot.com/6462084/diff/1004/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

http://codereview.appspot.com/6462084/diff/1004/include/core/SkPixelRef.h#new...
include/core/SkPixelRef.h:203: void setGenerationID(uint32_t genID) {
fGenerationID = genID; }
It seems like anyone who has access to this function already has access to
fGenerationID.

[Leon, 2012-08-20 20:28:56]

https://codereview.appspot.com/6462084/diff/1004/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6462084/diff/1004/include/core/SkPixelRef.h#ne...
include/core/SkPixelRef.h:203: void setGenerationID(uint32_t genID) {
fGenerationID = genID; }
On 2012/08/20 19:12:44, bsalomon wrote:
> It seems like anyone who has access to this function already has access to
> fGenerationID.

True. My thinking was that going through a method is clearer, but since I cannot
enforce that I suppose it does not help.

Done in next patch set.

[DerekS, 2012-08-20 20:42:06]

lgtm and do what you want with my comment.

https://codereview.appspot.com/6462084/diff/8002/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.appspot.com/6462084/diff/8002/tests/BitmapCopyTest.cpp#new...
tests/BitmapCopyTest.cpp:275: bool success = src.deepCopyTo(&dst,
gPairs[j].fConfig);
if this isn't going to test the GPU case do we need to change this?

[bsalomon, 2012-08-20 20:54:27]

I don't like the friend but I don't see any easy way around the memcpy case.

https://codereview.appspot.com/6462084/diff/8002/tests/GpuBitmapCopyTest.cpp
File tests/GpuBitmapCopyTest.cpp (right):

https://codereview.appspot.com/6462084/diff/8002/tests/GpuBitmapCopyTest.cpp#...
tests/GpuBitmapCopyTest.cpp:72: if (success) {
Maybe we should readback the values and verify that they are correct.

[Leon, 2012-08-20 22:05:45]

https://codereview.appspot.com/6462084/diff/3003/tests/GpuBitmapCopyTest.cpp
File tests/GpuBitmapCopyTest.cpp (right):

https://codereview.appspot.com/6462084/diff/3003/tests/GpuBitmapCopyTest.cpp#...
tests/GpuBitmapCopyTest.cpp:90: REPORTER_ASSERT(reporter, !memcmp(srcP, dstP,
srcReadBack.getSize()));
Is this what you had in mind? It fails :( Am I doing something wrong here?

[bsalomon, 2012-08-21 15:40:10]

Sorry meant to send this much earlier this morning but never hit publish.

http://codereview.appspot.com/6462084/diff/3003/tests/GpuBitmapCopyTest.cpp
File tests/GpuBitmapCopyTest.cpp (right):

http://codereview.appspot.com/6462084/diff/3003/tests/GpuBitmapCopyTest.cpp#n...
tests/GpuBitmapCopyTest.cpp:90: REPORTER_ASSERT(reporter, !memcmp(srcP, dstP,
srcReadBack.getSize()));
On 2012/08/20 22:05:45, scroggo-work wrote:
> Is this what you had in mind? It fails :( Am I doing something wrong here?
Yeah, does it fail all the time or just on some iterations? Offhand I don't see
anything wrong.

[Leon, 2012-08-21 15:47:19]

http://codereview.appspot.com/6462084/diff/3003/tests/GpuBitmapCopyTest.cpp
File tests/GpuBitmapCopyTest.cpp (right):

http://codereview.appspot.com/6462084/diff/3003/tests/GpuBitmapCopyTest.cpp#n...
tests/GpuBitmapCopyTest.cpp:90: REPORTER_ASSERT(reporter, !memcmp(srcP, dstP,
srcReadBack.getSize()));
On 2012/08/21 15:40:10, bsalomon wrote:
> On 2012/08/20 22:05:45, scroggo-work wrote:
> > Is this what you had in mind? It fails :( Am I doing something wrong here?
> Yeah, does it fail all the time or just on some iterations? Offhand I don't
see
> anything wrong.

It seems to fail consistently in 4444, and succeed in 8888.

[junov1, 2012-08-21 17:36:26]

http://codereview.appspot.com/6462084/diff/3003/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

http://codereview.appspot.com/6462084/diff/3003/include/core/SkPixelRef.h#new...
include/core/SkPixelRef.h:198: friend class SkBitmap;
I'm not sure this is the best way to do this.  A cleaner pattern that is used a
lot in skia is to split methods into base/derived implementations.  For example,
make SkPixelRef::copy non-virtual and add a protected virtual method
SkPixelRef::onCopy.  Move the code that is now in copy over to onCopy.
Implementation of SkPixelRef::copy would be:
1. call copy
2. copy the genID

[DerekS, 2012-08-21 17:49:12]

http://codereview.appspot.com/6462084/diff/3003/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

http://codereview.appspot.com/6462084/diff/3003/include/core/SkPixelRef.h#new...
include/core/SkPixelRef.h:198: friend class SkBitmap;
There are some subtleties to this particular case that make me disagree with
this suggestion.  I think you could make it work but it would be much more
confusing than what he did here.

The big issue is that SkPixelRef doesn't have the information it needs to do an
efficient copy and only the containing SkBitmap has that information.  So in
order to efficiently copy (i.e. memcopy) we need to do the copy in SkBitmap.

On 2012/08/21 17:36:26, junov1 wrote:
> I'm not sure this is the best way to do this.  A cleaner pattern that is used
a
> lot in skia is to split methods into base/derived implementations.  For
example,
> make SkPixelRef::copy non-virtual and add a protected virtual method
> SkPixelRef::onCopy.  Move the code that is now in copy over to onCopy.
> Implementation of SkPixelRef::copy would be:
> 1. call copy
> 2. copy the genID

[Leon, 2012-08-21 17:57:27]

Yes, copying SkPixelRef is tricky.

However, I did just look at SkMallocPixelRef, and it turns out that it does know
its size. We would just have to implement for each subclass.

If we do take this approach, any subclass that does not override the copy
function will either a) not get copied properly or b) not get the genID copied
properly. Which is the right solution?

[DerekS, 2012-08-21 19:15:17]

On 2012/08/21 17:57:27, scroggo-work wrote:
> Yes, copying SkPixelRef is tricky.
> 
> However, I did just look at SkMallocPixelRef, and it turns out that it does
know
> its size. We would just have to implement for each subclass.
> 
> If we do take this approach, any subclass that does not override the copy
> function will either a) not get copied properly or b) not get the genID copied
> properly. Which is the right solution?

It does know it's size, but it doesn't know it's format so it can't do any type
of conversion.  I think that adding copy() would be fine as long as it didn't
take a destination config.

However like you stated it isn't clear to me what the right solution is as both
have their cons.

[Leon, 2012-08-21 20:44:18]

I've uploaded a new patch that fixes the test.

Right now, there is SkPixelRef::deepCopy, which takes a destination config and
returns a new SkPixelRef (or NULL). I could use that. However, as you point out,
SkMallocPixelRef does not know its own config, so I'd need to pass that as well.
I could make SkMallocPixelRef::deepCopy return NULL if the two do not match.

Does that sound like a better approach?

[Leon, 2012-08-21 20:49:35]

Personally, I don't like telling the pixel ref what its own config is, but I
acknowledge the friend solution has its drawbacks.

[bsalomon, 2012-08-21 20:50:39]

On 2012/08/21 20:44:18, scroggo-work wrote:
> I've uploaded a new patch that fixes the test.
> 
> Right now, there is SkPixelRef::deepCopy, which takes a destination config and
> returns a new SkPixelRef (or NULL). I could use that. However, as you point
out,
> SkMallocPixelRef does not know its own config, so I'd need to pass that as
well.
> I could make SkMallocPixelRef::deepCopy return NULL if the two do not match.
> 
> Does that sound like a better approach?

IMO there is a larger refactor that involves moving more info from SkBitmap to
SkPixelRef that is outside the scope of this CL and needn't be addressed by this
change.

[DerekS, 2012-08-21 20:59:06]

I agree with Brian.


On Tue, Aug 21, 2012 at 4:50 PM, <bsalomon@google.com> wrote:

> On 2012/08/21 20:44:18, scroggo-work wrote:
>
>> I've uploaded a new patch that fixes the test.
>>
>
>  Right now, there is SkPixelRef::deepCopy, which takes a destination
>>
> config and
>
>> returns a new SkPixelRef (or NULL). I could use that. However, as you
>>
> point out,
>
>> SkMallocPixelRef does not know its own config, so I'd need to pass
>>
> that as well.
>
>> I could make SkMallocPixelRef::deepCopy return NULL if the two do not
>>
> match.
>
>  Does that sound like a better approach?
>>
>
> IMO there is a larger refactor that involves moving more info from
> SkBitmap to SkPixelRef that is outside the scope of this CL and needn't
> be addressed by this change.
>
>
>
http://codereview.appspot.com/**6462084/<http://codereview.appspot.com/6462084/>
>