Allow rendering from non-stream GL_TEXTURE_EXTERNAL_OES

Allow rendering from non-stream GL_TEXTURE_EXTERNAL_OES

Towards support for compositing from GL_TEXTURE_EXTERNAL_OES textures exported
by other drivers into the GL stack.

BUG=167417
TEST=local build, unittests, run on CrOS snow

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

[sheu, 2013-09-18 23:46:19]

piman@: broke this out as you requested.  PTAL

[piman, 2013-09-19 02:20:09]

As I mentioned in the other CL, at least GLES2DecoderImpl::DoCopyTextureCHROMIUM
seems to assume that GL_TEXTURE_EXTERNAL_OES is only supported with stream
textures. Can we fix it for the !stream case?

https://chromiumcodereview.appspot.com/24152009/diff/3001/gpu/command_buffer/...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/3001/gpu/command_buffer/...
gpu/command_buffer/service/texture_manager_unittest.cc:2021: }
nit: blank line after this.

[sheu, 2013-09-19 05:34:21]

I audited all the code paths for GL_TEXTURE_EXTERNAL_OES, IsStreamTexture(), and
LookupStreamTexture().

DoCopyTextureCHROMIUM can't work for pure GL_TEXTURE_EXTERNAL_OES textures,
since it requires a width/height, but imported external textures don't have
those attributes.

https://chromiumcodereview.appspot.com/24152009/diff/3001/gpu/command_buffer/...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/3001/gpu/command_buffer/...
gpu/command_buffer/service/texture_manager_unittest.cc:2021: }
On 2013/09/19 02:20:09, piman wrote:
> nit: blank line after this.

Done.

[sheu, 2013-09-19 05:34:32]

piman@: PTAL

[piman, 2013-09-19 19:42:37]

On Wed, Sep 18, 2013 at 10:34 PM, <sheu@chromium.org> wrote:

> I audited all the code paths for GL_TEXTURE_EXTERNAL_OES,
> IsStreamTexture(), and
> LookupStreamTexture().
>
> DoCopyTextureCHROMIUM can't work for pure GL_TEXTURE_EXTERNAL_OES textures,
> since it requires a width/height, but imported external textures don't have
> those attributes.


Then we have a problem... This is used in production, my understanding is
that it's used for binding a video to webgl and/or canvas.
See WebMediaPlayerImpl::copyVideoTextureToPlatformTexture
and RendererGpuVideoAcceleratorFactories::AsyncReadPixels.

In your case, my understanding is that the texture comes from the video
decoder, right? Can we figure out the size there, and somehow pipe it into
the Texture?


>
>
>
> https://chromiumcodereview.**appspot.com/24152009/diff/**
>
3001/gpu/command_buffer/**service/texture_manager_**unittest.cc<https://chromiumcodereview.appspot.com/24152009/diff/3001/gpu/command_buffer/service/texture_manager_unittest.cc>
> File gpu/command_buffer/service/**texture_manager_unittest.cc (right):
>
> https://chromiumcodereview.**appspot.com/24152009/diff/**
> 3001/gpu/command_buffer/**service/texture_manager_**
>
unittest.cc#newcode2021<https://chromiumcodereview.appspot.com/24152009/diff/3001/gpu/command_buffer/service/texture_manager_unittest.cc#newcode2021>
> gpu/command_buffer/service/**texture_manager_unittest.cc:**2021: }
> On 2013/09/19 02:20:09, piman wrote:
>
>> nit: blank line after this.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/24152009/<https://chromiumcodereview...
>

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

[sheu, 2013-09-19 21:39:37]

On 2013/09/19 19:42:37, piman wrote:
> Then we have a problem... This is used in production, my understanding is
> that it's used for binding a video to webgl and/or canvas.
> See WebMediaPlayerImpl::copyVideoTextureToPlatformTexture
> and RendererGpuVideoAcceleratorFactories::AsyncReadPixels.
> 
> In your case, my understanding is that the texture comes from the video
> decoder, right? Can we figure out the size there, and somehow pipe it into
> the Texture?

Thought about this some more.  We have the size, what's missing is the plumbing
into the Texture.

I'm wondering what the cleanest way to do that is (on the GPU service side).  I
think TextureManager is what we'll be needing to inform of the texture size, but
the manager seems to be a property of the GLES2DecoderImpl and not cleanly
exposed outside.  Alternatively, do some kind of generic implementation of
StreamTextureManager to handle GL_TEXTURE_EXTERNAL_OES textures in general, but
that seems a bit outside the remit of a StreamTextureManager (given that we're
not actually using streaming textures).

Thoughts?

[piman, 2013-09-19 23:18:10]

On Thu, Sep 19, 2013 at 2:39 PM, <sheu@chromium.org> wrote:

> On 2013/09/19 19:42:37, piman wrote:
>
>> Then we have a problem... This is used in production, my understanding is
>> that it's used for binding a video to webgl and/or canvas.
>> See WebMediaPlayerImpl::**copyVideoTextureToPlatformText**ure
>> and RendererGpuVideoAcceleratorFac**tories::AsyncReadPixels.
>>
>
>  In your case, my understanding is that the texture comes from the video
>> decoder, right? Can we figure out the size there, and somehow pipe it into
>> the Texture?
>>
>
> Thought about this some more.  We have the size, what's missing is the
> plumbing
> into the Texture.
>
> I'm wondering what the cleanest way to do that is (on the GPU service
> side).  I
> think TextureManager is what we'll be needing to inform of the texture
> size, but
> the manager seems to be a property of the GLES2DecoderImpl and not cleanly
> exposed outside.


GpuVideoDecodeAccelerator::OnAssignPictureBuffers already uses the
TextureManager (e.g. to map client IDs to service IDs, and also to do
things with sizes, hmm...), so I think it's fair game.


>  Alternatively, do some kind of generic implementation of
> StreamTextureManager to handle GL_TEXTURE_EXTERNAL_OES textures in
> general, but
> that seems a bit outside the remit of a StreamTextureManager (given that
> we're
> not actually using streaming textures).
>
> Thoughts?
>
>
https://chromiumcodereview.**appspot.com/24152009/<https://chromiumcodereview...
>

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

[sheu, 2013-09-24 21:50:21]

Done.

piman@: PTAL

[piman, 2013-09-24 22:14:06]

Overall LGTM, +fischman for GVDA

https://chromiumcodereview.appspot.com/24152009/diff/29001/content/common/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/29001/content/common/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:288: const
std::vector<gfx::Size>& sizes) {
Is it fair to say we don't care about sizes any more? Should we remove it
altogether (including from IPCs)?
Or should we check that they match texture_dimensions_ ?

[sheu, 2013-09-24 22:22:23]

https://chromiumcodereview.appspot.com/24152009/diff/29001/content/common/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/29001/content/common/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:288: const
std::vector<gfx::Size>& sizes) {
On 2013/09/24 22:14:07, piman wrote:
> Is it fair to say we don't care about sizes any more? Should we remove it
> altogether (including from IPCs)?
> Or should we check that they match texture_dimensions_ ?

I do a check against texture_dimensions_ at l.307 below.

Yes, it would be nice to remove it.  Unfortunately, the media::PictureBuffer
struct that we reconstruct in this IPC handler (to pass to further
VideoDecodeAccelerators) has a 'size' field, so we should populate it.  PPAPI
also uses a similar PictureBuffer struct also with a 'size' field -- and that's
a public API.

Do you mean to remove it from just IPC? i.e. don't transmit 'sizes', and just
assume that they are texture_dimensions_ in size, and reconstruct the
media::PictureBuffer appropriately

[piman, 2013-09-24 23:01:31]

https://chromiumcodereview.appspot.com/24152009/diff/29001/content/common/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/29001/content/common/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:288: const
std::vector<gfx::Size>& sizes) {
On 2013/09/24 22:22:23, sheu wrote:
> On 2013/09/24 22:14:07, piman wrote:
> > Is it fair to say we don't care about sizes any more? Should we remove it
> > altogether (including from IPCs)?
> > Or should we check that they match texture_dimensions_ ?
> 
> I do a check against texture_dimensions_ at l.307 below.

Oh, yeah, missed that somehow.

> 
> Yes, it would be nice to remove it.  Unfortunately, the media::PictureBuffer
> struct that we reconstruct in this IPC handler (to pass to further
> VideoDecodeAccelerators) has a 'size' field, so we should populate it.  PPAPI
> also uses a similar PictureBuffer struct also with a 'size' field -- and
that's
> a public API.

Down below, you fill it with texture_dimensions_ anyway (ignoring sizes).

> Do you mean to remove it from just IPC? i.e. don't transmit 'sizes', and just
> assume that they are texture_dimensions_ in size, and reconstruct the
> media::PictureBuffer appropriately

Right, remove it from the IPC, because all we use it for is for checking that
it's the same as what was given to the client in ProvidePictureBuffers. If it's
a client-created texture (GL_TEXTURE_2D path), we check the actual texture
anyway, and if it's a service-created texture (GL_TEXTURE_EXTERNAL_OES path,
either stream or not), the size given by the client is meaningless anyway.


I guess the fact that you have the same interface on the client and service side
makes it sound like you're stuck with a size in PictureBuffer, though it sounds
like we don't need it on the client side. It would be ok to ignore the
PP_PictureBuffer_Dev's size when making the PictureBuffer as a first step, and
as a second step, removing the size from PP_PictureBuffer_Dev, which is possible
by revving the API (this is a Dev interface, we can break it, it's not exposed
to NaCl, so the only client is Flash, that we can fix).

[sheu, 2013-09-25 00:02:05]

Updated to remove extra fields from IPC.

cdn@: trivial gpu_messages.h change for you.

[piman, 2013-09-25 00:27:02]

lgtm

[Ami GONE FROM CHROMIUM, 2013-09-25 01:09:29]

LGTM % comment (fine to submit with those bits reverted, otherwise I need to
understand more)

https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:270: DLOG(ERROR) <<
"BitstreamBuffer id " << id << " out of range";
Why this change, here and elsewhere?  These are effectively DCHECKs, which have
both value as documentation and as last-defense checks on bots.

[piman, 2013-09-25 01:12:05]

https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:270: DLOG(ERROR) <<
"BitstreamBuffer id " << id << " out of range";
On 2013/09/25 01:09:30, Ami Fischman wrote:
> Why this change, here and elsewhere?  These are effectively DCHECKs, which
have
> both value as documentation and as last-defense checks on bots.

This is untrusted data though, coming from the renderer? How can you assert its
values?

[Ami GONE FROM CHROMIUM, 2013-09-25 01:15:46]

https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:270: DLOG(ERROR) <<
"BitstreamBuffer id " << id << " out of range";
On 2013/09/25 01:12:10, piman wrote:
> On 2013/09/25 01:09:30, Ami Fischman wrote:
> > Why this change, here and elsewhere?  These are effectively DCHECKs, which
> have
> > both value as documentation and as last-defense checks on bots.
> 
> This is untrusted data though, coming from the renderer? How can you assert
its
> values?

For debug builds I am more concerned about asserting invariants than defending
against malicious renderers causing the browser to exit.
Do you disagree about the trade-off?

[piman, 2013-09-25 06:35:26]

On Tue, Sep 24, 2013 at 6:15 PM, <fischman@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/24152009/diff/**
>
43001/content/common/gpu/**media/gpu_video_decode_**accelerator.cc<https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu/media/gpu_video_decode_accelerator.cc>
> File content/common/gpu/media/gpu_**video_decode_accelerator.cc (right):
>
> https://chromiumcodereview.**appspot.com/24152009/diff/**
> 43001/content/common/gpu/**media/gpu_video_decode_**
>
accelerator.cc#newcode270<https://chromiumcodereview.appspot.com/24152009/diff/43001/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode270>
> content/common/gpu/media/gpu_**video_decode_accelerator.cc:**270:
> DLOG(ERROR) << "BitstreamBuffer id " << id << " out of range";
> On 2013/09/25 01:12:10, piman wrote:
>
>> On 2013/09/25 01:09:30, Ami Fischman wrote:
>> > Why this change, here and elsewhere?  These are effectively DCHECKs,
>>
> which
>
>> have
>> > both value as documentation and as last-defense checks on bots.
>>
>
>  This is untrusted data though, coming from the renderer? How can you
>>
> assert its
>
>> values?
>>
>
> For debug builds I am more concerned about asserting invariants than
> defending against malicious renderers causing the browser to exit.
> Do you disagree about the trade-off?
>

I do, actually. You can DCHECK this on the client side (it's actually
better since you're closer to the code that generated the invalid data,
hence the stack trace is more likely to be meaningful).
But from the GPU process side, my view is this: you have a set of inputs
for which all values are possible, because it comes from an untrusted side.
Both the meaningful and the non-meaningful input have to be handled
gracefully. It would actually be a good thing to have a test that feeds bad
input and checks that it's handled correctly - a DCHECK prevents that.
But the other thing is that the DCHECK should mean "at this point I verify
the condition is true, so the code below can assume it is, the onus is on
the caller to make sure it holds". It's a form of self-checking
documentation. In this case though, it's not. The DCHECK here seems to mean
"most of the time it should be true, but if it's not, well I have to handle
it anyway". I find it rather dangerous because the next person can come to
the code and see the DCHECK and start assuming the condition is always
true, and write new code that has that assumption. Especially, say, after
some refactoring if this code moves further from IPC, and it's not obvious
that this is security sensitive.


>
https://chromiumcodereview.**appspot.com/24152009/<https://chromiumcodereview...
>

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

[Ami GONE FROM CHROMIUM, 2013-09-25 06:40:44]

OIC DCHECK as a proxy for "it's a programming error if this trigger" is
inappropriate if external (to the GPU process) input can trigger the path.
 I buy that.

LGTM

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

[Cris Neckar, 2013-09-25 18:45:56]

IPC changes LGTM

[commit-bot: I haz the power, 2013-09-25 19:21:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/24152009/43001

[commit-bot: I haz the power, 2013-09-25 21:35:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/24152009/43001

[commit-bot: I haz the power, 2013-09-26 08:59:04]

Change committed as 225410