Implement StreamTexture::BindTexImage.

Implement StreamTexture::BindTexImage.

Implement StreamTexture::BindTexImage.

Currently StreamTexture takes the textureid as parameter and
binds it while creation. Ideally a valid image id can be
returned to the client so that it could then call
glBindTexImage2D()


BUG=339191
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

[sivag, 2015-05-12 15:20:27]

siva.gunturi@samsung.com changed reviewers:
+ sievers@chromium.org

[sivag, 2015-05-12 15:30:39]

Hi Sievers,

Please take a look at the flow
This is what i am planning to do

1.Pass stream_id, frame_id, player_id,(surface_texture_id?) using message to
BrowserSurfaceTextureManager , Currrently i am passing via
BrowserSurfaceTextureManager.

2.For inprocess, we can get surfacetexture via GetSurfaceTexture using stream_id
and SetSurfacePeer.

3. For outofprocess/browser process i am little bit confused about the surface
texture id ?Is this the textureid in the WebMediaPlayerAndroid?

Please share your thoughts.

[no sievers, 2015-05-28 21:33:35]

Sorry, I missed this.

On 2015/05/12 15:30:39, Siva Gunturi wrote:
> 1.Pass stream_id, frame_id, player_id,(surface_texture_id?) using message to
> BrowserSurfaceTextureManager , Currrently i am passing via
> BrowserSurfaceTextureManager.
> 

Ideally the surface texture manager would not know about media-specific ids.
Maybe we don't need to mix up this code with the gpu memory buffer stuff (or at
least not yet).

Can we have another map in ChildProcessLauncher.java? It now has sViewSurfaceMap
and sSurfaceTextureSurfaceMap.
We could add sStreamTextureMap with its own global namespace.

Then we could reroute the CreateStreamTexture IPC so that it goes from renderer
to browser to GPU process.
On the way back it returns a Surface to the browser.

I think we might be able to then entirely get rid of 'establish peer' because
the media player can tell the browser right away about the identifier of the
surface it needs to use for compositing. And I think the browser already might
know better anyways which surface to decode into (fullscreen, composited i.e.
stream texture, or external).

[sivag, 2015-05-29 09:42:01]

On 2015/05/28 21:33:35, sievers wrote:
> Sorry, I missed this.
> 
> On 2015/05/12 15:30:39, Siva Gunturi wrote:
> > 1.Pass stream_id, frame_id, player_id,(surface_texture_id?) using message to
> > BrowserSurfaceTextureManager , Currrently i am passing via
> > BrowserSurfaceTextureManager.
> > 
> 
> Ideally the surface texture manager would not know about media-specific ids.
> Maybe we don't need to mix up this code with the gpu memory buffer stuff (or
at
> least not yet).
> 
> Can we have another map in ChildProcessLauncher.java? It now has
sViewSurfaceMap
> and sSurfaceTextureSurfaceMap.
> We could add sStreamTextureMap with its own global namespace.
> 
> Then we could reroute the CreateStreamTexture IPC so that it goes from
renderer
> to browser to GPU process.
> On the way back it returns a Surface to the browser.
> 
> I think we might be able to then entirely get rid of 'establish peer' because
> the media player can tell the browser right away about the identifier of the
> surface it needs to use for compositing. And I think the browser already might
> know better anyways which surface to decode into (fullscreen, composited i.e.
> stream texture, or external).

Thanks for the inputs sievers, 
i will check this.

[sivag, 2015-06-11 16:05:01]

@sievers , Ptal..

1.i routed the ipc to browser via mediaplayer (please suggest if we need to go
through renderview)
2.from mediaplayer directed the createstream to RWHVA and then to gl helper
3.in glhelper using gl interface, the stream texture is created..
4.as of now the sync ipc createStreamTextureChromium returns the streamid,


TBD:
----
1. can we change the createStreamTextureChromium to give both streamid and
surfacetexture?
 this will simplify the code instead of posting this to gpu ? then save surface,
stream_id  in the browser map.
2. as of now we are maintaing texture_id, mail_box  on the renderer side
can we maintain this in the browser? just like createstreamtexture , we will
send a destroy message to browser
to destroy this?
3. stream_id we will send back to renderer through ipc once
the createstreamtexture is done.
4.Once this is done, establishpeer can be sent to browser directly.

[no sievers, 2015-06-11 20:23:43]

It should use the original client context to do things.
I think it should work more like gpu memory buffers are created, (or also see
CreateViewCOmmandBuffer for a client command that flows through the browser.)

[sivag, 2015-06-22 16:02:54]

On 2015/06/11 20:23:43, sievers wrote:
> It should use the original client context to do things.
> I think it should work more like gpu memory buffers are created, (or also see
> CreateViewCOmmandBuffer for a client command that flows through the browser.)

@Sievers, Just a small update,(the patch is still wip)
i changed the flow by using the client context,
the sync ipc has some problem, which i will fix, and then
adding surface_texture to map is pending.


ptal..and let me know your comments.

[sivag, 2015-06-23 15:39:59]

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
When i am trying to access TextureRef* texture  for the created texture id ,
texture is coming null.

When the same call is made from renderer to gpu, it works well, while using
gl->CreateStreamTextureCHROMIUM(*texture_id);

When i verifed , in both approaches (renderer->gpu, render->browser->gpu)both
the (stub pointers, stream_id, route_id, textureid) every thing is same when
calling StreamTexture::Create(this, texture_id, stream_id);

I am currently debugging this problem.
Am i missing something here, Any pointers would be helpful?

[no sievers, 2015-06-25 00:19:51]

On 2015/06/23 15:39:59, sivag wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> When i am trying to access TextureRef* texture  for the created texture id ,
> texture is coming null.
> 
> When the same call is made from renderer to gpu, it works well, while using
> gl->CreateStreamTextureCHROMIUM(*texture_id);
> 
> When i verifed , in both approaches (renderer->gpu, render->browser->gpu)both
> the (stub pointers, stream_id, route_id, textureid) every thing is same when
> calling StreamTexture::Create(this, texture_id, stream_id);
> 
> I am currently debugging this problem.
> Am i missing something here, Any pointers would be helpful?

Does it work with a Finish() after glGenTextures()?
The problem is likely that things happen on different IPC channels which are not
ordered wrt each other.

The easiest would be to sidestep the whole problem and not require a texture
from the client when creating the SurfaceTexture.

One way to achieve that would be to use the workaround from
gl_image_surface_texture.cc and create the SurfaceTexture with an id of 0, and
then later bind the actual texture with it through
DetachFrom/AttachToGLContext(). The client could do this with
glBindTexImage2DCHROMIUM(GL_TEXTURE_EXTERNAL_OES, image_id), but you'd have to
register the image with the ImageManager (you can generate the id from the
client I think before you send the IPC to the browser with
GpuChannelHost::ReserveImageId()).

[sivag, 2015-06-25 16:01:43]

On 2015/06/25 00:19:51, sievers wrote:
> On 2015/06/23 15:39:59, sivag wrote:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > When i am trying to access TextureRef* texture  for the created texture id ,
> > texture is coming null.
> > 
> > When the same call is made from renderer to gpu, it works well, while using
> > gl->CreateStreamTextureCHROMIUM(*texture_id);
> > 
> > When i verifed , in both approaches (renderer->gpu,
render->browser->gpu)both
> > the (stub pointers, stream_id, route_id, textureid) every thing is same when
> > calling StreamTexture::Create(this, texture_id, stream_id);
> > 
> > I am currently debugging this problem.
> > Am i missing something here, Any pointers would be helpful?
> 
> Does it work with a Finish() after glGenTextures()?
> The problem is likely that things happen on different IPC channels which are
not
> ordered wrt each other.
> 
> The easiest would be to sidestep the whole problem and not require a texture
> from the client when creating the SurfaceTexture.
> 
> One way to achieve that would be to use the workaround from
> gl_image_surface_texture.cc and create the SurfaceTexture with an id of 0, and
> then later bind the actual texture with it through
> DetachFrom/AttachToGLContext(). The client could do this with
> glBindTexImage2DCHROMIUM(GL_TEXTURE_EXTERNAL_OES, image_id), but you'd have to
> register the image with the ImageManager (you can generate the id from the
> client I think before you send the IPC to the browser with
> GpuChannelHost::ReserveImageId()).

Thanks for the inputs Daniel. After adding glFinish, video plays succesfully. 
Ordering is the issue here.

As you suggested i made the changes in patchset:10, 
As of now AttachToGLContext is not hitting even after calling 
gl->BindTexImage2DCHROMIUM(texture_target, image_id); i am looking into this,
will update you.

[sivag, 2015-06-26 17:52:36]

@Sievers,

The StreamTextureCreation flow is completed as discussed.
I tested it with html5 video, it works well.
ptal..the code patchset:11.

I will upload the streamtexturemap maintenance soon.

Thanks

[no sievers, 2015-06-26 19:58:31]

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

[no sievers, 2015-06-26 20:03:51]

+reveman to take over since i'll be OOO for two weeks

You can also consider just dealing with reimplementing the stream texture
creation and management flow in this CL and deal with actually getting rid of
EstablishPeer in a follow-up then.

[sivag, 2015-06-29 13:37:41]

siva.gunturi@samsung.com changed reviewers:
+ kbr@chromium.org, piman@chromium.org

[sivag, 2015-06-29 13:38:35]

siva.gunturi@samsung.com changed reviewers:
- kbr@chromium.org, piman@chromium.org

[sivag, 2015-06-29 13:39:26]

@reveman, ptal..

[reveman, 2015-06-29 16:02:01]

https://codereview.chromium.org/1129943006/diff/240001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_channel_host_factory.h (right):

https://codereview.chromium.org/1129943006/diff/240001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_channel_host_factory.h:65: 
why blank line here but not below?

https://codereview.chromium.org/1129943006/diff/240001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1129943006/diff/240001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:278: // Tells the GPU process to create a new
stream texture for rendering video*/
nit: remove "*/" at the end

https://codereview.chromium.org/1129943006/diff/240001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:399: // Respond from GPU to a
GpuMsg_CreateViewCommandBuffer message.
Response from...

https://codereview.chromium.org/1129943006/diff/260001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/1129943006/diff/260001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.h:95: int32 stream_id) = 0;
hm, could this be part of the GpuControl interface instead?

We already have CreateImage and adding a CreateStreamTextureImage function seems
reasonable.

[sivag, 2015-06-30 11:17:16]

siva.gunturi@samsung.com changed reviewers:
+ piman@chromium.org

[sivag, 2015-06-30 11:17:18]

ptal..

https://codereview.chromium.org/1129943006/diff/240001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_channel_host_factory.h (right):

https://codereview.chromium.org/1129943006/diff/240001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_channel_host_factory.h:65: 
On 2015/06/29 16:02:00, reveman wrote:
> why blank line here but not below?

Done.

https://codereview.chromium.org/1129943006/diff/240001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1129943006/diff/240001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:278: // Tells the GPU process to create a new
stream texture for rendering video*/
On 2015/06/29 16:02:01, reveman wrote:
> nit: remove "*/" at the end

Corrected in the latest patch.

https://codereview.chromium.org/1129943006/diff/240001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:399: // Respond from GPU to a
GpuMsg_CreateViewCommandBuffer message.
On 2015/06/29 16:02:00, reveman wrote:
> Response from...

Done.

https://codereview.chromium.org/1129943006/diff/260001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/1129943006/diff/260001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.h:95: int32 stream_id) = 0;
On 2015/06/29 16:02:01, reveman wrote:
> hm, could this be part of the GpuControl interface instead?
> 
> We already have CreateImage and adding a CreateStreamTextureImage function
seems
> reasonable. 

As of now we have the CreateStreamTextureImage(int textureid) interface in
gpu_control.h(https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer/client/gpu_control.h&l=80).
But this interacts with the gpu command buffer directly sends a message to the
gpu to create stream texture(
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...)

But in this patch  we first send a host message to browser then forward it to
gpu and still we use the renderer context to do this.

What i mean is though we add it in gpu_control.h, there will be no caller as we
call this from renderer client context via browser?

Please advice on how to proceed here?

[piman, 2015-06-30 23:04:20]

So, I don't see how the browser UI thread is involved. The renderer message is
picked up by the GpuMessageFilter which runs on the browser IO thread, and from
there directly goes to the GpuProcessHost and sends a message to the GPU process
from the IO thread.

So it looks like it's doing something much more complex for no direct benefit?

That said, the part of the patch that makes the flow go through the image
creation path is good. You can see that StreamTexture::Create can't fail any
more (on Android), so the existing GpuCommandBufferMsg_CreateStreamTexture
message could be made async (assuming it passes the image id instead of the
texture id).

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:239: command_buffer_routing_id_ =
route_id;
NAK. A channel has multiple command buffers, and they can be created from
multiple threads. This field doesn't make sense and is racy.

You definitely want to go via GpuControl, which is the CommandBufferProxyImpl,
that has the command buffer route id.

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:199: if (texture_id != texture_id_)
{
nit: this is really if (!texture_id_) because of the test above.

[sivag, 2015-07-01 17:05:13]

On 2015/06/30 23:04:20, piman (Very slow to review) wrote:
> So, I don't see how the browser UI thread is involved. The renderer message is
> picked up by the GpuMessageFilter which runs on the browser IO thread, and
from
> there directly goes to the GpuProcessHost and sends a message to the GPU
process
> from the IO thread.
> 
> So it looks like it's doing something much more complex for no direct benefit?

@piman,
Definitely CommandBufferProxyImpl is the right path to get the routing id and
add the new interface.
The reason for routing the createstreamtexture from (renderer->browser IO ->gpu)
instead of
(renderer->gpu through CommandBufferProxyImpl) is, when we create the stream
texture we want to add the new surface_texture_
and the stream to a map in browser while returning back from gpu, so that we
dont 
have to send a establishpeer to gpu  which as of now (renderer->gpu->browser)
sends a
message to gpu and gpu posts a task to browser with the surface_texture_.
Please share your thoughts on how can we proceed here.

[piman, 2015-07-01 20:22:43]

On Wed, Jul 1, 2015 at 10:05 AM, <siva.gunturi@samsung.com> wrote:

> On 2015/06/30 23:04:20, piman (Very slow to review) wrote:
>
>> So, I don't see how the browser UI thread is involved. The renderer
>> message is
>> picked up by the GpuMessageFilter which runs on the browser IO thread, and
>>
> from
>
>> there directly goes to the GpuProcessHost and sends a message to the GPU
>>
> process
>
>> from the IO thread.
>>
>
>  So it looks like it's doing something much more complex for no direct
>> benefit?
>>
>
> @piman,
> Definitely CommandBufferProxyImpl is the right path to get the routing id
> and
> add the new interface.
> The reason for routing the createstreamtexture from (renderer->browser IO
> ->gpu)
> instead of
> (renderer->gpu through CommandBufferProxyImpl) is, when we create the
> stream
> texture we want to add the new surface_texture_
> and the stream to a map in browser while returning back from gpu, so that
> we
> dont
> have to send a establishpeer to gpu  which as of now
> (renderer->gpu->browser)
> sends a
> message to gpu and gpu posts a task to browser with the surface_texture_.
> Please share your thoughts on how can we proceed here.
>

I see, I guess there's a follow-up to that, not clear from this CL.

You replace somewhat complex routing with somewhat complex synchronization
- I'm not sure it's a net win.
As it is, for CreateStreamTexture, you end up having to use a sync IPC to
the browser & GPU process main thread (which may be blocked by long GL
calls or something). Is there any way to make it asynchronous?

I assume a follow-up patch will change the EstablishPeer flow to go to the
browser, I guess it'd be nice to see that part too. Are there ordering
constraints between EstablishPeer vs the rest of the GL commands or other
channel IPCs? If you go to the browser you'll lose the implicit ordering.

https://codereview.chromium.org/1129943006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sivag, 2015-07-02 10:18:43]

> I see, I guess there's a follow-up to that, not clear from this CL.

$Siva-reply:
------------
Please check patch-set::12 it has the entire flow of adding the streamtexturemap
as well.

> You replace somewhat complex routing with somewhat complex synchronization
> - I'm not sure it's a net win.
> As it is, for CreateStreamTexture, you end up having to use a sync IPC to
> the browser & GPU process main thread (which may be blocked by long GL
> calls or something). Is there any way to make it asynchronous?


$Siva-reply:
------------
In stream_texture_factory_impl.cc with below piece of code from the patch
int32 image_id = channel_->ReserveImageId();  
gl->GenTextures(1, texture_id);  
stream_id = channel_->CreateStreamTexture(image_id);  
gl->BindTexture(texture_target, *texture_id);  
gl->BindTexImage2DCHROMIUM(texture_target, image_id);

I think the calls should be synchronous, 
If we make this asynchronous ordering issues may come
Please check :https://codereview.chromium.org/1129943006/#msg9

> I assume a follow-up patch will change the EstablishPeer flow to go to the
> browser, I guess it'd be nice to see that part too. Are there ordering
> constraints between EstablishPeer vs the rest of the GL commands or other
> channel IPCs? If you go to the browser you'll lose the implicit ordering.

$Siva-reply:
------------
Please check comment https://codereview.chromium.org/1129943006/#msg3.
We want to remove this (EstablishPeer call to gpu.)
With the current patch i didn't observe any ordering issues.

[piman, 2015-07-08 23:30:11]

On 2015/07/02 10:18:43, sivag wrote:
> > I see, I guess there's a follow-up to that, not clear from this CL.
> 
> $Siva-reply:
> ------------
> Please check patch-set::12 it has the entire flow of adding the
streamtexturemap
> as well.
> 
> > You replace somewhat complex routing with somewhat complex synchronization
> > - I'm not sure it's a net win.
> > As it is, for CreateStreamTexture, you end up having to use a sync IPC to
> > the browser & GPU process main thread (which may be blocked by long GL
> > calls or something). Is there any way to make it asynchronous?
> 
> 
> $Siva-reply:
> ------------
> In stream_texture_factory_impl.cc with below piece of code from the patch
> int32 image_id = channel_->ReserveImageId();  
> gl->GenTextures(1, texture_id);  
> stream_id = channel_->CreateStreamTexture(image_id);  
> gl->BindTexture(texture_target, *texture_id);  
> gl->BindTexImage2DCHROMIUM(texture_target, image_id);
> 
> I think the calls should be synchronous, 
> If we make this asynchronous ordering issues may come
> Please check :https://codereview.chromium.org/1129943006/#msg9
> 
> > I assume a follow-up patch will change the EstablishPeer flow to go to the
> > browser, I guess it'd be nice to see that part too. Are there ordering
> > constraints between EstablishPeer vs the rest of the GL commands or other
> > channel IPCs? If you go to the browser you'll lose the implicit ordering.
> 
> $Siva-reply:
> ------------
> Please check comment https://codereview.chromium.org/1129943006/#msg3.
> We want to remove this (EstablishPeer call to gpu.)
> With the current patch i didn't observe any ordering issues.

I would like to discuss the direction with sievers@ when he gets back. I would
like to avoid sync calls (especially since this adds more hops than before).
There's a part of this change which I think is good, which is the part where we
use the image infrastructure, which decouples from the texture id, which removes
failure cases. Can you split this patch into a separate one that only does that
(that I'm happy to review and land now), and the other part that changes the
flow to go to the browser, that I would like to discuss further?

[sivag, 2015-07-09 14:10:50]

On 2015/07/08 23:30:11, piman (Very slow to review) wrote:
> On 2015/07/02 10:18:43, sivag wrote:
> > > I see, I guess there's a follow-up to that, not clear from this CL.
> > 
> > $Siva-reply:
> > ------------
> > Please check patch-set::12 it has the entire flow of adding the
> streamtexturemap
> > as well.
> > 
> > > You replace somewhat complex routing with somewhat complex synchronization
> > > - I'm not sure it's a net win.
> > > As it is, for CreateStreamTexture, you end up having to use a sync IPC to
> > > the browser & GPU process main thread (which may be blocked by long GL
> > > calls or something). Is there any way to make it asynchronous?
> > 
> > 
> > $Siva-reply:
> > ------------
> > In stream_texture_factory_impl.cc with below piece of code from the patch
> > int32 image_id = channel_->ReserveImageId();  
> > gl->GenTextures(1, texture_id);  
> > stream_id = channel_->CreateStreamTexture(image_id);  
> > gl->BindTexture(texture_target, *texture_id);  
> > gl->BindTexImage2DCHROMIUM(texture_target, image_id);
> > 
> > I think the calls should be synchronous, 
> > If we make this asynchronous ordering issues may come
> > Please check :https://codereview.chromium.org/1129943006/#msg9
> > 
> > > I assume a follow-up patch will change the EstablishPeer flow to go to the
> > > browser, I guess it'd be nice to see that part too. Are there ordering
> > > constraints between EstablishPeer vs the rest of the GL commands or other
> > > channel IPCs? If you go to the browser you'll lose the implicit ordering.
> > 
> > $Siva-reply:
> > ------------
> > Please check comment https://codereview.chromium.org/1129943006/#msg3.
> > We want to remove this (EstablishPeer call to gpu.)
> > With the current patch i didn't observe any ordering issues.
> 
> I would like to discuss the direction with sievers@ when he gets back. I would
> like to avoid sync calls (especially since this adds more hops than before).
> There's a part of this change which I think is good, which is the part where
we
> use the image infrastructure, which decouples from the texture id, which
removes
> failure cases. Can you split this patch into a separate one that only does
that
> (that I'm happy to review and land now), and the other part that changes the
> flow to go to the browser, that I would like to discuss further?

Sure piman, we can discuss this with sievers.
I will make a seperate patch for the image part.

[no sievers, 2015-07-13 22:04:56]

I think eventually we want to end up with something like this (without saying
anything about which channel the IPCs have to flow through):

1) the renderer sends an IPC to create the SurfaceTexture (in the GPU process)
and then binds it to a texture
2) in the browser we can get a Surface which is connected to that SurfaceTexture
3) the media player instance in the renderer sends an id to its counterpart in
the browser which identifies the Surface(Texture)

For 2) we need a synchronous Binder call (but it doesn't have to go to the main
thread in the GPU process). 

And maybe we don't need 1) to be synchronous i.e. the renderer to block on this.
In other words, 2) could happen after 3). Then 1) could happen on the
renderer->gpu channel.

[sivag, 2015-07-16 14:24:58]

ptal..

https://codereview.chromium.org/1129943006/diff/260001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/1129943006/diff/260001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.h:95: int32 stream_id) = 0;
On 2015/06/29 16:02:01, reveman wrote:
> hm, could this be part of the GpuControl interface instead?
> 
> We already have CreateImage and adding a CreateStreamTextureImage function
seems
> reasonable. 

Done.

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/cli...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/cli...
content/common/gpu/client/gpu_channel_host.cc:239: command_buffer_routing_id_ =
route_id;
On 2015/06/30 23:04:20, piman (Very slow to review) wrote:
> NAK. A channel has multiple command buffers, and they can be created from
> multiple threads. This field doesn't make sense and is racy.
> 
> You definitely want to go via GpuControl, which is the CommandBufferProxyImpl,
> that has the command buffer route id.

Done.

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1129943006/diff/300001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:199: if (texture_id != texture_id_)
{
On 2015/06/30 23:04:20, piman (Very slow to review) wrote:
> nit: this is really if (!texture_id_) because of the test above.

Done.

[sivag, 2015-07-16 15:06:48]

Apologies, this is having some autogen issue,
I will fix it and let you know.

[sivag, 2015-07-17 14:01:26]

ptal..

Some autogen files are heavily formatted after running the script.This is
probably due to the changes in the script build_gles2_cmd_buffer.py.

[no sievers, 2015-07-20 23:33:08]

https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:439: uint32
CommandBufferProxyImpl::CreateStreamTexture(uint32 texture_id) {
Do we need to keep around the old version or can it all go away now?

https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:455: uint32
CommandBufferProxyImpl::CreateStreamTextureImage(int32 image_id) {
It would be nice if we could return the image_id here instead of passing it in,
so that it works the same as CreateImage().

Maybe we can make |stream_id| internal to StreamTextureHost somehow, otherwise
it's confusing to have two ids in the API.

We can probably do it by just shuffling code around a bit in between
StreamTextureFactory/Proxy and StreamTextureHost (EstablishPeer and SetSize
would go through StreamTextureProxy instead, so outside of proxy/host nobody
needs to know the stream/route id).

[sivag, 2015-07-23 14:36:32]

On 2015/07/20 23:33:08, sievers wrote:
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> content/common/gpu/client/command_buffer_proxy_impl.cc:439: uint32
> CommandBufferProxyImpl::CreateStreamTexture(uint32 texture_id) {
> Do we need to keep around the old version or can it all go away now?
> 
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> content/common/gpu/client/command_buffer_proxy_impl.cc:455: uint32
> CommandBufferProxyImpl::CreateStreamTextureImage(int32 image_id) {
> It would be nice if we could return the image_id here instead of passing it
in,
> so that it works the same as CreateImage().
> 
> Maybe we can make |stream_id| internal to StreamTextureHost somehow, otherwise
> it's confusing to have two ids in the API.
> 
> We can probably do it by just shuffling code around a bit in between
> StreamTextureFactory/Proxy and StreamTextureHost (EstablishPeer and SetSize
> would go through StreamTextureProxy instead, so outside of proxy/host nobody
> needs to know the stream/route id).

I went through the code, below are the points that came to my mind

API-1
-----
we can make stream_id part of StreamTextureHost 
(in constructor channel_->GenerateRouteID() keep it till lifecycle and use it
via proxy if needed in webmediaplayer and else where)
but we may have to pass stream_id to CreateStreamTextureImage, as we may not
generate id 
in command_buffer_proxy_impl call again  we will end up with below signature

/*image_id*/int32_t CreateStreamTextureImage(int32_t stream_id) = 0;

API-2
-----
One approach is to keep code as it is and return StreamInfo
Struct StreamInfo {
 int32 stream_id;
 int32 image_id;
};

the interface can be made simple like 
StreamInfo CreateStreamTextureImage(); 
Only thing is do we want to maintain just the gltypes (GLint etc) as return?

API-3
------
/*image_id*/int32_t CreateStreamTextureImage() = 0;
I cant see any way of implementing this as the routing id can be maintained
by one entity (StreamTextureHost)and has to be passed to the other
entity(CommandbufferProxyImpl) during a call.

If API-3 is not possible to implement, i would prefer API-2.
Please share your thoughts.


in_reply_to: 5662069344960512
send_mail: 1
subject: Implement StreamTexture::BindTexImage.

----------
Please reload the previous page and post again.

[sivag, 2015-08-03 15:13:41]

On 2015/07/20 23:33:08, sievers wrote:
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> content/common/gpu/client/command_buffer_proxy_impl.cc:439: uint32
> CommandBufferProxyImpl::CreateStreamTexture(uint32 texture_id) {
> Do we need to keep around the old version or can it all go away now?
> 
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> content/common/gpu/client/command_buffer_proxy_impl.cc:455: uint32
> CommandBufferProxyImpl::CreateStreamTextureImage(int32 image_id) {
> It would be nice if we could return the image_id here instead of passing it
in,
> so that it works the same as CreateImage().
> 
> Maybe we can make |stream_id| internal to StreamTextureHost somehow, otherwise
> it's confusing to have two ids in the API.
> 
> We can probably do it by just shuffling code around a bit in between
> StreamTextureFactory/Proxy and StreamTextureHost (EstablishPeer and SetSize
> would go through StreamTextureProxy instead, so outside of proxy/host nobody
> needs to know the stream/route id).

hi sievers, any views on how to go about this?
please see my comments in #31.

[no sievers, 2015-08-03 19:18:50]

On 2015/08/03 15:13:41, sivag wrote:
> On 2015/07/20 23:33:08, sievers wrote:
> >
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> > File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> > content/common/gpu/client/command_buffer_proxy_impl.cc:439: uint32
> > CommandBufferProxyImpl::CreateStreamTexture(uint32 texture_id) {
> > Do we need to keep around the old version or can it all go away now?
> > 
> >
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> > content/common/gpu/client/command_buffer_proxy_impl.cc:455: uint32
> > CommandBufferProxyImpl::CreateStreamTextureImage(int32 image_id) {
> > It would be nice if we could return the image_id here instead of passing it
> in,
> > so that it works the same as CreateImage().
> > 
> > Maybe we can make |stream_id| internal to StreamTextureHost somehow,
otherwise
> > it's confusing to have two ids in the API.
> > 
> > We can probably do it by just shuffling code around a bit in between
> > StreamTextureFactory/Proxy and StreamTextureHost (EstablishPeer and SetSize
> > would go through StreamTextureProxy instead, so outside of proxy/host nobody
> > needs to know the stream/route id).
> 
> hi sievers, any views on how to go about this?
> please see my comments in #31.

Agreed with all the comments. The direction of API-1 seems easiest.
I was mainly worried about the two ids leaking into the caller (as in
WebMediaPlayer or so), but actually WMP might not have to know about either
image_ or stream_id - it really only needs the texture_id (we might have to
slightly reroute some calls through factory or proxy).
I think we should also rename stream_id to route_id.

[sivag, 2015-08-04 07:17:40]

On 2015/08/03 19:18:50, sievers wrote:
> On 2015/08/03 15:13:41, sivag wrote:
> > On 2015/07/20 23:33:08, sievers wrote:
> > >
> >
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> > > File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> > > content/common/gpu/client/command_buffer_proxy_impl.cc:439: uint32
> > > CommandBufferProxyImpl::CreateStreamTexture(uint32 texture_id) {
> > > Do we need to keep around the old version or can it all go away now?
> > > 
> > >
> >
>
https://codereview.chromium.org/1129943006/diff/440001/content/common/gpu/cli...
> > > content/common/gpu/client/command_buffer_proxy_impl.cc:455: uint32
> > > CommandBufferProxyImpl::CreateStreamTextureImage(int32 image_id) {
> > > It would be nice if we could return the image_id here instead of passing
it
> > in,
> > > so that it works the same as CreateImage().
> > > 
> > > Maybe we can make |stream_id| internal to StreamTextureHost somehow,
> otherwise
> > > it's confusing to have two ids in the API.
> > > 
> > > We can probably do it by just shuffling code around a bit in between
> > > StreamTextureFactory/Proxy and StreamTextureHost (EstablishPeer and
SetSize
> > > would go through StreamTextureProxy instead, so outside of proxy/host
nobody
> > > needs to know the stream/route id).
> > 
> > hi sievers, any views on how to go about this?
> > please see my comments in #31.
> 
> Agreed with all the comments. The direction of API-1 seems easiest.
> I was mainly worried about the two ids leaking into the caller (as in
> WebMediaPlayer or so), but actually WMP might not have to know about either
> image_ or stream_id - it really only needs the texture_id (we might have to
> slightly reroute some calls through factory or proxy).
> I think we should also rename stream_id to route_id.

I will take this up in a seperate patch. Logged a new issue for the same
http://code.google.com/p/chromium/issues/detail?id=516585.

[sivag, 2016-06-10 11:03:56]

Description was changed from

==========
Implement StreamTexture::BindTexImage.

Currently StreamTexture takes the textureid as parameter and
binds it while creation. Ideally a valid image id can be
returned to the client so that it could then call
glBindTexImage2D(). This implements StreamTexture::BindTexImage flow.


BUG=339191
==========

to

==========
Implement StreamTexture::BindTexImage.

Currently StreamTexture takes the textureid as parameter and
binds it while creation. Ideally a valid image id can be
returned to the client so that it could then call
glBindTexImage2D(). This implements StreamTexture::BindTexImage flow.


BUG=339191
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
==========

[sivag, 2016-09-20 11:01:32]

Description was changed from

==========
Implement StreamTexture::BindTexImage.

Currently StreamTexture takes the textureid as parameter and
binds it while creation. Ideally a valid image id can be
returned to the client so that it could then call
glBindTexImage2D(). This implements StreamTexture::BindTexImage flow.


BUG=339191
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

==========
Implement StreamTexture::BindTexImage.

Make stream_id part of StreamTextureHost 
(in constructor channel_->GenerateRouteID() keep it till lifecycle and use it
via proxy if needed in webmediaplayer and else where)
but we may have to pass stream_id to CreateStreamTextureImage, as we may not
generate id 
in command_buffer_proxy_impl call again  we will end up with below signature.

/*image_id*/int32_t CreateStreamTextureImage(int32_t stream_id) = 0;



BUG=339191
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
==========

[sivag, 2016-09-20 11:02:08]

Description was changed from

==========
Implement StreamTexture::BindTexImage.

Make stream_id part of StreamTextureHost 
(in constructor channel_->GenerateRouteID() keep it till lifecycle and use it
via proxy if needed in webmediaplayer and else where)
but we may have to pass stream_id to CreateStreamTextureImage, as we may not
generate id 
in command_buffer_proxy_impl call again  we will end up with below signature.

/*image_id*/int32_t CreateStreamTextureImage(int32_t stream_id) = 0;



BUG=339191
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

==========
Make stream_id part of StreamTextureHost.

Make stream_id part of StreamTextureHost 
(in constructor channel_->GenerateRouteID() keep it till lifecycle and use it
via proxy if needed in webmediaplayer and else where)
but we may have to pass stream_id to CreateStreamTextureImage, as we may not
generate id 
in command_buffer_proxy_impl call again  we will end up with below signature.

/*image_id*/int32_t CreateStreamTextureImage(int32_t stream_id) = 0;



BUG=339191
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
==========

[sivag, 2016-10-27 09:10:45]

Description was changed from

==========
Make stream_id part of StreamTextureHost.

Make stream_id part of StreamTextureHost 
(in constructor channel_->GenerateRouteID() keep it till lifecycle and use it
via proxy if needed in webmediaplayer and else where)
but we may have to pass stream_id to CreateStreamTextureImage, as we may not
generate id 
in command_buffer_proxy_impl call again  we will end up with below signature.

/*image_id*/int32_t CreateStreamTextureImage(int32_t stream_id) = 0;



BUG=339191
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

==========
Implement StreamTexture::BindTexImage.

Implement StreamTexture::BindTexImage.

Currently StreamTexture takes the textureid as parameter and
binds it while creation. Ideally a valid image id can be
returned to the client so that it could then call
glBindTexImage2D()


BUG=339191

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
==========