DO NOT COMMIT - DialogSurface initial implementation.

DialogSurface initial implementation.

This CL has been split up into:

https://codereview.chromium.org/2178973004
https://codereview.chromium.org/2220623002/

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=618368
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

[liberato (no reviews please), 2016-05-10 15:32:36]

Description was changed from

==========
DO NOT COMMIT - GpuSurfaceTexture for backup.

started getting window token
get context
compiles
started gpu surfaces

BUG=
==========

to

==========
DO NOT COMMIT - GpuSurfaceTexture for backup.

started getting window token
get context
compiles
started gpu surfaces

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-05-13 21:21:20]

Patchset #2 (id:20001) has been deleted

[liberato (no reviews please), 2016-05-13 21:25:20]

Description was changed from

==========
DO NOT COMMIT - GpuSurfaceTexture for backup.

started getting window token
get context
compiles
started gpu surfaces

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
DO NOT COMMIT - GpuSurfaceTexture for backup.

started getting window token
get context
compiles
started gpu surfaces

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-05-13 21:28:16]

just FYI - not ready for review.

i'm going to split this up into several CLs and clean it up more.  as it sits,
it will pick one AVDA instance at random and use an overlay for it.  turns off
the full screen capability for AVDA so that it isn't restarted on full screen,
which makes the 'magic' avda instance keep its surface and just resize to fit
full screen.

vertical positioning is off, since it's not converting from compositor
coordinates to screen coordinates.  things like the status bar change it.

-fl

[liberato (no reviews please), 2016-06-08 17:34:31]

Description was changed from

==========
DO NOT COMMIT - GpuSurfaceTexture for backup.

started getting window token
get context
compiles
started gpu surfaces

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
DialogSurface initial implementation.

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-06-08 17:36:59]

Description was changed from

==========
DialogSurface initial implementation.

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
DialogSurface initial implementation.

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=618368
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-06-08 17:37:57]

Description was changed from

==========
DialogSurface initial implementation.

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=618368
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
DialogSurface initial implementation.

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=618368
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-06-08 17:37:57]

liberato@chromium.org changed reviewers:
+ watk@chromium.org

[liberato (no reviews please), 2016-06-08 17:40:32]

it's now ready to review.  mind taking a first pass at it before i open it up to
a wider audience?

PS6 has example code showing how it's used.

also, suggestions on how to test any of this are appreciated.

thanks
-fl

[liberato (no reviews please), 2016-06-08 22:18:56]

liberato@chromium.org changed reviewers:
+ dalecurtis@chromium.org, timav@chromium.org

[liberato (no reviews please), 2016-06-08 22:19:55]

per offline discussion with watk@, opening this up for a bit wider review.

see PS6 for example usage (android_deferred_...cc)

thanks
-fl

[DaleCurtis, 2016-06-09 18:39:33]

Haven't looked through this much yet, but if you're going to use these dialog
surfaces outside of media, some place in gpu/ or content/ might be a better
place for it than media/

[liberato (no reviews please), 2016-06-09 18:45:09]

On 2016/06/09 18:39:33, DaleCurtis wrote:
> Haven't looked through this much yet, but if you're going to use these dialog
> surfaces outside of media, some place in gpu/ or content/ might be a better
> place for it than media/

it's unclear.  i opted for media since it will be our first consumer, and we can
work out the bugs under a tree in which we're owners.  we won't use it elsewhere
until then anyway.

thanks
-fl

[watk, 2016-06-10 21:46:39]

Just publishing nits I came across while doing a quick read over. I haven't got
the architecture clear enough in my head to consider higher level comments yet.

https://codereview.chromium.org/1967553002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl
(right):

https://codereview.chromium.org/1967553002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl:8:
//import org.chromium.media.IDialogSurfaceManager;
delete

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceCallback.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCallback.java:11: *
Callback helper for native.  This works with our JNI implementation to hop
Maybe a naive question, but I wonder if you considered removing the need for
this class and having DialogSurfaceControllerWrapper hide this from the native
code?

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCallback.java:13: *
object.  This isn't a wrapper because it's only used for native, and it's
Probably could delete the wrapper comment since it probably doesn't make a lot
of sense to anyone looking at this file in isolation.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceController.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceController.java:93:
release();
Should we log or assert here?

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceController.java:145:
final Dialog dialog = dialog_sync;
Do you need dialog_sync? Seems like you only need one local Dialog

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceManager.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManager.java:52:
private static int sMaxSurfaces = 1;
final

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java:14:
* Wrapper for IDialogSurfaceManager, to provide JNI bindings for arbitrariy
arbitrary

[liberato (no reviews please), 2016-06-10 22:50:05]

thanks for the feedback.

after some offline discussion, i think that i'll take dale's suggestion and
rename DialogSurfaceController into DialogSurface.  it's not part of this PS.

-fl

https://codereview.chromium.org/1967553002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl
(right):

https://codereview.chromium.org/1967553002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl:8:
//import org.chromium.media.IDialogSurfaceManager;
On 2016/06/10 21:46:38, watk wrote:
> delete

Done.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceCallback.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCallback.java:11: *
Callback helper for native.  This works with our JNI implementation to hop
On 2016/06/10 21:46:38, watk wrote:
> Maybe a naive question, but I wonder if you considered removing the need for
> this class and having DialogSurfaceControllerWrapper hide this from the native
> code?

unfortunately, i don't think that will work.  in order for the browser to use
the callback, it must have its own aidl interface.  i suppose that i could make
the controller wrapper also implement that interface, but not sure that wouldn't
be confusing.

the java object holds the callback, and the controller.cc holds the java object.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCallback.java:13: *
object.  This isn't a wrapper because it's only used for native, and it's
On 2016/06/10 21:46:38, watk wrote:
> Probably could delete the wrapper comment since it probably doesn't make a lot
> of sense to anyone looking at this file in isolation.

Done.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceController.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceController.java:93:
release();
On 2016/06/10 21:46:38, watk wrote:
> Should we log or assert here?

log, done.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceController.java:145:
final Dialog dialog = dialog_sync;
On 2016/06/10 21:46:38, watk wrote:
> Do you need dialog_sync? Seems like you only need one local Dialog

true, good point.  it has to be final for binding in the anonymous inner class.
moving the post inside the sync fixes it.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceManager.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManager.java:52:
private static int sMaxSurfaces = 1;
On 2016/06/10 21:46:38, watk wrote:
> final

Done.

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java
(right):

https://codereview.chromium.org/1967553002/diff/160001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java:14:
* Wrapper for IDialogSurfaceManager, to provide JNI bindings for arbitrariy
On 2016/06/10 21:46:38, watk wrote:
> arbitrary

Done.

[DaleCurtis, 2016-06-13 17:46:15]

Thinking longer term, should this just be a specialization of gfx::Surface? Or,
at least designed in such a way that it's not a major change to make whether
it's a dialog surface or regular surface indistinguishable to a client?

[liberato (no reviews please), 2016-06-13 18:05:26]

On 2016/06/13 17:46:15, DaleCurtis wrote:
> Thinking longer term, should this just be a specialization of gfx::Surface?
Or,
> at least designed in such a way that it's not a major change to make whether
> it's a dialog surface or regular surface indistinguishable to a client?

that's a very good question.  do you mean so that a client can write the code
and drop in DialogSurface or CVV or (maybe) VIDEO_HOLE implementations?

i'm not sure how to do that, since what's different in all those things is the
setup of the surface.  at the end of the day, one gets a ScopedJavaSurface, but
the setup is entirely different.

what do you have in mind?

[DaleCurtis, 2016-06-13 18:15:14]

I don't have any strongly-formed thoughts :) You'll need to chat further with
the compositing and GPU folks if you think the idea is appealing. My only
thought was that we might already have an abstraction system for
platform-specific surfaces -- in which case ensuring we can drop in later sounds
reasonable. It's possible no such system exists.

[liberato (no reviews please), 2016-06-13 18:30:38]

On 2016/06/13 18:15:14, DaleCurtis wrote:
> I don't have any strongly-formed thoughts :) You'll need to chat further with
> the compositing and GPU folks if you think the idea is appealing. My only
> thought was that we might already have an abstraction system for
> platform-specific surfaces -- in which case ensuring we can drop in later
sounds
> reasonable. It's possible no such system exists.

makes sense.  it'd be nice if it does, and we can do as you suggest.  i'll chat
with sievers to see if he has thoughts on how best to do it.

also, i updated the design doc.  it should make things clearer now.  i
completely forgot that i wrote one a while ago.  :)

[liberato (no reviews please), 2016-07-26 15:35:27]

liberato@chromium.org changed reviewers:
+ sievers@chromium.org

[liberato (no reviews please), 2016-08-09 16:43:38]

Description was changed from

==========
DialogSurface initial implementation.

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=618368
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
DialogSurface initial implementation.

This CL has been split up into:

https://codereview.chromium.org/2178973004
https://codereview.chromium.org/2220623002/

This CL adds support for getting Android Surfaces via Dialog, rather
than via SurfaceView.

These are constructed in the browser process.  If one is willing to
send the window token from the browser to the gpu, then the dialogs
may be created directly in the gpu process as well.

Positioning is exposed directly via the native wrappers.  In the
current CL, the position is not adjusted for the compositor's screen
position, just to simplify the CL.

BUG=618368
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-08-09 16:43:38]

liberato@chromium.org changed reviewers:
- dalecurtis@chromium.org, sievers@chromium.org, timav@chromium.org,
watk@chromium.org