This is initial API support required for enabling SurfaceTexture backed zero-copy for Android

This is initial API support required for enabling SurfaceTexture backed zero-copy for Android

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

[reveman, 2014-03-08 14:10:20]

https://codereview.chromium.org/191933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:24:
private static SurfaceTexture create(int textureId, boolean single_buffered) {
s/single_buffered/singleBufferMode/

https://codereview.chromium.org/191933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:51:
private static void releaseTexImage (SurfaceTexture surfaceTexture) {
nit: no need for a space between "TexImage" and "(SurfaceTex"

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_texture.h
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_textur...
ui/gl/android/surface_texture.h:24: SurfaceTexture(int texture_id, bool
single_buffered = false);
s/single_buffered/single_buffer_mode/ to be consistent with Android API and I
prefer if it doesn't have a default value.

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_textur...
ui/gl/android/surface_texture.h:36: // Releases the the texture content. This is
needed in single buffered mode
s/the the/the/

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_textur...
ui/gl/android/surface_texture.h:37: // to allow the image content producer to
take ownership of the image buffer
Comment should end with a period ".".

[sohanjg, 2014-03-08 14:20:15]

PTAL. Thanks

https://codereview.chromium.org/191933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:24:
private static SurfaceTexture create(int textureId, boolean single_buffered) {
On 2014/03/08 14:10:20, reveman wrote:
> s/single_buffered/singleBufferMode/

Done.

https://codereview.chromium.org/191933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:51:
private static void releaseTexImage (SurfaceTexture surfaceTexture) {
On 2014/03/08 14:10:20, reveman wrote:
> nit: no need for a space between "TexImage" and "(SurfaceTex"

Done.

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_texture.h
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_textur...
ui/gl/android/surface_texture.h:24: SurfaceTexture(int texture_id, bool
single_buffered = false);
On 2014/03/08 14:10:20, reveman wrote:
> s/single_buffered/single_buffer_mode/ to be consistent with Android API and I
> prefer if it doesn't have a default value.

Done.

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_textur...
ui/gl/android/surface_texture.h:36: // Releases the the texture content. This is
needed in single buffered mode
On 2014/03/08 14:10:20, reveman wrote:
> s/the the/the/

Done.

https://codereview.chromium.org/191933002/diff/1/ui/gl/android/surface_textur...
ui/gl/android/surface_texture.h:37: // to allow the image content producer to
take ownership of the image buffer
On 2014/03/08 14:10:20, reveman wrote:
> Comment should end with a period ".".

Done.

[reveman, 2014-03-08 14:28:25]

https://codereview.chromium.org/191933002/diff/20001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.cc (right):

https://codereview.chromium.org/191933002/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.cc:29: SurfaceTexture::SurfaceTexture(int
texture_id, bool single_buffered) {
s/single_buffered/single_buffer_mode/

https://codereview.chromium.org/191933002/diff/20001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:36: // Release the the texture content. This is
needed in single buffered mode
nit: s/the the/the/

[reveman, 2014-03-08 14:47:49]

lgtm

+sievers
+piman for gpu/command_buffer
+bulach for ui/android

[bulach, 2014-03-10 12:32:30]

hmm... I think bots won't be happy, more inline.

sievers certainly know more about it:

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:25:
return new SurfaceTexture(textureId, singleBufferMode);
I think this will break quite a few bots not on KitKat :(

at a minimum, it needs to be guarded with Build.VERSION.SDK_INT >=
Build.VERSION_CODES.KITKAT.

this is going to be tricky to implement down in C++.
what are the semantics there?
should we fail when trying singleBufferMode before KK?
should we gracefully fallback?
how about the releaseTextImage, would it fail or fallback?

some options:
- have two explicit factory methods, "create(int textureId)" and
"createSingleBuffer(int textureId)".
- this would then translate to two constructors (or better, factory methods) in
the C++ side..

alternatively:
- have a subclass in C++, SingleBufferSurfaceTexture, deriving from
SurfaceTexture and introducing the ReleaseTextImage() method.. then, it'd be
easier to spot that it's only used where actually available..


to be clear: the java part is private and an internal detail of the C++ side..
I'm not too concerned about this.
my concern is actually on the C++ side, and making sure that we won't start
sprinkling version checks all over the place there...

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:52:
surfaceTexture.releaseTexImage();
assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT;

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.cc (right):

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.cc:55: void SurfaceTexture::ReleaseTexImage() {
should this be protected by base::android::BuildInfo::GetInstance()->sdk_int()
check?

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:37: // to allow the image content producer to
take ownership of the image buffer.
should we comment that this is only available since KitKat?

[reveman, 2014-03-10 13:33:05]

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:25:
return new SurfaceTexture(textureId, singleBufferMode);
On 2014/03/10 12:32:31, bulach wrote:
> I think this will break quite a few bots not on KitKat :(
> 
> at a minimum, it needs to be guarded with Build.VERSION.SDK_INT >=
> Build.VERSION_CODES.KITKAT.

Right. Forgot that this is only available since kitkat.

> 
> this is going to be tricky to implement down in C++.
> what are the semantics there?
> should we fail when trying singleBufferMode before KK?
> should we gracefully fallback?

Silent fallback can be a problem. Let's not do that.

> how about the releaseTextImage, would it fail or fallback?

Fail. We should probably have a DCHECK in C++ code to guard against it being
used on non-single buffered surface textures.

> 
> some options:
> - have two explicit factory methods, "create(int textureId)" and
> "createSingleBuffer(int textureId)".
> - this would then translate to two constructors (or better, factory methods)
in
> the C++ side..
> 
> alternatively:
> - have a subclass in C++, SingleBufferSurfaceTexture, deriving from
> SurfaceTexture and introducing the ReleaseTextImage() method.. then, it'd be
> easier to spot that it's only used where actually available..

Either is fine with me. Maybe prefer the two ctor approach a bit as it matches
the Android Java API for SurfaceTexture better.

I guess another alternative is adding:
static bool IsSingleBufferModeSupported()
or just making it single_buffer_mode=true illegal unless on kitkat and later and
push the responsibility of checking
base::android::BuildInfo::GetInstance()->sdk_int() to the client.

> 
> 
> to be clear: the java part is private and an internal detail of the C++ side..
> I'm not too concerned about this.
> my concern is actually on the C++ side, and making sure that we won't start
> sprinkling version checks all over the place there...

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.cc (right):

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.cc:55: void SurfaceTexture::ReleaseTexImage() {
On 2014/03/10 12:32:31, bulach wrote:
> should this be protected by base::android::BuildInfo::GetInstance()->sdk_int()
> check?

I'd prefer DCHECK(single_buffer_mode_)

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/40001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:37: // to allow the image content producer to
take ownership of the image buffer.
On 2014/03/10 12:32:31, bulach wrote:
> should we comment that this is only available since KitKat?

I don't think so. We can add a comment about this only being supported in single
buffer mode but we should not expose the idea of android versions unless the
client of this API can also use that to determine if single buffered mode is
supported.

[bulach, 2014-03-10 13:47:48]

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:25:
return new SurfaceTexture(textureId, singleBufferMode);
On 2014/03/10 13:33:05, reveman wrote:
> On 2014/03/10 12:32:31, bulach wrote:
> > I think this will break quite a few bots not on KitKat :(
> > 
> > at a minimum, it needs to be guarded with Build.VERSION.SDK_INT >=
> > Build.VERSION_CODES.KITKAT.
> 
> Right. Forgot that this is only available since kitkat.
> 
> > 
> > this is going to be tricky to implement down in C++.
> > what are the semantics there?
> > should we fail when trying singleBufferMode before KK?
> > should we gracefully fallback?
> 
> Silent fallback can be a problem. Let's not do that.
> 
> > how about the releaseTextImage, would it fail or fallback?
> 
> Fail. We should probably have a DCHECK in C++ code to guard against it being
> used on non-single buffered surface textures.
> 
> > 
> > some options:
> > - have two explicit factory methods, "create(int textureId)" and
> > "createSingleBuffer(int textureId)".
> > - this would then translate to two constructors (or better, factory methods)
> in
> > the C++ side..
> > 
> > alternatively:
> > - have a subclass in C++, SingleBufferSurfaceTexture, deriving from
> > SurfaceTexture and introducing the ReleaseTextImage() method.. then, it'd be
> > easier to spot that it's only used where actually available..
> 
> Either is fine with me. Maybe prefer the two ctor approach a bit as it matches
> the Android Java API for SurfaceTexture better.
> 
> I guess another alternative is adding:
> static bool IsSingleBufferModeSupported()
> or just making it single_buffer_mode=true illegal unless on kitkat and later
and
> push the responsibility of checking
> base::android::BuildInfo::GetInstance()->sdk_int() to the client.

thanks reveman! of all alternatives, I think your proposal of "static bool
IsSingleBufferModeSupported()" would be my preferred :)
that, and making the C++ constructor private and having explicitly named factory
functions in c++...
the reason being that further down line, it'll probably be easier for reviewers
to ensure things like:

if (SurfaceTexture::IsSingleBufferModeSupported())
  ... SurfaceTexture::CreateSingleBufferModeTexture(...);

than:
if (BuildInfo...sdk_int()...)
  ... new SurfaceTexture::SurfaceTexture(..., true);


I think we're in the same page here, but just want to be more explicit. :)

oh, and of course, "CreateSingleBufferModeTexture()" should have a comment
saying "this must only be called if |IsSingleBufferModeSupported()|", and a
DCHECK for that.

the java part also needs to be split, since this ctor with two params is not
available pre kk..

> 
> > 
> > 
> > to be clear: the java part is private and an internal detail of the C++
side..
> > I'm not too concerned about this.
> > my concern is actually on the C++ side, and making sure that we won't start
> > sprinkling version checks all over the place there...

[reveman, 2014-03-10 16:16:58]

https://codereview.chromium.org/191933002/diff/50001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/50001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:25:
return new SurfaceTexture(textureId, singleBufferMode);
you'll need to add a createSingleBuffered as this doesn't work unless
"Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT"

https://codereview.chromium.org/191933002/diff/50001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/50001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:24: static SurfaceTexture* Create(int
texture_id);
this should return a scoped_refptr<SurfaceTexture>

https://codereview.chromium.org/191933002/diff/50001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:26: static SurfaceTexture*
CreateSingleBufferMode(int texture_id);
I think "CreateSingleBuffered" is better

https://codereview.chromium.org/191933002/diff/50001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:67: // static
nit: no need for "// static" comment

https://codereview.chromium.org/191933002/diff/50001/ui/gl/android/surface_te...
ui/gl/android/surface_texture.h:73: SurfaceTexture(int texture_id, bool
single_buffer_mode);
How about we make this private and take a jobject:
SurfaceTexture(jobject surface_texture)

the Create function would do something like this instead:

::Create(int texture_id) {
  JNIEnv* env = base::android::AttachCurrentThread();
  base::android::ScopedJavaGlobalRef<jobject> j_surface_texture(
      Java_SurfaceTexturePlatformWrapper_create(env, texture_id))
  return make_scoped_refptr(
      new SurfaceTexture(j_surface_texture.get());
}

and ::CreateSingleBuffer would do the same but use
Java_SurfaceTexturePlatformWrapper_createSingleBuffered instead

[epennerAtGoogle, 2014-03-10 18:54:07]

https://codereview.chromium.org/191933002/diff/50001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/50001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:25:
return new SurfaceTexture(textureId, singleBufferMode);
On 2014/03/10 16:16:58, reveman wrote:
> you'll need to add a createSingleBuffered as this doesn't work unless
> "Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT"

Would something like this work? Unless we want more explicit names in java also.
In that case we should have two functions.

if (singleBufferMode) {
  assert(Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT);
  return new SurfaceTexture(textureId, true);
} else {
  return new SurfaceTexture(textureId);
}

[reveman, 2014-03-10 19:02:13]

https://codereview.chromium.org/191933002/diff/50001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java
(right):

https://codereview.chromium.org/191933002/diff/50001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java:25:
return new SurfaceTexture(textureId, singleBufferMode);
On 2014/03/10 18:54:07, epennerAtGoogle wrote:
> On 2014/03/10 16:16:58, reveman wrote:
> > you'll need to add a createSingleBuffered as this doesn't work unless
> > "Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT"
> 
> Would something like this work? Unless we want more explicit names in java
also.
> In that case we should have two functions.
> 
> if (singleBufferMode) {
>   assert(Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT);
>   return new SurfaceTexture(textureId, true);
> } else {
>   return new SurfaceTexture(textureId);
> }

It would work but I prefer if we only branch once and it happens when the client
checks the value of IsSingleBufferModeSupported(). It's a bit ugly to have an
"if (singleBufferMode)" statement in the java code when we've already done "if
(IsSingleBufferModeSupported())" in the c++ world IMO.

[epennerAtGoogle, 2014-03-10 19:03:41]

> It would work but I prefer if we only branch once and it happens when the
client
> checks the value of IsSingleBufferModeSupported(). It's a bit ugly to have an
> "if (singleBufferMode)" statement in the java code when we've already done "if
> (IsSingleBufferModeSupported())" in the c++ world IMO.

Works with me. Was just reading the reasoning for the C++. I agree two functions
is more clear (and less prone to making mistakes and not checking API level).

[piman, 2014-03-10 19:33:57]

LGTM % other comments.

[sohanjg, 2014-03-11 08:11:32]

Updated with the comments.
PTAL.Thanks

[bulach, 2014-03-11 16:13:16]

lgtm % some small comments.. sievers / epenner may want to take a look at it
too?

https://codereview.chromium.org/191933002/diff/110001/ui/gl/android/surface_t...
File ui/gl/android/surface_texture.h (right):

https://codereview.chromium.org/191933002/diff/110001/ui/gl/android/surface_t...
ui/gl/android/surface_texture.h:40: // of the image buffer.
nit: I'd be even more explicit:
this is *only* supported on SurfaceTexture instantiated via
|CreateSingleBuffered(...)|.

https://codereview.chromium.org/191933002/diff/110001/ui/gl/android/surface_t...
ui/gl/android/surface_texture.h:67: static bool IsSingleBufferModeSupported();
nit: hmm... do we need a non-static method too?

I'm thinking about the following pattern:
scoped_refptr<SurfaceTexture> s = SurfaceTexture::Create(0);
if (SurfaceTexture::IsSingleBufferModeSupported()) {
  s->ReleaseTexImage();
}


should we document that this should only be used to guard the
CreateSingleBuffered() call?

[epennerAtGoogle, 2014-03-11 19:22:07]

lgtm % other nits. Looking good!

[sohanjg, 2014-03-12 06:14:15]

Addressed build bot issue and nits.
PTAL. Thanks

[sohanjg, 2014-03-12 06:14:36]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-03-12 06:15:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/191933002/120001

[sohanjg, 2014-03-12 06:38:05]

The CQ bit was unchecked by sohan.jyoti@samsung.com

[sohanjg, 2014-03-12 08:26:35]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-03-12 08:28:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/191933002/140001

[commit-bot: I haz the power, 2014-03-12 16:51:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/191933002/140001

[commit-bot: I haz the power, 2014-03-12 19:11:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/191933002/140001

[commit-bot: I haz the power, 2014-03-12 19:38:55]

Change committed as 256604

[sohanjg, 2015-11-18 09:26:43]

Description was changed from

==========
This is initial API support required for enabling SurfaceTexture backed
zero-copy for Android

BUG=269808

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

to

==========
This is initial API support required for enabling SurfaceTexture backed
zero-copy for Android

BUG=269808

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