Remove Bitmaps Raw Pixel Support.

Remove Bitmaps Raw Pixel Support.

bitmap.setPixels(...) now creates a mutable pixelRef instead
of just setting fPixels.

[reed1, 2012-07-09 14:14:26]

https://codereview.appspot.com/6344076/diff/4001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.appspot.com/6344076/diff/4001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:1444: } else {
might be simplier to use (as you did in the reader below)

char* dst = buffer.reserve...
const char* src = (const char*)fPixels;
for (unsigned i = 0; i < fHeight; ++i) {
    memcpy(dst, src, flattenedRowBytes);
    src += fRowBytes;
    dst += flattenedRowBytes;
}

https://codereview.appspot.com/6344076/diff/4001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:1493: size_t flattenedSize = flattenedRowBytes * fHeight;
Do you mean <= here?

[DerekS, 2012-07-16 13:13:58]

https://codereview.appspot.com/6344076/diff/4001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.appspot.com/6344076/diff/4001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:1444: } else {
On 2012/07/09 14:14:26, reed1 wrote:
> might be simplier to use (as you did in the reader below)
> 
> char* dst = buffer.reserve...
> const char* src = (const char*)fPixels;
> for (unsigned i = 0; i < fHeight; ++i) {
>     memcpy(dst, src, flattenedRowBytes);
>     src += fRowBytes;
>     dst += flattenedRowBytes;
> }

Done.

https://codereview.appspot.com/6344076/diff/4001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:1493: size_t flattenedSize = flattenedRowBytes * fHeight;
On 2012/07/09 14:14:26, reed1 wrote:
> Do you mean <= here?

Yes.

[junov1, 2012-07-16 13:40:07]

I would like to see a gm test that draws multiple subsets of the same source
bitmap.

My concern is that we must make sure that the pixels get re-flattened each time
even though they are from the same pixelref. I am pretty sure this CL breaks
SkPicture in that respect, and possibly SkGPipe as well.  If you look at
SkPictureRecord::find(const SkBitmap& bitmap), you will see that flattened
bitmaps get indexed by generationID, which is problematic.

I would also like to see a bench test for the same kind of use case. Think of a
web game that uses a single bitmap as a sprite map (very common use case).  In
the case where most sprites are used at each frame, what kind of performance
degradation do we get from this CL?

[TomH, 2012-07-16 14:05:50]

Maybe I'm missing something, Justin - why do you expect performance degradation?

[DerekS, 2012-07-16 14:14:36]

On 2012/07/16 14:05:50, TomH wrote:
> Maybe I'm missing something, Justin - why do you expect performance
degradation?

I think you may be misinterpreting what this CL is doing.  The CL shouldn't
break any existing code as we are just being smarter about writing the pixels
during serialization.  Currently we write the extracted (i.e. subset) portion of
the original bitmap and in doing so include a bunch of padding.  This CL just
removes the padding at record time so that we don't spend time serializing a
bunch of unused data.

[DerekS, 2012-07-18 13:59:26]

On 2012/07/16 14:14:36, DerekS wrote:
> On 2012/07/16 14:05:50, TomH wrote:
> > Maybe I'm missing something, Justin - why do you expect performance
> degradation?
> 
> I think you may be misinterpreting what this CL is doing.  The CL shouldn't
> break any existing code as we are just being smarter about writing the pixels
> during serialization.  Currently we write the extracted (i.e. subset) portion
of
> the original bitmap and in doing so include a bunch of padding.  This CL just
> removes the padding at record time so that we don't spend time serializing a
> bunch of unused data.

Ok, So this CL has completely changed it's purpose so I'm going to delete it and
upload it as a new CL.  After going deeper in the rabbit hole I'm going to break
up the work into 3 different CLs.

1. Remove support for the concept of SkBitmaps with pixels that are not in a
pixel ref.
2. Fix picture recording when we have a picture that has an extracted bitmap. 
I've got a local change to the extract bitmap GM that demonstrates this problem.
3. Fix picture recording to always serialize a mutable bitmap when we first see
it. This is hidden behind a flag currently and should be the default behavior.

[reed1, 2012-07-18 14:33:36]

http://codereview.appspot.com/6344076/diff/16001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

http://codereview.appspot.com/6344076/diff/16001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:358: bool SkBitmap::lockPixelsAreWritable() const {
return (I don't think we need/use these params) ? :

http://codereview.appspot.com/6344076/diff/16001/src/core/SkMallocPixelRef.cpp
File src/core/SkMallocPixelRef.cpp (right):

http://codereview.appspot.com/6344076/diff/16001/src/core/SkMallocPixelRef.cp...
src/core/SkMallocPixelRef.cpp:14: if (NULL == storage) {
Lets either assert here that managePixels is true, or set it to true, since we
*are* allocating the memory

[DerekS, 2012-07-18 14:56:16]

http://codereview.appspot.com/6344076/diff/16001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

http://codereview.appspot.com/6344076/diff/16001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:358: bool SkBitmap::lockPixelsAreWritable() const {
On 2012/07/18 14:33:36, reed1 wrote:
> return (I don't think we need/use these params) ? :

SyntaxError: invalid syntax

http://codereview.appspot.com/6344076/diff/16001/src/core/SkMallocPixelRef.cpp
File src/core/SkMallocPixelRef.cpp (right):

http://codereview.appspot.com/6344076/diff/16001/src/core/SkMallocPixelRef.cp...
src/core/SkMallocPixelRef.cpp:14: if (NULL == storage) {
On 2012/07/18 14:33:36, reed1 wrote:
> Lets either assert here that managePixels is true, or set it to true, since we
> *are* allocating the memory

To preserve the old behavior I think we should add managePixels to this if
statement to ensure we only create it if managePixels is set to true.

[junov1, 2012-07-18 15:10:01]

> 3. Fix picture recording to always serialize a mutable bitmap when we first
see
> it. This is hidden behind a flag currently and should be the default behavior.

Don't forget to check the pixelRef's genID because mutable bitmaps may be
modified at any time, so they may need to be re-serialized.

[DerekS, 2012-07-18 15:27:14]

On 2012/07/18 15:10:01, junov1 wrote:
> > 3. Fix picture recording to always serialize a mutable bitmap when we first
> see
> > it. This is hidden behind a flag currently and should be the default
behavior.
> 
> Don't forget to check the pixelRef's genID because mutable bitmaps may be
> modified at any time, so they may need to be re-serialized.

Yep, it is actually deeper than that as right now we have a bug in picture
recording because we assume that if the pixelRef has the right genId then the
bitmaps are the same and we don't create another dictionary entry in the
picture.  This causes a problem when we have extracted a bitmap from another
bitmap, because while they share the sample pixelRef (with the same ref count)
they have different dimensions and offsets.  I plan on fixing that in my second
CL.

[junov1, 2012-07-18 16:04:15]

So genID + subrect would be an appropriate key?
I think doing that would address the correctness concerns I raised earlier.

I am still concerned about performance for the sprite map use case: When
all (or almost all) of the source bitmap ends up being used to draw a
frame, but we only use one subrect at a time, do we get a significant
performance degradation from the additional overhead of breaking up the
bitmap and serializing it one subrect at a time?  I would like to get an
idea of how this will impact GPU rendering, in particular, we will be
multiplying the quantity of texture uploads by doing this, and we may end
up hitting the object count ceiling of the texture cache much faster.

Looking at SkGpuDevice::lockCachedTexture, it seems like the texture cache
is building a key that takes into account genID, pixelRefOffset, width and
height. Therefore, I am pretty sure the texture cache can robustly handle
subset bitmaps robustly when rendering directly.  During SkPicture
playback, it might be trickier, depending on how the subsets are
unflattened.

On Wed, Jul 18, 2012 at 11:27 AM, <djsollen@google.com> wrote:

> On 2012/07/18 15:10:01, junov1 wrote:
>
>> > 3. Fix picture recording to always serialize a mutable bitmap when
>>
> we first
>
>> see
>> > it. This is hidden behind a flag currently and should be the default
>>
> behavior.
>
>  Don't forget to check the pixelRef's genID because mutable bitmaps may
>>
> be
>
>> modified at any time, so they may need to be re-serialized.
>>
>
> Yep, it is actually deeper than that as right now we have a bug in
> picture recording because we assume that if the pixelRef has the right
> genId then the bitmaps are the same and we don't create another
> dictionary entry in the picture.  This causes a problem when we have
> extracted a bitmap from another bitmap, because while they share the
> sample pixelRef (with the same ref count) they have different dimensions
> and offsets.  I plan on fixing that in my second CL.
>
>
http://codereview.appspot.com/**6344076/<http://codereview.appspot.com/6344076/>
>