Android: Remove Surface cruft

Android: Remove Surface cruft

This removes unused Surface-related Java-level IPC plumbing.
What is not needed anymore is the code to pass Surfaces to child
processes. This was only used in a deprecated render path and for
MediaPlayer being instantiated in the renderer (which is not supported
anymore, it lives in the browser process now).

NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186356

[no sievers, 2013-03-01 00:14:51]

This code is messy, I'm ripping out the unused paths here.

Min, can you double-check the video stuff? I did test playback with ContentShell
though.

Marcus, mind glancing over the mini-refactoring I've done with regard to JNI and
java object refs and such?
I added the Surface class (surface.h/.cc) to avoid having to include
jni/Surface_jni.h multiple times (see surface_texture_brige.cc and
surface_texture_peer_browser_impl.cc).

[no sievers, 2013-03-01 00:18:33]

https://codereview.chromium.org/12388038/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SandboxedProcessLauncher.java
(right):

https://codereview.chromium.org/12388038/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SandboxedProcessLauncher.java:280:
// right media player instance.
I should explain this further.

The old path was supporting bouncing a Surface from a GPU child process through
here (browser) and on to a renderer child (where it is passed to MediaPlayer).
That is unused since the MediaPlayer moved from the renderer into the browser.

However, what should happen here is that we pass the Surface coming from the GPU
process here to the MediaPlayer in this (browser process). Currently we only
support this with the GPU thread running in the browser (where it goes through
SurfaceTexturePeerBrowserImpl).

[qinmin, 2013-03-01 00:36:26]

https://codereview.chromium.org/12388038/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/12388038/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2705: if
(cmd_line->HasSwitch(switches::kMediaPlayerInRenderProcess)) {
keep this logic to create the mediaplayer in render process, we still need it
for DRT tests

[no sievers, 2013-03-01 00:43:04]

https://codereview.chromium.org/12388038/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/12388038/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2705: if
(cmd_line->HasSwitch(switches::kMediaPlayerInRenderProcess)) {
On 2013/03/01 00:36:26, qinmin wrote:
> keep this logic to create the mediaplayer in render process, we still need it
> for DRT tests

I made this use the same flag then (kInProcessWebGL) as 
RenderViewImpl::CreateGraphicsContext3D in here, which I *think* should then
have the same effect when it comes to tests that would instantiate a RenderView
without the full browser-side support.

[qinmin, 2013-03-01 03:32:17]

lgtm

[bulach, 2013-03-01 11:38:23]

hey daniel, nice stuff!

I have one suggestion below that could potentially simplify this a lot more, but
entirely possible I may have missed something :) let me know if that would work

the idea behind that we should try to enforce is to make java types as opaque as
possible:
if we're doing multiple operations or (un)boxing them in native, most of the
time is easier to just have a thin wrapper in java, send the params there, and
do everything in that side... it looks like this one is inspecting too much of
surface / surfacetexture / mediaplayer, and I hope we'll be able to wrap it all
the java side...

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.cc:50: env, cls.obj(),
"<init>", "(Landroid/graphics/SurfaceTexture;)V");
yay, let's remove *all* the cruft! :)

it seems to me MediaPlayerBridge* in SetVideoSurface @ 55 has already a JNI up
to MediaPlayerBridge.java..

so, let's remove all this block 47-56 here, and just:
player->SetVideoSurfaceTexture(surface_texture_bridge->j_surface_texture().obj())

in media_player_bridge.cc, change SetVideoSurface to SetVideoSurfaceTexture,
then rather than calling JNI_MediaPlayer::setSurface, call:

Java_MediaPlayerBridget_setSurfaceTexture(...)

in the java side, have a static method in MediaPlayerBridge.java passing the
media player and this surface_text object, like:

@CalledByNative
private static void setVideoSurfaceTexture(MediaPlayer player, SurfaceTexture
surface_texture) {
  Surface surface = new Surface(surface_texture);
  player.setSurface(surface);
}


wdyt?

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
File content/browser/android/surface_texture_peer_browser_impl.h (right):

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.h:22: explicit
SurfaceTexturePeerBrowserImpl();
nit: s/explicit//

https://codereview.chromium.org/12388038/diff/4002/content/common/android/sur...
File content/common/android/surface.h (right):

https://codereview.chromium.org/12388038/diff/4002/content/common/android/sur...
content/common/android/surface.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
see my comments above, I think we can get rid of this..

[no sievers, 2013-03-01 22:29:12]

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.cc:50: env, cls.obj(),
"<init>", "(Landroid/graphics/SurfaceTexture;)V");
On 2013/03/01 11:38:24, bulach wrote:
> yay, let's remove *all* the cruft! :)
> 

Thanks for pointing out this block, I should have read the code here a little
closer. Yea this should all go too, but what about this: This is basically all
the JNI (creating a Surface from SurfaceTexture) that I already added in this
patch, so I can just create a local Surface object here. That way it doesn't
narrow down the MediaPlayerBridge interface (a Surface is a more generic type
and it can also be passed across process boundaries, while SurfaceTexture
cannot).

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
File content/browser/android/surface_texture_peer_browser_impl.h (right):

https://codereview.chromium.org/12388038/diff/4002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.h:22: explicit
SurfaceTexturePeerBrowserImpl();
On 2013/03/01 11:38:24, bulach wrote:
> nit: s/explicit//

Done.

[bulach, 2013-03-04 10:44:51]

lgtm, I like this much better, thanks daniel!

I still have some suggestions here, hopefully the Reset/Release mismatch will
convince you to not have any surface.h whatsoever :) but feel free to go ahead
if you disagree, it's self-contained enough anyways...

a few nits and other random suggestions below:

https://codereview.chromium.org/12388038/diff/12001/content/browser/android/s...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/browser/android/s...
content/browser/android/surface_texture_peer_browser_impl.cc:47:
player->SetVideoSurface(surface.j_surface().obj());
I don't quite understand your concerns here, my main point is to avoid crossing
language boundaries and these little wrappers everywhere :)

how about player->SetVideoSurfaceFromSurfaceTexture, would that be a good
compromise? and then SurfaceTextureBridge::CreateSurface() would be the only
place where it needs to call up to Surface.java, but then again, it's just two
extra lines compared to the current version in this patch set, rather than an
entire .h/.cc file :) wdyt?

https://codereview.chromium.org/12388038/diff/12001/content/browser/android/s...
content/browser/android/surface_texture_peer_browser_impl.cc:48:
surface.Release();
ok, so if you don't agree with the above :)

"Release" method seems redundant.. maybe call it "ScopedSurface" so that it's
more in line with the other ScopedJava(Global|Local) method, and then call Reset
in its dtor? (Release means ownership being transferred out, Reset means
destruction, which is what I think it means here..)

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
File content/common/android/surface.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
nit: 2013 :)

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.cc:39: JNI_Surface::Java_Surface_release(env,
j_surface_.obj());
I see the naming clash here on Release vs Reset... :-/
since it's not really exposing anything from Surface but it rather looks like a
ScopedJavaRef with an extra call on destruction, I think this method should be
Reset, and called on the dtor "if (j_surface_.obj())"

and also, I think j_surface_.Reset() should be called here as well, right?

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
File content/common/android/surface.h (right):

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
nit: 2013 :)

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.h:16: Surface(const
base::android::JavaRef<jobject>& surface_texture);
nit: explicit

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.h:19: void Release();
nit: as above, it seems that this method can go away, just move it to the dtor

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
File content/common/android/surface_texture_bridge.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface_texture_bridge.cc:144: surface.Release();
nit: as above..

https://codereview.chromium.org/12388038/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2705: if
(cmd_line->HasSwitch(switches::kInProcessWebGL)) {
nit: is this right?

[no sievers, 2013-03-04 18:09:52]

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
File content/common/android/surface.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.cc:39: JNI_Surface::Java_Surface_release(env,
j_surface_.obj());
On 2013/03/04 10:44:51, bulach wrote:
> I see the naming clash here on Release vs Reset... :-/
> since it's not really exposing anything from Surface but it rather looks like
a
> ScopedJavaRef with an extra call on destruction, I think this method should be
> Reset, and called on the dtor "if (j_surface_.obj())"
> 
> and also, I think j_surface_.Reset() should be called here as well, right?

Actually, this method (in the java class) is named a bit ambiguously. What it
does is release the graphics memory referenced from the native side of the class
instance (to avoid having to rely on GC for this to happen). I think it might be
better to leave it as an explicit method to deal with this (one has to be a bit
careful when to do this), but I'll add a comment to the method.

I have another patch I'm working on where I need to call Surface.release(), but
there I create the Surface object in Java, so I'd either need to expose a static
Release(jobject) method after all, or have a constructor (from a jobject - I
guess I could expose three static methods for construction: Create(),
CreateFromSurface(jobject), CreateFromSurfaceTexture(jobject)). What do you
think?

https://codereview.chromium.org/12388038/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2705: if
(cmd_line->HasSwitch(switches::kInProcessWebGL)) {
On 2013/03/04 10:44:51, bulach wrote:
> nit: is this right?

Sort of :)
This code is currently unused I think, but Min wants to keep it. It's meant for
testing, where taking the same path here as CreateGraphicsContext3D() in this
file (which checks the same cmdline) should be the right thing.
I want to get rid of the 'InRendererProcess' command line, because that is
deprecated. For testing it's really something else (it's running single-process,
and is probably mocking browser-side parts).

[bulach, 2013-03-04 18:48:51]

still lgtm but some clarifications below: I think I'm just extending your
proposal a little bit further but not being too controversial :)

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
File content/common/android/surface.cc (right):

https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
content/common/android/surface.cc:39: JNI_Surface::Java_Surface_release(env,
j_surface_.obj());
On 2013/03/04 18:09:52, Daniel Sievers wrote:
> On 2013/03/04 10:44:51, bulach wrote:
> > I see the naming clash here on Release vs Reset... :-/
> > since it's not really exposing anything from Surface but it rather looks
like
> a
> > ScopedJavaRef with an extra call on destruction, I think this method should
be
> > Reset, and called on the dtor "if (j_surface_.obj())"
> > 
> > and also, I think j_surface_.Reset() should be called here as well, right?
> 
> Actually, this method (in the java class) is named a bit ambiguously. What it
> does is release the graphics memory referenced from the native side of the
class
> instance (to avoid having to rely on GC for this to happen). I think it might
be
> better to leave it as an explicit method to deal with this (one has to be a
bit
> careful when to do this), but I'll add a comment to the method.
> 
> I have another patch I'm working on where I need to call Surface.release(),
but
> there I create the Surface object in Java, so I'd either need to expose a
static
> Release(jobject) method after all, or have a constructor (from a jobject - I
> guess I could expose three static methods for construction: Create(),
> CreateFromSurface(jobject), CreateFromSurfaceTexture(jobject)). What do you
> think?

I agree it's going to be hard to fix the API naming mismatch :)
let's try to hide that then!!

I'm not sure about "CreateFromSurface" though, since it won't really create a
surface, it'd just wrap an existing one, right?

how about this:
- name this "ScopedJavaSurface", so that it's clear it will NOT expose the whole
java Surface API ever.
- make the constructor take no arguments.
- make the dtor call a private release, or even better, just call these two
lines here in the dtor..
- have two methods: "ResetFromSurface(jobject)" and
"ResetFromSurfaceTexture(jobject)".. document that the first will just ensure
that Surface::Release will be called upon going out of scope, and the latter
will create a a new Surface for the given SurfaceTexture, and ensure it'll be
Release once it goes out of scope..

overall goals being:
- it hides badly named APIs ;)
- it mimics the existing |ScopedJava*| API and semantics with Reset and
releasing refs on dtor
- by being "ScopedJavaSurface" rather than just "Surface" we ensure this will
not grow out of proportions, and it also clarifies its only purpose is to manage
a Surface.java lifetime..

wdyt?

[no sievers, 2013-03-04 19:57:38]

On 2013/03/04 18:48:51, bulach wrote:
> still lgtm but some clarifications below: I think I'm just extending your
> proposal a little bit further but not being too controversial :)
> 
>
https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
> File content/common/android/surface.cc (right):
> 
>
https://codereview.chromium.org/12388038/diff/12001/content/common/android/su...
> content/common/android/surface.cc:39: JNI_Surface::Java_Surface_release(env,
> j_surface_.obj());
> On 2013/03/04 18:09:52, Daniel Sievers wrote:
> > On 2013/03/04 10:44:51, bulach wrote:
> > > I see the naming clash here on Release vs Reset... :-/
> > > since it's not really exposing anything from Surface but it rather looks
> like
> > a
> > > ScopedJavaRef with an extra call on destruction, I think this method
should
> be
> > > Reset, and called on the dtor "if (j_surface_.obj())"
> > > 
> > > and also, I think j_surface_.Reset() should be called here as well, right?
> > 
> > Actually, this method (in the java class) is named a bit ambiguously. What
it
> > does is release the graphics memory referenced from the native side of the
> class
> > instance (to avoid having to rely on GC for this to happen). I think it
might
> be
> > better to leave it as an explicit method to deal with this (one has to be a
> bit
> > careful when to do this), but I'll add a comment to the method.
> > 
> > I have another patch I'm working on where I need to call Surface.release(),
> but
> > there I create the Surface object in Java, so I'd either need to expose a
> static
> > Release(jobject) method after all, or have a constructor (from a jobject - I
> > guess I could expose three static methods for construction: Create(),
> > CreateFromSurface(jobject), CreateFromSurfaceTexture(jobject)). What do you
> > think?
> 
> I agree it's going to be hard to fix the API naming mismatch :)
> let's try to hide that then!!
> 
> I'm not sure about "CreateFromSurface" though, since it won't really create a
> surface, it'd just wrap an existing one, right?
> 
> how about this:
> - name this "ScopedJavaSurface", so that it's clear it will NOT expose the
whole
> java Surface API ever.
> - make the constructor take no arguments.
> - make the dtor call a private release, or even better, just call these two
> lines here in the dtor..
> - have two methods: "ResetFromSurface(jobject)" and
> "ResetFromSurfaceTexture(jobject)".. document that the first will just ensure
> that Surface::Release will be called upon going out of scope, and the latter
> will create a a new Surface for the given SurfaceTexture, and ensure it'll be
> Release once it goes out of scope..
> 
> overall goals being:
> - it hides badly named APIs ;)
> - it mimics the existing |ScopedJava*| API and semantics with Reset and
> releasing refs on dtor
> - by being "ScopedJavaSurface" rather than just "Surface" we ensure this will
> not grow out of proportions, and it also clarifies its only purpose is to
manage
> a Surface.java lifetime..
> 
> wdyt?

Yes, I like it, esp. since Surface is probably never going to be a very useful
object in the native code, apart from the need to pass it around like here. See
updated patch. I'll be happy to change the constructors to explicit
ResetFrom..() functions like you suggested, although it seems it's pretty
type-safe this way (with the distinguished type and InstanceOf check for
construct-from-surface).

[no sievers, 2013-03-04 22:53:36]

+piman for content/ OWNERS.

Missing OWNER reviewers for these files:
    content/common/gpu/gpu_channel.h
    content/renderer/gpu/stream_texture_host_android.h
    content/browser/browser_main_loop.cc
    content/renderer/gpu/stream_texture_host_android.cc
    content/common/gpu/media/android_video_decode_accelerator.cc
    content/common/gpu/stream_texture_manager_android.h
    content/common/gpu/gpu_messages.h
    content/content_common.gypi
    content/common/gpu/stream_texture_manager_android.cc
    content/renderer/media/stream_texture_factory_impl_android.cc
    content/common/gpu/gpu_channel.cc
    content/public/common/content_switches.cc
    content/public/common/content_switches.h
    content/renderer/render_view_impl.cc

[piman, 2013-03-04 22:56:29]

LGTM+nit

https://codereview.chromium.org/12388038/diff/9028/content/common/android/sco...
File content/common/android/scoped_java_surface.h (right):

https://codereview.chromium.org/12388038/diff/9028/content/common/android/sco...
content/common/android/scoped_java_surface.h:24: ScopedJavaSurface(const
base::android::JavaRef<jobject>& surface);
nit: explicit

https://codereview.chromium.org/12388038/diff/9028/content/common/android/sco...
content/common/android/scoped_java_surface.h:28: ScopedJavaSurface(const
SurfaceTextureBridge* surface_texture);
nit:explicit

[no sievers, 2013-03-04 22:56:40]

https://codereview.chromium.org/12388038/diff/9028/content/common/gpu/media/a...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/12388038/diff/9028/content/common/gpu/media/a...
content/common/gpu/media/android_video_decode_accelerator.cc:9: #include
"base/android/jni_android.h"
Ok, the jni includes and the 'using base::android::MethodID' below can be
removed too.

[no sievers, 2013-03-04 22:59:16]

https://codereview.chromium.org/12388038/diff/9028/content/common/android/sco...
File content/common/android/scoped_java_surface.h (right):

https://codereview.chromium.org/12388038/diff/9028/content/common/android/sco...
content/common/android/scoped_java_surface.h:24: ScopedJavaSurface(const
base::android::JavaRef<jobject>& surface);
On 2013/03/04 22:56:29, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/12388038/diff/9028/content/common/android/sco...
content/common/android/scoped_java_surface.h:28: ScopedJavaSurface(const
SurfaceTextureBridge* surface_texture);
On 2013/03/04 22:56:29, piman wrote:
> nit:explicit

Done.

[bulach, 2013-03-05 10:32:33]

lgtm, android and the JNI is now much clearer, thanks a ton!!

https://codereview.chromium.org/12388038/diff/6034/content/common/android/sco...
File content/common/android/scoped_java_surface.h (right):

https://codereview.chromium.org/12388038/diff/6034/content/common/android/sco...
content/common/android/scoped_java_surface.h:17: // When going out of scope,
release() is called on the Java object to make
nit: to be extra clear, "Surface.release() is called..."

https://codereview.chromium.org/12388038/diff/6034/content/common/android/sco...
content/common/android/scoped_java_surface.h:24: explicit
ScopedJavaSurface(const base::android::JavaRef<jobject>& surface);
the thing about "ResetFrom*" is that we could potentially have
"ResetFromJavaFoo" and "ResetFromJavaBar" and so on, all taking a
base::android::JavaRef<jobject>... this is not possible by just overriding the
constructor..


but yeah, since there doesn't seem to be any other way to create Surface anyway,
leave as is... :)

[no sievers, 2013-03-05 21:21:46]

https://codereview.chromium.org/12388038/diff/6034/content/common/android/sco...
File content/common/android/scoped_java_surface.h (right):

https://codereview.chromium.org/12388038/diff/6034/content/common/android/sco...
content/common/android/scoped_java_surface.h:17: // When going out of scope,
release() is called on the Java object to make
On 2013/03/05 10:32:33, bulach wrote:
> nit: to be extra clear, "Surface.release() is called..."

Done.

[no sievers, 2013-03-05 21:30:22]

+palmer: Could you bless the changes to gpu_messages.h? Am just removing a field
there.

[no sievers, 2013-03-06 00:08:56]

+other Chris for removing a field from content/common/gpu/gpu_messages.h

[palmer, 2013-03-06 00:20:20]

IPC security LGTM.

[commit-bot: I haz the power, 2013-03-06 00:26:38]

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

[commit-bot: I haz the power, 2013-03-06 04:23:34]

Change committed as 186356