Expose the WebGraphicsContext3D for Android

Expose the WebGraphicsContext3D for Android

- Expose the WebGraphicsContext3D for Android's Compositor.

BUG=


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

[no sievers, 2012-10-11 20:49:38]

On 2012/10/11 20:22:09, dtrainor wrote:

lgtm

[sky, 2012-10-11 21:31:00]

I suspect we don't want to expose WebGraphicsContext3D to the world, but I could
be wrong. Antoine should review this.

[no sievers, 2012-10-11 21:32:19]

This is needed in the interim to upload ETC1 compressed textures for the
inactive tabs in the tab stack. The compositor currently does not have a way to
handle ETC1, so we need to use WebExternalTextureLayer.

[no sievers, 2012-10-11 21:33:13]

I guess the alternative is an uploadCompressedTexture() API.

On 2012/10/11 21:32:19, Daniel Sievers wrote:
> This is needed in the interim to upload ETC1 compressed textures for the
> inactive tabs in the tab stack. The compositor currently does not have a way
to
> handle ETC1, so we need to use WebExternalTextureLayer.

[David Trainor- moved to gerrit, 2012-10-11 21:49:27]

For Android, we're currently relying on WebExternalTextureLayers to do our
switcher thumbnail painting.  As we port to the compositor we're going to
eventually remove the dependency on the WebGraphicsContext3D and rely on the
existing content/image layers in some fashion, but currently they manage the
data lifecycle differently than the rest of our system, which causes memory
pressure and requires us to do a larger refactor.

This is to hit feature parity with our current rendering system as I implement
some of the needed features in the compositor.

[piman, 2012-10-12 16:38:48]

Beware of 2 things:
- if you're ever planning to use the compositor thread in the UI compositor,
this won't work (contexts are bound to a single thread)
- beware of lost contexts

I think it would be better to have the UI push ETC1 data to the lower layers
rather than give it a blank check - as is it's ripe for abuse and hacks imo,
hence shouldn't belong in the content public API.
In particular, mucking around with the currently bound texture at an arbitrary
time may break some assumptions in the compositor.

If all you need is the texCompressedImage2D, how about exposing something like
this on the public API: int CreateETCTexture(gfx::Size dimensions, size_t size,
char* data);

That being said I'm hoping that we can find a better long term solution. File a
bug/reference here?

https://chromiumcodereview.appspot.com/11090075/diff/1/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/11090075/diff/1/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:64:
scoped_ptr<WebKit::WebGraphicsContext3D> m_context3D;
I just realize because of this CL, both this and the
ImageTransportFactoryAndroid own (in the scoped_ptr sense) the context. Isn't it
a problem?

[no sievers, 2012-10-12 16:42:43]

https://chromiumcodereview.appspot.com/11090075/diff/1/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/11090075/diff/1/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:64:
scoped_ptr<WebKit::WebGraphicsContext3D> m_context3D;
Actually they ImageTransportFactory has its own context that lives on the UI
thread. And we share resources between all contexts.

In the longer term I hope we can get rid of this extra context and the need for
it to do ETC1 uploads too.

On 2012/10/12 16:38:48, piman wrote:
> I just realize because of this CL, both this and the
> ImageTransportFactoryAndroid own (in the scoped_ptr sense) the context. Isn't
it
> a problem?

[David Trainor- moved to gerrit, 2012-10-12 17:06:56]

Thanks for the comments!  This is definitely a short term solution.  It's to
help us build upstream Chromium.  Long term this will all be refactored into the
compositor.

I can add the methods you suggested.  To hit feature parity I'll probably need
the following.  I'll add these instead and update the CL:

int CreateETCTexture(...)
int CreateTexture(...)
void DestroyTexture(...)
void ReadbackTextureToSkBitmap(...)

[no sievers, 2012-10-12 22:45:56]

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

https://chromiumcodereview.appspot.com/11090075/diff/8001/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:29: #define
ETC1_RGB8_OES 0x8D64
you should not need this.

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

https://chromiumcodereview.appspot.com/11090075/diff/8001/content/public/brow...
content/public/browser/android/compositor.h:68: // or false if the texture could
not be deleted due to a lost context.
I think it maybe be hard to guarantee without a sync flush that the commands
landed. It might be easier to handle it internally and not burden the caller
with trying to delete it again, i.e. just make this return void instead.

[piman, 2012-10-12 22:49:48]

+jam for content/public
I like this version a little better.

[David Trainor- moved to gerrit, 2012-10-15 19:01:56]

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

https://chromiumcodereview.appspot.com/11090075/diff/8001/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:29: #define
ETC1_RGB8_OES 0x8D64
On 2012/10/12 22:45:56, Daniel Sievers wrote:
> you should not need this.

Done.

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

https://chromiumcodereview.appspot.com/11090075/diff/8001/content/public/brow...
content/public/browser/android/compositor.h:68: // or false if the texture could
not be deleted due to a lost context.
On 2012/10/12 22:45:56, Daniel Sievers wrote:
> I think it maybe be hard to guarantee without a sync flush that the commands
> landed. It might be easier to handle it internally and not burden the caller
> with trying to delete it again, i.e. just make this return void instead.

Done.

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

On 2012/10/12 22:49:48, piman wrote:
> +jam for content/public
> I like this version a little better.

I defer my lgtm to piman for this

[piman, 2012-10-16 16:57:18]

lgtm

[commit-bot: I haz the power, 2012-10-16 17:34:05]

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

[commit-bot: I haz the power, 2012-10-16 17:34:15]

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

Patch:       content/browser/renderer_host/compositor_impl_android.h
Index: content/browser/renderer_host/compositor_impl_android.h
diff --git a/content/browser/renderer_host/compositor_impl_android.h
b/content/browser/renderer_host/compositor_impl_android.h
index
4d6065d9a28fd90867c77c944a0b5fcddad8ffbc..017e0b97a347a87374853a95872fc55644520ca7
100644
--- a/content/browser/renderer_host/compositor_impl_android.h
+++ b/content/browser/renderer_host/compositor_impl_android.h
@@ -13,6 +13,7 @@
 #include
"third_party/WebKit/Source/Platform/chromium/public/WebLayerTreeView.h"
 #include
"third_party/WebKit/Source/Platform/chromium/public/WebLayerTreeViewClient.h"
 
+
 struct ANativeWindow;
 
 namespace content {
@@ -37,6 +38,10 @@ class CompositorImpl : public Compositor,
       void *pixels, const gfx::Rect& rect) OVERRIDE;
   virtual void OnSurfaceUpdated(
       const SurfacePresentedCallback& callback) OVERRIDE;
+  virtual WebKit::WebGLId GenerateTexture(gfx::JavaBitmap& bitmap) OVERRIDE;
+  virtual WebKit::WebGLId GenerateCompressedTexture(
+      gfx::Size& size, int data_size, void* data) OVERRIDE;
+  virtual void DeleteTexture(WebKit::WebGLId texture_id) OVERRIDE;
 
   // WebLayerTreeViewClient implementation.
   virtual void updateAnimations(double frameBeginTime) OVERRIDE;
@@ -51,6 +56,10 @@ class CompositorImpl : public Compositor,
   virtual void scheduleComposite() OVERRIDE;
 
  private:
+  WebKit::WebGLId BuildBasicTexture();
+  WebKit::WGC3Denum GetGLFormatForBitmap(gfx::JavaBitmap& bitmap);
+  WebKit::WGC3Denum GetGLTypeForBitmap(gfx::JavaBitmap& bitmap);
+
   scoped_ptr<WebKit::WebLayer> root_layer_;
   scoped_ptr<WebKit::WebLayerTreeView> host_;

[commit-bot: I haz the power, 2012-10-16 17:50:59]

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

[commit-bot: I haz the power, 2012-10-16 20:31:56]

Change committed as 162231