[Android] Implement asynchronous zero-copy bitmap capture API.

[Android] Implement asynchronous zero-copy bitmap capture API.

This introduces zero-copy bitmap capture API for Android. Using this, the
caller avoids a duplicate copy of the bitmap being created. This saves us
about ~8MB of memory (while using ARGB_8888 for 1920x1080 resolution) or
about ~4MB of memory (while using RGB565 for 1920x1080 resolution). 

This also significantly saves the power on variety of android devices using KitKat.

[vivekg, 2014-02-26 01:34:55]

PTAL, thank you!

[vivekg, 2014-02-26 01:44:45]

https://codereview.chromium.org/176943004/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/176943004/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:1531: //SkBitmap::Config
bitmap_config = gfx::ConvertToSkiaConfig(jbitmap_config);
This would be enabled once https://codereview.chromium.org/157033007/ lands.

[no sievers, 2014-02-26 02:17:44]

Where does it avoid a copy? I don't think it avoids a copy in any of the code
here.
Does it avoid a copy in the app when you want to use a JavaBitmap? If so, why
don't you avoid using Java bitmaps instead?
I think they also incur a memset.

[vivekg, 2014-02-26 02:21:37]

On 2014/02/26 02:17:44, sievers wrote:
> Where does it avoid a copy? I don't think it avoids a copy in any of the code
> here.
> Does it avoid a copy in the app when you want to use a JavaBitmap? If so, why
> don't you avoid using Java bitmaps instead?
> I think they also incur a memset.

The copy is avoided in RenderWidgetHostViewAndroid::GetScaledContentBitmap. When
the BitmapAllocator is sent, it will be invoked to create a proxy bitmap which
just wraps the pixels of JavaBitmap.

This is then just passed to
RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where there is a
check before the allocPixels is invoked. In doing so we avoid the copy.

[no sievers, 2014-02-26 03:19:20]

On 2014/02/26 02:21:37, vivekg_ wrote:
> On 2014/02/26 02:17:44, sievers wrote:
> > Where does it avoid a copy? I don't think it avoids a copy in any of the
code
> > here.
> > Does it avoid a copy in the app when you want to use a JavaBitmap? If so,
why
> > don't you avoid using Java bitmaps instead?
> > I think they also incur a memset.
> 
> The copy is avoided in RenderWidgetHostViewAndroid::GetScaledContentBitmap.
When
> the BitmapAllocator is sent, it will be invoked to create a proxy bitmap which
> just wraps the pixels of JavaBitmap.
> 
> This is then just passed to
> RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where there is a
> check before the allocPixels is invoked. In doing so we avoid the copy.

You are suggesting that an application saves a copy when it uses a Java bitmap
because you cannot create a Java bitmap from an existing native bitmap, correct?
But I don't see a copy saved in the code being touched here.

[no sievers, 2014-02-26 03:20:37]

> This also significantly saves the power on variety of android devices using
KitKat. 

Do you have any data for that?
Readbacks should not really be done very often.

[vivekg, 2014-02-26 03:51:03]

On 2014/02/26 03:19:20, sievers wrote:
> On 2014/02/26 02:21:37, vivekg_ wrote:
> > On 2014/02/26 02:17:44, sievers wrote:
> > > Where does it avoid a copy? I don't think it avoids a copy in any of the
> code
> > > here.
> > > Does it avoid a copy in the app when you want to use a JavaBitmap? If so,
> why
> > > don't you avoid using Java bitmaps instead?
> > > I think they also incur a memset.
> > 
> > The copy is avoided in RenderWidgetHostViewAndroid::GetScaledContentBitmap.
> When
> > the BitmapAllocator is sent, it will be invoked to create a proxy bitmap
which
> > just wraps the pixels of JavaBitmap.
> > 
> > This is then just passed to
> > RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where there is a
> > check before the allocPixels is invoked. In doing so we avoid the copy.
> 
> You are suggesting that an application saves a copy when it uses a Java bitmap
> because you cannot create a Java bitmap from an existing native bitmap,
correct?
> But I don't see a copy saved in the code being touched here.

Not really... What I mean to do in this patch is send some pre-allocated
SkBitmap to CopyFromCompositingSurface. (May be
CopyFromCompositingSurfaceToBitmap would be better and analogous name to
CopyFromCompositingSurfaceToVideoFrame.) 

In doing so, we let the helper know that it can work on already allocated bitmap
and need not to allocate any extra buffers.
This is done with the help of BitmapAllocator callback. The API
GetScaledContentBitmap is now made to accept this Callback but its an optional
param. So the call site can decide if it wants to do some optimizations to the
bitmap before the call is being made to readback.

The callback ContentViewCoreImpl::ProxyBitmapAllocator is sent to the
GetScaledContentBitmap. This function basically creates a
android.graphics.Bitmap object and keeps a ScopedJavaGlobalRef to this newly
created object. Now that we have created the bitmap in the java world, using
gfx::JavaBitmap we do lock the pixels and these pixels are wrapped in the
SkBitmap using SkBitmap::setConfig() and SkBitmap::setPixels() APIs. 

Now as this scoped_ptr<SkBitmap> is passed around, finally it reaches
PrepareTextureCopyOutputResult and there is a check for pre-allocation. If its
already containing a valid address, we just pass those pixels to the underlying
GLHelper call. 

So now when the result callback [void(bool, const SkBitmap&)] comes back, we
unlock the pixels held by JavaBitmap and then release the java bitmap to the
onBitmapReady java API for further handling of the filled bitmap.

Thoughts?

[no sievers, 2014-02-26 17:31:38]

On 2014/02/26 03:51:03, vivekg_ wrote:
> On 2014/02/26 03:19:20, sievers wrote:
> > On 2014/02/26 02:21:37, vivekg_ wrote:
> > > On 2014/02/26 02:17:44, sievers wrote:
> > > > Where does it avoid a copy? I don't think it avoids a copy in any of the
> > code
> > > > here.
> > > > Does it avoid a copy in the app when you want to use a JavaBitmap? If
so,
> > why
> > > > don't you avoid using Java bitmaps instead?
> > > > I think they also incur a memset.
> > > 
> > > The copy is avoided in
RenderWidgetHostViewAndroid::GetScaledContentBitmap.
> > When
> > > the BitmapAllocator is sent, it will be invoked to create a proxy bitmap
> which
> > > just wraps the pixels of JavaBitmap.
> > > 
> > > This is then just passed to
> > > RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where there is
a
> > > check before the allocPixels is invoked. In doing so we avoid the copy.
> > 
> > You are suggesting that an application saves a copy when it uses a Java
bitmap
> > because you cannot create a Java bitmap from an existing native bitmap,
> correct?
> > But I don't see a copy saved in the code being touched here.
> 
> Not really... What I mean to do in this patch is send some pre-allocated
> SkBitmap to CopyFromCompositingSurface. (May be
> CopyFromCompositingSurfaceToBitmap would be better and analogous name to
> CopyFromCompositingSurfaceToVideoFrame.) 
> 
> In doing so, we let the helper know that it can work on already allocated
bitmap
> and need not to allocate any extra buffers.
> This is done with the help of BitmapAllocator callback. The API
> GetScaledContentBitmap is now made to accept this Callback but its an optional
> param. So the call site can decide if it wants to do some optimizations to the
> bitmap before the call is being made to readback.
> 
> The callback ContentViewCoreImpl::ProxyBitmapAllocator is sent to the
> GetScaledContentBitmap. This function basically creates a
> android.graphics.Bitmap object and keeps a ScopedJavaGlobalRef to this newly
> created object. Now that we have created the bitmap in the java world, using
> gfx::JavaBitmap we do lock the pixels and these pixels are wrapped in the
> SkBitmap using SkBitmap::setConfig() and SkBitmap::setPixels() APIs. 
> 
> Now as this scoped_ptr<SkBitmap> is passed around, finally it reaches
> PrepareTextureCopyOutputResult and there is a check for pre-allocation. If its
> already containing a valid address, we just pass those pixels to the
underlying
> GLHelper call. 
> 
> So now when the result callback [void(bool, const SkBitmap&)] comes back, we
> unlock the pixels held by JavaBitmap and then release the java bitmap to the
> onBitmapReady java API for further handling of the filled bitmap.
> 
> Thoughts?

I still don't see any mention of copy in your explanation.
Your patch adds a path where the caller can provide the memory instead of the
callee allocating it.
Are you trying to recycle bitmaps across readbacks?

[vivekg, 2014-02-26 17:48:00]

On 2014/02/26 17:31:38, sievers wrote:
> On 2014/02/26 03:51:03, vivekg_ wrote:
> > On 2014/02/26 03:19:20, sievers wrote:
> > > On 2014/02/26 02:21:37, vivekg_ wrote:
> > > > On 2014/02/26 02:17:44, sievers wrote:
> > > > > Where does it avoid a copy? I don't think it avoids a copy in any of
the
> > > code
> > > > > here.
> > > > > Does it avoid a copy in the app when you want to use a JavaBitmap? If
> so,
> > > why
> > > > > don't you avoid using Java bitmaps instead?
> > > > > I think they also incur a memset.
> > > > 
> > > > The copy is avoided in
> RenderWidgetHostViewAndroid::GetScaledContentBitmap.
> > > When
> > > > the BitmapAllocator is sent, it will be invoked to create a proxy bitmap
> > which
> > > > just wraps the pixels of JavaBitmap.
> > > > 
> > > > This is then just passed to
> > > > RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where there
is
> a
> > > > check before the allocPixels is invoked. In doing so we avoid the copy.
> > > 
> > > You are suggesting that an application saves a copy when it uses a Java
> bitmap
> > > because you cannot create a Java bitmap from an existing native bitmap,
> > correct?
> > > But I don't see a copy saved in the code being touched here.
> > 
> > Not really... What I mean to do in this patch is send some pre-allocated
> > SkBitmap to CopyFromCompositingSurface. (May be
> > CopyFromCompositingSurfaceToBitmap would be better and analogous name to
> > CopyFromCompositingSurfaceToVideoFrame.) 
> > 
> > In doing so, we let the helper know that it can work on already allocated
> bitmap
> > and need not to allocate any extra buffers.
> > This is done with the help of BitmapAllocator callback. The API
> > GetScaledContentBitmap is now made to accept this Callback but its an
optional
> > param. So the call site can decide if it wants to do some optimizations to
the
> > bitmap before the call is being made to readback.
> > 
> > The callback ContentViewCoreImpl::ProxyBitmapAllocator is sent to the
> > GetScaledContentBitmap. This function basically creates a
> > android.graphics.Bitmap object and keeps a ScopedJavaGlobalRef to this newly
> > created object. Now that we have created the bitmap in the java world, using
> > gfx::JavaBitmap we do lock the pixels and these pixels are wrapped in the
> > SkBitmap using SkBitmap::setConfig() and SkBitmap::setPixels() APIs. 
> > 
> > Now as this scoped_ptr<SkBitmap> is passed around, finally it reaches
> > PrepareTextureCopyOutputResult and there is a check for pre-allocation. If
its
> > already containing a valid address, we just pass those pixels to the
> underlying
> > GLHelper call. 
> > 
> > So now when the result callback [void(bool, const SkBitmap&)] comes back, we
> > unlock the pixels held by JavaBitmap and then release the java bitmap to the
> > onBitmapReady java API for further handling of the filled bitmap.
> > 
> > Thoughts?
> 
> I still don't see any mention of copy in your explanation.
> Your patch adds a path where the caller can provide the memory instead of the
> callee allocating it.
> Are you trying to recycle bitmaps across readbacks?

No, we are not recycling the bitmaps. 

With the current callback, which provides SkBitmap, we need to convert it to
java Bitmap before making a call to java world using the API [1]. This is where
the copy is created with pixels being memcopied [2]. 

With the patch, we need not worry about any separate pixel buffer and hence we
avoid the copy.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/android/jav...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/android/jav...

[no sievers, 2014-02-26 17:58:58]

On 2014/02/26 17:48:00, vivekg_ wrote:
> On 2014/02/26 17:31:38, sievers wrote:
> > On 2014/02/26 03:51:03, vivekg_ wrote:
> > > On 2014/02/26 03:19:20, sievers wrote:
> > > > On 2014/02/26 02:21:37, vivekg_ wrote:
> > > > > On 2014/02/26 02:17:44, sievers wrote:
> > > > > > Where does it avoid a copy? I don't think it avoids a copy in any of
> the
> > > > code
> > > > > > here.
> > > > > > Does it avoid a copy in the app when you want to use a JavaBitmap?
If
> > so,
> > > > why
> > > > > > don't you avoid using Java bitmaps instead?
> > > > > > I think they also incur a memset.
> > > > > 
> > > > > The copy is avoided in
> > RenderWidgetHostViewAndroid::GetScaledContentBitmap.
> > > > When
> > > > > the BitmapAllocator is sent, it will be invoked to create a proxy
bitmap
> > > which
> > > > > just wraps the pixels of JavaBitmap.
> > > > > 
> > > > > This is then just passed to
> > > > > RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where
there
> is
> > a
> > > > > check before the allocPixels is invoked. In doing so we avoid the
copy.
> > > > 
> > > > You are suggesting that an application saves a copy when it uses a Java
> > bitmap
> > > > because you cannot create a Java bitmap from an existing native bitmap,
> > > correct?
> > > > But I don't see a copy saved in the code being touched here.
> > > 
> > > Not really... What I mean to do in this patch is send some pre-allocated
> > > SkBitmap to CopyFromCompositingSurface. (May be
> > > CopyFromCompositingSurfaceToBitmap would be better and analogous name to
> > > CopyFromCompositingSurfaceToVideoFrame.) 
> > > 
> > > In doing so, we let the helper know that it can work on already allocated
> > bitmap
> > > and need not to allocate any extra buffers.
> > > This is done with the help of BitmapAllocator callback. The API
> > > GetScaledContentBitmap is now made to accept this Callback but its an
> optional
> > > param. So the call site can decide if it wants to do some optimizations to
> the
> > > bitmap before the call is being made to readback.
> > > 
> > > The callback ContentViewCoreImpl::ProxyBitmapAllocator is sent to the
> > > GetScaledContentBitmap. This function basically creates a
> > > android.graphics.Bitmap object and keeps a ScopedJavaGlobalRef to this
newly
> > > created object. Now that we have created the bitmap in the java world,
using
> > > gfx::JavaBitmap we do lock the pixels and these pixels are wrapped in the
> > > SkBitmap using SkBitmap::setConfig() and SkBitmap::setPixels() APIs. 
> > > 
> > > Now as this scoped_ptr<SkBitmap> is passed around, finally it reaches
> > > PrepareTextureCopyOutputResult and there is a check for pre-allocation. If
> its
> > > already containing a valid address, we just pass those pixels to the
> > underlying
> > > GLHelper call. 
> > > 
> > > So now when the result callback [void(bool, const SkBitmap&)] comes back,
we
> > > unlock the pixels held by JavaBitmap and then release the java bitmap to
the
> > > onBitmapReady java API for further handling of the filled bitmap.
> > > 
> > > Thoughts?
> > 
> > I still don't see any mention of copy in your explanation.
> > Your patch adds a path where the caller can provide the memory instead of
the
> > callee allocating it.
> > Are you trying to recycle bitmaps across readbacks?
> 
> No, we are not recycling the bitmaps. 
> 
> With the current callback, which provides SkBitmap, we need to convert it to
> java Bitmap before making a call to java world using the API [1]. This is
where
> the copy is created with pixels being memcopied [2]. 
> 
> With the patch, we need not worry about any separate pixel buffer and hence we
> avoid the copy.
> 

Ok that's what I thought. I suggest that instead of making the API more complex,
it's better to avoid using Java Bitmaps. That's the direction we are moving
into, since we can really just deal with native SkBitmaps entirely for most of
what we are doing with the readback data.

If there are cases where you absolutely need a Java Bitmap, you could still
create that from an SkBitmap on another thread without janking the UI thread (in
case the bitmap is very big and takes long to copy). I don't really think it
needs to be exposed here.

The problem with exposing it is that it makes the API more complex. Basically
you are adding a second code path, which is neither covered by any existing
tests, nor is it implemented on any platform other than Android with your patch
(all other platforms silently ignore the argument).

It also restricts us in that if we change the underlying implementation, we now
have to support a bitmap being preallocated and passed in. For example, if we
were to move more of the functionality from GLHelper into cc, we'd now have to
plumb through all of this just to be compatible with this. I don't think it's
really worth it if the problem like here is very application-specific and can be
easier avoided in the first place.

Does that make any sense?

> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/android/jav...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/android/jav...

[vivekg, 2014-02-26 18:18:58]

On 2014/02/26 17:58:58, sievers wrote:
> On 2014/02/26 17:48:00, vivekg_ wrote:
> > On 2014/02/26 17:31:38, sievers wrote:
> > > On 2014/02/26 03:51:03, vivekg_ wrote:
> > > > On 2014/02/26 03:19:20, sievers wrote:
> > > > > On 2014/02/26 02:21:37, vivekg_ wrote:
> > > > > > On 2014/02/26 02:17:44, sievers wrote:
> > > > > > > Where does it avoid a copy? I don't think it avoids a copy in any
of
> > the
> > > > > code
> > > > > > > here.
> > > > > > > Does it avoid a copy in the app when you want to use a JavaBitmap?
> If
> > > so,
> > > > > why
> > > > > > > don't you avoid using Java bitmaps instead?
> > > > > > > I think they also incur a memset.
> > > > > > 
> > > > > > The copy is avoided in
> > > RenderWidgetHostViewAndroid::GetScaledContentBitmap.
> > > > > When
> > > > > > the BitmapAllocator is sent, it will be invoked to create a proxy
> bitmap
> > > > which
> > > > > > just wraps the pixels of JavaBitmap.
> > > > > > 
> > > > > > This is then just passed to
> > > > > > RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where
> there
> > is
> > > a
> > > > > > check before the allocPixels is invoked. In doing so we avoid the
> copy.
> > > > > 
> > > > > You are suggesting that an application saves a copy when it uses a
Java
> > > bitmap
> > > > > because you cannot create a Java bitmap from an existing native
bitmap,
> > > > correct?
> > > > > But I don't see a copy saved in the code being touched here.
> > > > 
> > > > Not really... What I mean to do in this patch is send some pre-allocated
> > > > SkBitmap to CopyFromCompositingSurface. (May be
> > > > CopyFromCompositingSurfaceToBitmap would be better and analogous name to
> > > > CopyFromCompositingSurfaceToVideoFrame.) 
> > > > 
> > > > In doing so, we let the helper know that it can work on already
allocated
> > > bitmap
> > > > and need not to allocate any extra buffers.
> > > > This is done with the help of BitmapAllocator callback. The API
> > > > GetScaledContentBitmap is now made to accept this Callback but its an
> > optional
> > > > param. So the call site can decide if it wants to do some optimizations
to
> > the
> > > > bitmap before the call is being made to readback.
> > > > 
> > > > The callback ContentViewCoreImpl::ProxyBitmapAllocator is sent to the
> > > > GetScaledContentBitmap. This function basically creates a
> > > > android.graphics.Bitmap object and keeps a ScopedJavaGlobalRef to this
> newly
> > > > created object. Now that we have created the bitmap in the java world,
> using
> > > > gfx::JavaBitmap we do lock the pixels and these pixels are wrapped in
the
> > > > SkBitmap using SkBitmap::setConfig() and SkBitmap::setPixels() APIs. 
> > > > 
> > > > Now as this scoped_ptr<SkBitmap> is passed around, finally it reaches
> > > > PrepareTextureCopyOutputResult and there is a check for pre-allocation.
If
> > its
> > > > already containing a valid address, we just pass those pixels to the
> > > underlying
> > > > GLHelper call. 
> > > > 
> > > > So now when the result callback [void(bool, const SkBitmap&)] comes
back,
> we
> > > > unlock the pixels held by JavaBitmap and then release the java bitmap to
> the
> > > > onBitmapReady java API for further handling of the filled bitmap.
> > > > 
> > > > Thoughts?
> > > 
> > > I still don't see any mention of copy in your explanation.
> > > Your patch adds a path where the caller can provide the memory instead of
> the
> > > callee allocating it.
> > > Are you trying to recycle bitmaps across readbacks?
> > 
> > No, we are not recycling the bitmaps. 
> > 
> > With the current callback, which provides SkBitmap, we need to convert it to
> > java Bitmap before making a call to java world using the API [1]. This is
> where
> > the copy is created with pixels being memcopied [2]. 
> > 
> > With the patch, we need not worry about any separate pixel buffer and hence
we
> > avoid the copy.
> > 
> 
> Ok that's what I thought. I suggest that instead of making the API more
complex,
> it's better to avoid using Java Bitmaps. That's the direction we are moving
> into, since we can really just deal with native SkBitmaps entirely for most of
> what we are doing with the readback data.
> 
> If there are cases where you absolutely need a Java Bitmap, you could still
> create that from an SkBitmap on another thread without janking the UI thread
(in
> case the bitmap is very big and takes long to copy). I don't really think it
> needs to be exposed here.
> 
> The problem with exposing it is that it makes the API more complex. Basically
> you are adding a second code path, which is neither covered by any existing
> tests, nor is it implemented on any platform other than Android with your
patch
> (all other platforms silently ignore the argument).

I mostly agree with the concerns about the API complexity being added. While
going thr' this, I also encountered one place here [1] where we could recycle
the bitmap with the proposed patch. As can be seen, the Dev tools gets the pixel
data and converts it into base64 encoded data and sends this to the frontend.
This essentially makes the bitmap to be recycled instead of creating new one
every time. This is again done for android though.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...

[no sievers, 2014-02-26 19:37:55]

On 2014/02/26 18:18:58, vivekg_ wrote:
> On 2014/02/26 17:58:58, sievers wrote:
> > On 2014/02/26 17:48:00, vivekg_ wrote:
> > > On 2014/02/26 17:31:38, sievers wrote:
> > > > On 2014/02/26 03:51:03, vivekg_ wrote:
> > > > > On 2014/02/26 03:19:20, sievers wrote:
> > > > > > On 2014/02/26 02:21:37, vivekg_ wrote:
> > > > > > > On 2014/02/26 02:17:44, sievers wrote:
> > > > > > > > Where does it avoid a copy? I don't think it avoids a copy in
any
> of
> > > the
> > > > > > code
> > > > > > > > here.
> > > > > > > > Does it avoid a copy in the app when you want to use a
JavaBitmap?
> > If
> > > > so,
> > > > > > why
> > > > > > > > don't you avoid using Java bitmaps instead?
> > > > > > > > I think they also incur a memset.
> > > > > > > 
> > > > > > > The copy is avoided in
> > > > RenderWidgetHostViewAndroid::GetScaledContentBitmap.
> > > > > > When
> > > > > > > the BitmapAllocator is sent, it will be invoked to create a proxy
> > bitmap
> > > > > which
> > > > > > > just wraps the pixels of JavaBitmap.
> > > > > > > 
> > > > > > > This is then just passed to
> > > > > > > RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResult where
> > there
> > > is
> > > > a
> > > > > > > check before the allocPixels is invoked. In doing so we avoid the
> > copy.
> > > > > > 
> > > > > > You are suggesting that an application saves a copy when it uses a
> Java
> > > > bitmap
> > > > > > because you cannot create a Java bitmap from an existing native
> bitmap,
> > > > > correct?
> > > > > > But I don't see a copy saved in the code being touched here.
> > > > > 
> > > > > Not really... What I mean to do in this patch is send some
pre-allocated
> > > > > SkBitmap to CopyFromCompositingSurface. (May be
> > > > > CopyFromCompositingSurfaceToBitmap would be better and analogous name
to
> > > > > CopyFromCompositingSurfaceToVideoFrame.) 
> > > > > 
> > > > > In doing so, we let the helper know that it can work on already
> allocated
> > > > bitmap
> > > > > and need not to allocate any extra buffers.
> > > > > This is done with the help of BitmapAllocator callback. The API
> > > > > GetScaledContentBitmap is now made to accept this Callback but its an
> > > optional
> > > > > param. So the call site can decide if it wants to do some
optimizations
> to
> > > the
> > > > > bitmap before the call is being made to readback.
> > > > > 
> > > > > The callback ContentViewCoreImpl::ProxyBitmapAllocator is sent to the
> > > > > GetScaledContentBitmap. This function basically creates a
> > > > > android.graphics.Bitmap object and keeps a ScopedJavaGlobalRef to this
> > newly
> > > > > created object. Now that we have created the bitmap in the java world,
> > using
> > > > > gfx::JavaBitmap we do lock the pixels and these pixels are wrapped in
> the
> > > > > SkBitmap using SkBitmap::setConfig() and SkBitmap::setPixels() APIs. 
> > > > > 
> > > > > Now as this scoped_ptr<SkBitmap> is passed around, finally it reaches
> > > > > PrepareTextureCopyOutputResult and there is a check for
pre-allocation.
> If
> > > its
> > > > > already containing a valid address, we just pass those pixels to the
> > > > underlying
> > > > > GLHelper call. 
> > > > > 
> > > > > So now when the result callback [void(bool, const SkBitmap&)] comes
> back,
> > we
> > > > > unlock the pixels held by JavaBitmap and then release the java bitmap
to
> > the
> > > > > onBitmapReady java API for further handling of the filled bitmap.
> > > > > 
> > > > > Thoughts?
> > > > 
> > > > I still don't see any mention of copy in your explanation.
> > > > Your patch adds a path where the caller can provide the memory instead
of
> > the
> > > > callee allocating it.
> > > > Are you trying to recycle bitmaps across readbacks?
> > > 
> > > No, we are not recycling the bitmaps. 
> > > 
> > > With the current callback, which provides SkBitmap, we need to convert it
to
> > > java Bitmap before making a call to java world using the API [1]. This is
> > where
> > > the copy is created with pixels being memcopied [2]. 
> > > 
> > > With the patch, we need not worry about any separate pixel buffer and
hence
> we
> > > avoid the copy.
> > > 
> > 
> > Ok that's what I thought. I suggest that instead of making the API more
> complex,
> > it's better to avoid using Java Bitmaps. That's the direction we are moving
> > into, since we can really just deal with native SkBitmaps entirely for most
of
> > what we are doing with the readback data.
> > 
> > If there are cases where you absolutely need a Java Bitmap, you could still
> > create that from an SkBitmap on another thread without janking the UI thread
> (in
> > case the bitmap is very big and takes long to copy). I don't really think it
> > needs to be exposed here.
> > 
> > The problem with exposing it is that it makes the API more complex.
Basically
> > you are adding a second code path, which is neither covered by any existing
> > tests, nor is it implemented on any platform other than Android with your
> patch
> > (all other platforms silently ignore the argument).
> 
> I mostly agree with the concerns about the API complexity being added. While
> going thr' this, I also encountered one place here [1] where we could recycle
> the bitmap with the proposed patch. As can be seen, the Dev tools gets the
pixel
> data and converts it into base64 encoded data and sends this to the frontend.
> This essentially makes the bitmap to be recycled instead of creating new one
> every time. This is again done for android though.
> 

Ok now you are saying though that you want to save a bitmap malloc per frame
(not copy) in devtools screen capture,
which I doubt is really worth worrying about, esp. given the previous concerns.


> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...

[vivekg, 2014-02-27 04:42:09]

Sure. Thanks for the review.

I am also curious to know where the API GetScaledContentBitmap is being used. As
per [1], I couldn't find any usage at least with the open source code.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=GetScaledContentBitma...

[powei, 2014-02-27 07:22:50]

On 2014/02/27 04:42:09, vivekg_ wrote:
> Sure. Thanks for the review.
> 
> I am also curious to know where the API GetScaledContentBitmap is being used.
As
> per [1], I couldn't find any usage at least with the open source code.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=GetScaledContentBitma...

GetScaledContentBitmap is used in our Chrome for Android code base.  However, I
think we will be removing the method soon and opt for a single copy path through
RenderWidgetHost::CopyFromBackingStore, which call CopyFromCompositingSurface.

[piman, 2014-03-05 21:07:22]

https://codereview.chromium.org/176943004/diff/1/content/port/browser/render_...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/176943004/diff/1/content/port/browser/render_...
content/port/browser/render_widget_host_view_port.h:186: scoped_ptr<SkBitmap>
bitmap = scoped_ptr<SkBitmap>()) = 0;
nit: no default argument.

I don't think it's ok that this argument is ignored on all platforms but
Android.

[no sievers, 2014-03-05 21:08:37]

On 2014/03/05 21:07:22, piman wrote:
>
https://codereview.chromium.org/176943004/diff/1/content/port/browser/render_...
> File content/port/browser/render_widget_host_view_port.h (right):
> 
>
https://codereview.chromium.org/176943004/diff/1/content/port/browser/render_...
> content/port/browser/render_widget_host_view_port.h:186: scoped_ptr<SkBitmap>
> bitmap = scoped_ptr<SkBitmap>()) = 0;
> nit: no default argument.
> 
> I don't think it's ok that this argument is ignored on all platforms but
> Android.

Actually, can we close the issue? Given that we'd rather work around the problem
by not using Java bitmaps so much.

[vivekg_samsung, 2014-03-06 00:26:30]

On 2014/03/05 21:08:37, sievers wrote:
> On 2014/03/05 21:07:22, piman wrote:
> >
>
https://codereview.chromium.org/176943004/diff/1/content/port/browser/render_...
> > File content/port/browser/render_widget_host_view_port.h (right):
> > 
> >
>
https://codereview.chromium.org/176943004/diff/1/content/port/browser/render_...
> > content/port/browser/render_widget_host_view_port.h:186:
scoped_ptr<SkBitmap>
> > bitmap = scoped_ptr<SkBitmap>()) = 0;
> > nit: no default argument.
> > 
> > I don't think it's ok that this argument is ignored on all platforms but
> > Android.
> 
> Actually, can we close the issue? Given that we'd rather work around the
problem
> by not using Java bitmaps so much.

Sorry for the confusion, closed the issue. Thank you!