For SkGPipe, store bitmaps in a common location.

For SkGPipe, store bitmaps in a common location.

Also make a change to SkBitmap::copyTo() so that we do not
crash.
Committed: https://code.google.com/p/skia/source/detail?r=4249

[Leon, 2012-06-12 21:10:28]

I am now using the new methods writePtr and readPtr

[reed1, 2012-06-12 21:21:25]

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:77: 
Is it useful to use the heap for 'immutable' bitmap as well? If so, we could (I
guess) sniff the bitmap, and if its mutable, make a shallow copy instead of a
deep one.

if (src.isImmutable()) {
    copy = new SkBitmap(src);
} else {
    src.copyTo(copy)....
}

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:87: bool addBitmap(const SkBitmap& bm, void**pointer)
{
Maybe this guy can just return 'const SkBitmap*'

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:102: }
} else {
    delete copy;
}

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:107: SkBitmap* fBitmap;
// We store the genID of the caller's bitmap, which will differ from our copy's

[Leon, 2012-06-12 21:46:50]

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:77: 
On 2012/06/12 21:21:25, reed1 wrote:
> Is it useful to use the heap for 'immutable' bitmap as well? If so, we could
(I
> guess) sniff the bitmap, and if its mutable, make a shallow copy instead of a
> deep one.
> 
> if (src.isImmutable()) {
>     copy = new SkBitmap(src);
> } else {
>     src.copyTo(copy)....
> }

That would mean less copying, but then the SkPixelRef won't necessarily be in
the shared address space (once we start using the shared address space at
all...).

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:87: bool addBitmap(const SkBitmap& bm, void**pointer)
{
On 2012/06/12 21:21:25, reed1 wrote:
> Maybe this guy can just return 'const SkBitmap*'

Done.

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:102: }
On 2012/06/12 21:21:25, reed1 wrote:
> } else {
>     delete copy;
> }

Done.

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:107: SkBitmap* fBitmap;
On 2012/06/12 21:21:25, reed1 wrote:
> // We store the genID of the caller's bitmap, which will differ from our
copy's

Done.

[reed1, 2012-06-13 14:55:43]

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:77: 
True, this immutable check would only be done if the Heap also was told it was
in "normal" address mode, as opposed to "shared-cross-process" address mode.

On 2012/06/12 21:46:50, scroggo1 wrote:
> On 2012/06/12 21:21:25, reed1 wrote:
> > Is it useful to use the heap for 'immutable' bitmap as well? If so, we could
> (I
> > guess) sniff the bitmap, and if its mutable, make a shallow copy instead of
a
> > deep one.
> > 
> > if (src.isImmutable()) {
> >     copy = new SkBitmap(src);
> > } else {
> >     src.copyTo(copy)....
> > }
> 
> That would mean less copying, but then the SkPixelRef won't necessarily be in
> the shared address space (once we start using the shared address space at
> all...).

https://codereview.appspot.com/6296066/diff/6002/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6296066/diff/6002/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:83: */
return const SkBitmap* ?

[reed1, 2012-06-13 14:56:35]

How is the heap informed when it can release the bitmap?

[Leon, 2012-06-13 16:24:18]

> How is the heap informed when it can release the bitmap?

At the moment, it is not (there is a TODO regarding that). I have not yet
figured that out for the case with multiple readers.

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6296066/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:77: 
On 2012/06/13 14:55:44, reed1 wrote:
> True, this immutable check would only be done if the Heap also was told it was
> in "normal" address mode, as opposed to "shared-cross-process" address mode.
> 
> On 2012/06/12 21:46:50, scroggo1 wrote:
> > On 2012/06/12 21:21:25, reed1 wrote:
> > > Is it useful to use the heap for 'immutable' bitmap as well? If so, we
could
> > (I
> > > guess) sniff the bitmap, and if its mutable, make a shallow copy instead
of
> a
> > > deep one.
> > > 
> > > if (src.isImmutable()) {
> > >     copy = new SkBitmap(src);
> > > } else {
> > >     src.copyTo(copy)....
> > > }
> > 
> > That would mean less copying, but then the SkPixelRef won't necessarily be
in
> > the shared address space (once we start using the shared address space at
> > all...).
> 

I have added a TODO

https://codereview.appspot.com/6296066/diff/6002/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6296066/diff/6002/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:83: */
On 2012/06/13 14:55:44, reed1 wrote:
> return const SkBitmap* ?

Done.

[reed1, 2012-06-13 17:43:54]

LGTM