Add GL surface creation for Android.

Add GL surface creation for Android.
  
This creates either a texture transport surface (for compositor use) or a native window surface.
    
Also add a way to look up a surface handle that cannot be serialized over IPC. We simply get it from the tracker as we are running the GPU thread in the browser process.

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

[no sievers, 2012-07-20 23:52:26]

Is this way too ugly?
I could also fudge the pointer through the IPC channel :)
Or add a proper virtual interface to look it up from the other component?
(although I thought it might not justify the extra code).

[apatrick_chromium, 2012-07-24 18:34:16]

https://chromiumcodereview.appspot.com/10795058/diff/1/content/browser/gpu/gp...
File content/browser/gpu/gpu_surface_tracker.cc (right):

https://chromiumcodereview.appspot.com/10795058/diff/1/content/browser/gpu/gp...
content/browser/gpu/gpu_surface_tracker.cc:114: GpuSurfaceTracker* tracker =
GpuSurfaceTracker::GetInstance();
This member function is not static. Why is it getting "this" through
GetInstance?

https://chromiumcodereview.appspot.com/10795058/diff/1/content/browser/gpu/gp...
content/browser/gpu/gpu_surface_tracker.cc:117: return
tracker->surface_map_[surface_id].native_widget;
How exactly does this differ from the handle returned by GetSurfaceWindowHandle?

https://chromiumcodereview.appspot.com/10795058/diff/1/content/common/gpu/ima...
File content/common/gpu/image_transport_surface_android.cc (right):

https://chromiumcodereview.appspot.com/10795058/diff/1/content/common/gpu/ima...
content/common/gpu/image_transport_surface_android.cc:22:
DCHECK(handle.transport);
DCHECK is redundant.

[no sievers, 2012-07-26 21:21:16]

Removed the code where it touches the refcount - the owner on the UI thread can
take care of this and maybe as part of b/119006 we can do something nicer.

http://codereview.chromium.org/10795058/diff/1/content/browser/gpu/gpu_surfac...
File content/browser/gpu/gpu_surface_tracker.cc (right):

http://codereview.chromium.org/10795058/diff/1/content/browser/gpu/gpu_surfac...
content/browser/gpu/gpu_surface_tracker.cc:114: GpuSurfaceTracker* tracker =
GpuSurfaceTracker::GetInstance();
On 2012/07/24 18:34:16, apatrick_chromium wrote:
> This member function is not static. Why is it getting "this" through
> GetInstance?

Done.

http://codereview.chromium.org/10795058/diff/1/content/browser/gpu/gpu_surfac...
content/browser/gpu/gpu_surface_tracker.cc:117: return
tracker->surface_map_[surface_id].native_widget;
The native AcceleratedWidget on Android is a pointer type.
PluginWindowHandle is a serializable that gets sent in an IPC message. It's
basically unused on Android, since the pointer handle cannot be serialized.

On 2012/07/24 18:34:16, apatrick_chromium wrote:
> How exactly does this differ from the handle returned by
GetSurfaceWindowHandle?

http://codereview.chromium.org/10795058/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_android.cc (right):

http://codereview.chromium.org/10795058/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_android.cc:22:
DCHECK(handle.transport);
On 2012/07/24 18:34:16, apatrick_chromium wrote:
> DCHECK is redundant.

Done.

[piman, 2012-07-26 21:33:28]

http://codereview.chromium.org/10795058/diff/5001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_android.cc (right):

http://codereview.chromium.org/10795058/diff/5001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_android.cc:11: gfx::AcceleratedWidget
GetNativeWidgetAndroid(int surface_id);
mmh, this is just here to workaround the include DEPS isn't it? How can we clean
this up?

[no sievers, 2012-07-26 21:38:59]

Well, I could make a real interface, something like GpuSurfaceLookup.
Then GpuSurfaceTracker could implement it for the browser process.

Android will likely only need it in the browser process, as that's where the gpu
thread for the compositor will run (to be able to share resources). However, the
interface might also be nicer in case there is some test build I'm missing that
might need a stub impl.


On 2012/07/26 21:33:28, piman wrote:
>
http://codereview.chromium.org/10795058/diff/5001/content/common/gpu/image_tr...
> File content/common/gpu/image_transport_surface_android.cc (right):
> 
>
http://codereview.chromium.org/10795058/diff/5001/content/common/gpu/image_tr...
> content/common/gpu/image_transport_surface_android.cc:11:
gfx::AcceleratedWidget
> GetNativeWidgetAndroid(int surface_id);
> mmh, this is just here to workaround the include DEPS isn't it? How can we
clean
> this up?

[piman, 2012-07-26 21:40:42]

On Thu, Jul 26, 2012 at 2:38 PM, <sievers@chromium.org> wrote:

> Well, I could make a real interface, something like GpuSurfaceLookup.
> Then GpuSurfaceTracker could implement it for the browser process.
>
> Android will likely only need it in the browser process, as that's where
> the gpu
> thread for the compositor will run (to be able to share resources).
> However, the
> interface might also be nicer in case there is some test build I'm missing
> that
> might need a stub impl.


Cool, that sounds like a good approach.

Antoine


>
>
>
> On 2012/07/26 21:33:28, piman wrote:
>
> http://codereview.chromium.**org/10795058/diff/5001/**
>
content/common/gpu/image_**transport_surface_android.cc<http://codereview.chromium.org/10795058/diff/5001/content/common/gpu/image_transport_surface_android.cc>
>
>> File content/common/gpu/image_**transport_surface_android.cc (right):
>>
>
>
> http://codereview.chromium.**org/10795058/diff/5001/**
>
content/common/gpu/image_**transport_surface_android.cc#**newcode11<http://codereview.chromium.org/10795058/diff/5001/content/common/gpu/image_transport_surface_android.cc#newcode11>
>
>> content/common/gpu/image_**transport_surface_android.cc:**11:
>>
> gfx::AcceleratedWidget
>
>> GetNativeWidgetAndroid(int surface_id);
>> mmh, this is just here to workaround the include DEPS isn't it? How can we
>>
> clean
>
>> this up?
>>
>
>
>
>
http://codereview.chromium.**org/10795058/<http://codereview.chromium.org/107...
>

[no sievers, 2012-07-26 23:47:10]

Done.

[apatrick_chromium, 2012-07-26 23:54:21]

http://codereview.chromium.org/10795058/diff/12001/content/browser/gpu/gpu_su...
File content/browser/gpu/gpu_surface_tracker.cc (right):

http://codereview.chromium.org/10795058/diff/12001/content/browser/gpu/gpu_su...
content/browser/gpu/gpu_surface_tracker.cc:17: GpuSurfaceTracker*
GpuSurfaceTracker::GetInstance() {
Why not delete this GetInstance and just use the one in GpuSurfaceLookup
everywhere? It would need to use Singleton<> though since it is called on
multiple threads.

[piman, 2012-07-26 23:55:52]

one last thing, then I think it's good.

http://codereview.chromium.org/10795058/diff/12001/content/browser/gpu/gpu_su...
File content/browser/gpu/gpu_surface_tracker.cc (right):

http://codereview.chromium.org/10795058/diff/12001/content/browser/gpu/gpu_su...
content/browser/gpu/gpu_surface_tracker.cc:14:
GpuSurfaceTracker::~GpuSurfaceTracker() {
It'd be good to have a GpuSurfaceLookup::ResetInstance() here or something.

[no sievers, 2012-07-27 00:39:54]

Hmm, since the Singleton is a template does that not make it incompatible with
the class hierarchy? I guess the derived class would still have to instantiate
it, but then the base class cannot have a GetInstance(). Or maybe I'm missing a
way how to do it with one singleton..

[no sievers, 2012-07-27 00:40:09]

Done.

On 2012/07/26 23:55:52, piman wrote:
> one last thing, then I think it's good.
> 
>
http://codereview.chromium.org/10795058/diff/12001/content/browser/gpu/gpu_su...
> File content/browser/gpu/gpu_surface_tracker.cc (right):
> 
>
http://codereview.chromium.org/10795058/diff/12001/content/browser/gpu/gpu_su...
> content/browser/gpu/gpu_surface_tracker.cc:14:
> GpuSurfaceTracker::~GpuSurfaceTracker() {
> It'd be good to have a GpuSurfaceLookup::ResetInstance() here or something.

[apatrick_chromium, 2012-07-27 00:54:37]

Fair enough. LGTM.

[no sievers, 2012-07-27 01:31:15]

Add jam for gyp change.

[jam, 2012-07-27 02:04:15]

On 2012/07/27 01:31:15, Daniel Sievers wrote:
> Add jam for gyp change.

lgtm

feel free to tbr trivial gyp changes like this in the future

[commit-bot: I haz the power, 2012-07-27 02:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/10795058/14002

[commit-bot: I haz the power, 2012-07-27 02:25:34]

Try job failure for 10795058-14002 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-27 02:29:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/10795058/16002

[commit-bot: I haz the power, 2012-07-27 03:53:14]

Change committed as 148715