Enable texture readback support for Android

Enable texture readback support for Android

- Adjust the GLHelper to include methods we can use for our texture readback support.
- Tie it into the RenderWidgetHostViewAndroid/ContentViewCore/Compositor.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163455

[David Trainor- moved to gerrit, 2012-10-19 20:44:33]

Hey guys.  Added another GL-related API we need to grab snapshots.  Thanks!

[jam, 2012-10-19 21:14:09]

On 2012/10/19 20:44:33, dtrainor wrote:
> Hey guys.  Added another GL-related API we need to grab snapshots.  Thanks!

since you have multiple reviewers, please tell us which parts you want us to
review

[piman, 2012-10-19 22:10:10]

nits, otherwise LGTM for content/ stuff. I haven't reviewed any of the java
bits.

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:242: (unsigned char*)
bitmap.pixels());
nit: static_cast or reinterpret_cast as appropriate, but no c-style casts.

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_android.cc:169: (unsigned
char*) bitmap.pixels());
nit: static_cast or reinterpret_cast

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
File content/common/gpu/client/gl_helper.h (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
content/common/gpu/client/gl_helper.h:46: void
SyncCopyRawTextureTo(WebKit::WebGLId texture,
I'm not sure what you mean by Raw.
This looks like CopyTextureTo except without cropping or scaling, and
synchronous.
Maybe rename CopyTextureTo into CropScaleAndReadbackTexture, whereas this one
would be ReadbackTextureSync, so that there's no confusion?

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
content/common/gpu/client/gl_helper.h:56: WebKit::WebGLId
CopyTexture(WebKit::WebGLId texture,
No overloading please. CopyAndScaleTexture?

[David Trainor- moved to gerrit, 2012-10-19 22:33:54]

jam@: Sorry for the confusion!  Mainly the content/public bits to make sure
you're okay with the interface changes.
piman@: The GL stuff (basically all other native?).  Looks like you did it
already thanks!
mazda@: Seemed like you owned the GLHelper, wanted to keep you in the loop/get
your LGTM on those changes.
sievers@: Anything Java

Thanks!

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
File content/common/gpu/client/gl_helper.h (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
content/common/gpu/client/gl_helper.h:46: void
SyncCopyRawTextureTo(WebKit::WebGLId texture,
So CopyTextureTo seems to also potentially swizzle R and B/vertically flip the
image (inside WebGraphicsContext3D::readbackFramebuffer).  This version performs
no post operations on the texture it reads back for performance reasons.  It
uses readPixels instead of the utility method readbackFramebuffer.  I like the
name changes though.  I could go with CopyScaleReadbackAndCleanTexture
(mouthful, but accurate) and ReadbackTextureSync?

On 2012/10/19 22:10:10, piman wrote:
> I'm not sure what you mean by Raw.
> This looks like CopyTextureTo except without cropping or scaling, and
> synchronous.
> Maybe rename CopyTextureTo into CropScaleAndReadbackTexture, whereas this one
> would be ReadbackTextureSync, so that there's no confusion?

[David Trainor- moved to gerrit, 2012-10-19 23:11:26]

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:242: (unsigned char*)
bitmap.pixels());
On 2012/10/19 22:10:10, piman wrote:
> nit: static_cast or reinterpret_cast as appropriate, but no c-style casts.

Done.

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_android.cc:169: (unsigned
char*) bitmap.pixels());
On 2012/10/19 22:10:10, piman wrote:
> nit: static_cast or reinterpret_cast

Done.

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
File content/common/gpu/client/gl_helper.h (right):

https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/...
content/common/gpu/client/gl_helper.h:56: WebKit::WebGLId
CopyTexture(WebKit::WebGLId texture,
On 2012/10/19 22:10:10, piman wrote:
> No overloading please. CopyAndScaleTexture?

Done.

[piman, 2012-10-20 00:00:24]

On Fri, Oct 19, 2012 at 3:33 PM, <dtrainor@chromium.org> wrote:

> jam@: Sorry for the confusion!  Mainly the content/public bits to make
> sure
> you're okay with the interface changes.
> piman@: The GL stuff (basically all other native?).  Looks like you did it
> already thanks!
> mazda@: Seemed like you owned the GLHelper, wanted to keep you in the
> loop/get
> your LGTM on those changes.
> sievers@: Anything Java
>
> Thanks!
>
>
>
> https://chromiumcodereview.**appspot.com/11234008/diff/**
>
1001/content/common/gpu/**client/gl_helper.h<https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/client/gl_helper.h>
> File content/common/gpu/client/gl_**helper.h (right):
>
> https://chromiumcodereview.**appspot.com/11234008/diff/**
>
1001/content/common/gpu/**client/gl_helper.h#newcode46<https://chromiumcodereview.appspot.com/11234008/diff/1001/content/common/gpu/client/gl_helper.h#newcode46>
> content/common/gpu/client/gl_**helper.h:46: void
> SyncCopyRawTextureTo(WebKit::**WebGLId texture,
> So CopyTextureTo seems to also potentially swizzle R and B/vertically
> flip the image (inside WebGraphicsContext3D::**readbackFramebuffer).  This
> version performs no post operations on the texture it reads back for
> performance reasons.  It uses readPixels instead of the utility method
> readbackFramebuffer.  I like the name changes though.  I could go with
> CopyScaleReadbackAndCleanTextu**re (mouthful, but accurate) and
> ReadbackTextureSync?


Sounds good.


>
>
> On 2012/10/19 22:10:10, piman wrote:
>
>> I'm not sure what you mean by Raw.
>> This looks like CopyTextureTo except without cropping or scaling, and
>> synchronous.
>> Maybe rename CopyTextureTo into CropScaleAndReadbackTexture, whereas
>>
> this one
>
>> would be ReadbackTextureSync, so that there's no confusion?
>>
>
>
https://chromiumcodereview.**appspot.com/11234008/<https://chromiumcodereview...
>

[no sievers, 2012-10-20 00:42:36]

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/and...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/and...
content/browser/android/content_view_core_impl.cc:449: if
(!CompositorImpl::IsInitialized())
Can you move this check to RWHVA? Same below.

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_android.cc:27: #include
"third_party/WebKit/Source/Platform/chromium/public/WebGraphicsContext3D.h"
already included in header

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/public/brow...
File content/public/browser/android/compositor.h (right):

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/public/brow...
content/public/browser/android/compositor.h:76: gfx::JavaBitmap& bitmap) = 0;
Do you really need to add this generic interface?
Isn't the one in ContentViewCore enough?

[mazda, 2012-10-20 00:57:42]

gl_helper lgtm

[David Trainor- moved to gerrit, 2012-10-22 08:31:07]

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/public/brow...
File content/public/browser/android/compositor.h (right):

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/public/brow...
content/public/browser/android/compositor.h:76: gfx::JavaBitmap& bitmap) = 0;
For ContentViewCore#getBitmap it is, but for some of the other delayed capture
work the RWHV might be destroyed, in which case I need a way to grab the bitmap
from a texture.  ContentViewCore has a method to copy and scale the RWHV's
texture to a new one, which I can readback here even if the original one is
gone.

On 2012/10/20 00:42:36, Daniel Sievers wrote:
> Do you really need to add this generic interface?
> Isn't the one in ContentViewCore enough?

[jam, 2012-10-22 16:22:53]

On 2012/10/19 22:33:54, dtrainor wrote:
> jam@: Sorry for the confusion!  Mainly the content/public bits to make sure
> you're okay with the interface changes.

lgtm (just looked at the C++ files)

[no sievers, 2012-10-22 17:39:15]

Might it still make sense to add that other API to ContentViewCore also (instead
of Compositor), and then have that access CompositorImpl instead?

On 2012/10/22 08:31:07, dtrainor wrote:
>
https://chromiumcodereview.appspot.com/11234008/diff/8001/content/public/brow...
> File content/public/browser/android/compositor.h (right):
> 
>
https://chromiumcodereview.appspot.com/11234008/diff/8001/content/public/brow...
> content/public/browser/android/compositor.h:76: gfx::JavaBitmap& bitmap) = 0;
> For ContentViewCore#getBitmap it is, but for some of the other delayed capture
> work the RWHV might be destroyed, in which case I need a way to grab the
bitmap
> from a texture.  ContentViewCore has a method to copy and scale the RWHV's
> texture to a new one, which I can readback here even if the original one is
> gone.
> 
> On 2012/10/20 00:42:36, Daniel Sievers wrote:
> > Do you really need to add this generic interface?
> > Isn't the one in ContentViewCore enough?

[David Trainor- moved to gerrit, 2012-10-22 17:56:16]

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/and...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/and...
content/browser/android/content_view_core_impl.cc:449: if
(!CompositorImpl::IsInitialized())
On 2012/10/20 00:42:36, Daniel Sievers wrote:
> Can you move this check to RWHVA? Same below.

Done.

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_android.cc:27: #include
"third_party/WebKit/Source/Platform/chromium/public/WebGraphicsContext3D.h"
On 2012/10/20 00:42:36, Daniel Sievers wrote:
> already included in header

Done.

[no sievers, 2012-10-22 18:03:26]

On 2012/10/22 17:56:16, dtrainor wrote:
>
https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/and...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/and...
> content/browser/android/content_view_core_impl.cc:449: if
> (!CompositorImpl::IsInitialized())
> On 2012/10/20 00:42:36, Daniel Sievers wrote:
> > Can you move this check to RWHVA? Same below.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/ren...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11234008/diff/8001/content/browser/ren...
> content/browser/renderer_host/render_widget_host_view_android.cc:27: #include
> "third_party/WebKit/Source/Platform/chromium/public/WebGraphicsContext3D.h"
> On 2012/10/20 00:42:36, Daniel Sievers wrote:
> > already included in header
> 
> Done.

lgtm

[commit-bot: I haz the power, 2012-10-22 18:04:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/11234008/6005

[commit-bot: I haz the power, 2012-10-22 21:49:47]

Failed to apply patch for
content/browser/renderer_host/image_transport_factory_android.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/image_transport_factory_android.h
  Hunk #1 FAILED at 8.
  Hunk #2 succeeded at 23 (offset -1 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/renderer_host/image_transport_factory_android.h.rej

Patch:       content/browser/renderer_host/image_transport_factory_android.h
Index: content/browser/renderer_host/image_transport_factory_android.h
diff --git a/content/browser/renderer_host/image_transport_factory_android.h
b/content/browser/renderer_host/image_transport_factory_android.h
index
42855c7346d5007ec2962a3474162a7654bb0008..f4b7a6264d74415ed16b336b1dbde7b1f9c12cb8
100644
--- a/content/browser/renderer_host/image_transport_factory_android.h
+++ b/content/browser/renderer_host/image_transport_factory_android.h
@@ -8,10 +8,16 @@
 #include "base/memory/scoped_ptr.h"
 #include "ui/gfx/native_widget_types.h"
 
+namespace WebKit {
+  class WebGraphicsContext3D;
+}
+
 class WebGraphicsContext3DCommandBufferImpl;
 
 namespace content {
 
+class GLHelper;
+
 class ImageTransportFactoryAndroid {
  public:
   ImageTransportFactoryAndroid();
@@ -24,9 +30,12 @@ class ImageTransportFactoryAndroid {
 
   uint32_t InsertSyncPoint();
 
-  WebGraphicsContext3DCommandBufferImpl* GetContext3D();
+  WebKit::WebGraphicsContext3D* GetContext3D();
+  GLHelper* GetGLHelper();
+
  private:
   scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context_;
+  scoped_ptr<GLHelper> gl_helper_;
 };
 
 }  // namespace content

[commit-bot: I haz the power, 2012-10-22 22:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/11234008/2019

[commit-bot: I haz the power, 2012-10-23 00:27:22]

Change committed as 163455