gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id, GLsizei width, GLsiz…

gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id, GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id| StreamTexture.

The type of |stream_id| in some code is uint32, and the type in others is int32. Change
the type of |stream_id| to be int32 always because the type of id generator is int32. 

As the decoder knows the correct size of StreamTexture, remove the unnecessary branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531

[dshwang, 2015-11-04 09:52:56]

Description was changed from

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

BUG=549531
==========

to

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

As the decoder knows correct size of StreamTexture, remove unnecessary branch
for StreamTexture in 
GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

[dshwang, 2015-11-04 09:52:56]

dongseong.hwang@intel.com changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[dshwang, 2015-11-04 09:54:35]

sievers, could you review? As we discussed in BUG=549531, this CL introduces
glSetStreamTextureSizeCHROMIUM and removes GpuStreamTextureMsg_SetSize hack

[dshwang, 2015-11-04 09:55:13]

Description was changed from

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

As the decoder knows correct size of StreamTexture, remove unnecessary branch
for StreamTexture in 
GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

to

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

As the decoder knows the correct size of StreamTexture, remove unnecessary the
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

[dshwang, 2015-11-04 09:55:32]

Description was changed from

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

As the decoder knows the correct size of StreamTexture, remove unnecessary the
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

to

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

As the decoder knows the correct size of StreamTexture, remove the unnecessary
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

[dshwang, 2015-11-04 09:58:00]

Description was changed from

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

As the decoder knows the correct size of StreamTexture, remove the unnecessary
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

to

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

Some code uses uint32 for |stream_id|, and others use int32 for |stream_id|.
Change |stream_id|
to be int32 always because the type of id generator is int32. 

As the decoder knows the correct size of StreamTexture, remove the unnecessary
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

[dshwang, 2015-11-04 09:59:46]

Description was changed from

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

Some code uses uint32 for |stream_id|, and others use int32 for |stream_id|.
Change |stream_id|
to be int32 always because the type of id generator is int32. 

As the decoder knows the correct size of StreamTexture, remove the unnecessary
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

to

==========
gpu: introduce glSetStreamTextureSizeCHROMIUM(GLuint texture, GLint stream_id,
GLsizei width, GLsizei height)

Introduce the new CHROMIUM extension to set the size for the given |stream_id|
StreamTexture.

The type of |stream_id| in some code is uint32, and the type in others is int32.
Change
the type of |stream_id| to be int32 always because the type of id generator is
int32. 

As the decoder knows the correct size of StreamTexture, remove the unnecessary
branch for
StreamTexture in GLES2DecoderImpl::Do[Compressed]Copy[Sub]TextureCHROMIUM

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=549531
==========

[dshwang, 2015-11-06 18:11:59]

On 2015/11/04 09:54:35, dshwang wrote:
> sievers, could you review? As we discussed in BUG=549531, this CL introduces
> glSetStreamTextureSizeCHROMIUM and removes GpuStreamTextureMsg_SetSize hack

PTAL?

[no sievers, 2015-11-06 23:15:46]

On 2015/11/06 18:11:59, dshwang wrote:
> On 2015/11/04 09:54:35, dshwang wrote:
> > sievers, could you review? As we discussed in BUG=549531, this CL introduces
> > glSetStreamTextureSizeCHROMIUM and removes GpuStreamTextureMsg_SetSize hack
> 
> PTAL?

Hmm this plumbs a lot of new StreamTexture-specific stuff into the gl/gpu code,
while at the same time there were attempts to eliminate the notion and unify it
further with the GLImage framework.

See:
https://codereview.chromium.org/1385883002/
https://codereview.chromium.org/1129943006/

In other words, it would be nice to not fix WebView by reintroducing an
abstraction in GL (instead of using the factory abstraction).

The other thing is that this still uses an IPC , so how does this change much?

Sorry, can you explain the exact bug again? When the SetSize hacks were first
added I think we were sending the IPC that updates the size before we initiate
the copy. Maybe that changed.

[dshwang, 2015-11-07 15:53:03]

On 2015/11/06 23:15:46, sievers wrote:
> On 2015/11/06 18:11:59, dshwang wrote:
> > On 2015/11/04 09:54:35, dshwang wrote:
> > > sievers, could you review? As we discussed in BUG=549531, this CL
introduces
> > > glSetStreamTextureSizeCHROMIUM and removes GpuStreamTextureMsg_SetSize
hack
> > 
> > PTAL?
> 
> Hmm this plumbs a lot of new StreamTexture-specific stuff into the gl/gpu
code,
> while at the same time there were attempts to eliminate the notion and unify
it
> further with the GLImage framework.
> 
> See:
> https://codereview.chromium.org/1385883002/
> https://codereview.chromium.org/1129943006/

I see but it's desirable to fix bug first before refactoring in general.
glSetStreamTextureSizeCHROMIUM implementation is kind of pair of
glCreateStreamTextureCHROMIUM. All implementation stay near
glCreateStreamTextureCHROMIUM implementation. When glCreateStreamTextureCHROMIUM
is remove, it's very easy to remove glSetStreamTextureSizeCHROMIUM. So I don't
think it adds burden to unification of GLImage framework.

> In other words, it would be nice to not fix WebView by reintroducing an
> abstraction in GL (instead of using the factory abstraction).
> 
> The other thing is that this still uses an IPC , so how does this change much?

I think IPC order may be fine. sorry for confusing

> Sorry, can you explain the exact bug again? When the SetSize hacks were first
> added I think we were sending the IPC that updates the size before we initiate
> the copy. Maybe that changed.

Let me explain on the code. see below comments.

https://codereview.chromium.org/1417853006/diff/40001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.h (left):

https://codereview.chromium.org/1417853006/diff/40001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.h:66: void OnSetSize(const gfx::Size&
size) { size_ = size; }
Current SetSize hack doesn't update the size of texture. it updates only size of
GLImage.

https://codereview.chromium.org/1417853006/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1417853006/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13033: gfx::Size size =
image->GetSize();
As SetSize hack just update size of GLImage, this unnecessary branch is needed.

https://codereview.chromium.org/1417853006/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13220: if
(!source_texture->ValidForTexture(source_texture->target(), 0, x, y, 0,
In addition, this code has bug due to SetSize hack. ValidForTexture() internally
uses size of texture and falsely fires GL_INVALID_VALUE error.

[dshwang, 2015-11-26 15:47:19]

sievers, could you review again?