Addition of unpremultiply setting to GL_CHROMIUM_copy_texture.

Addition of an unpremultiplied-alpha setting for the GL_CHROMIUM_copy_texture command buffer GL extension.

The extension needs to support the ability to unpremultiply the source of the copy because of the canvas2d-webgl texture use case.  Canvas is stored in premultiplied form, yet WebGL expects texture contents to be in unpremultiplied form.

Change also includes conversion of argument types to overriders of WebGraphicsContext3D::copyTextureCHROMIUM(...).  
This change is necessary as a result of a wk-change:  https://bugs.webkit.org/show_bug.cgi?id=86275

BUG=101051
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142962

[Jeff Timanus, 2012-06-14 20:36:33]

PTAL.

One thing that concerns me with this change is the behaviour for alpha=0 when
unpremultiplying.  Do we need to have a conditional jump on 0 alpha pixels?

[apatrick_chromium, 2012-06-14 21:06:29]

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:30: const int
kNumShaders = 6;
7

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:111:
gl_FragColor.rgb /= gl_FragColor.a;
GLSL ES spec says:

Treatment of conditions such as divide by 0 may lead to an unspecified result,
but must not lead to the interruption or termination of processing.

Is that okay?

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:121:
TEST_F(GLCopyTextureCHROMIUMTest, UnpremultiplyAlpha) {
How about the unpremultiply flip y variant as well?

[Jeff Timanus, 2012-06-14 22:54:14]

Thx for the review.

I'm concerned that I did not have to make any modifications to the unit test
expectations in gles2_cmd_decoder_unittest*, yet I am able to run gpu_unittests
without any failures.  Shouldn't I need to add expectations for the extra shader
construction GL calls?

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:30: const int
kNumShaders = 6;
On 2012/06/14 21:06:29, apatrick_chromium wrote:
> 7

Done.

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:111:
gl_FragColor.rgb /= gl_FragColor.a;
On 2012/06/14 21:06:29, apatrick_chromium wrote:
> GLSL ES spec says:
> 
> Treatment of conditions such as divide by 0 may lead to an unspecified result,
> but must not lead to the interruption or termination of processing.
> 
> Is that okay?
I think that's ok.  Unpremultiplying a buffer with 0-alpha pixels is technically
undefined.

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:121:
TEST_F(GLCopyTextureCHROMIUMTest, UnpremultiplyAlpha) {
On 2012/06/14 21:06:29, apatrick_chromium wrote:
> How about the unpremultiply flip y variant as well?

Done.

[apatrick_chromium, 2012-06-14 23:43:18]

On 2012/06/14 22:54:14, Jeff Timanus wrote:
> Thx for the review.
> 
> I'm concerned that I did not have to make any modifications to the unit test
> expectations in gles2_cmd_decoder_unittest*, yet I am able to run
gpu_unittests
> without any failures.  Shouldn't I need to add expectations for the extra
shader
> construction GL calls?
> 
>
https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:30: const int
> kNumShaders = 6;
> On 2012/06/14 21:06:29, apatrick_chromium wrote:
> > 7
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:111:
> gl_FragColor.rgb /= gl_FragColor.a;
> On 2012/06/14 21:06:29, apatrick_chromium wrote:
> > GLSL ES spec says:
> > 
> > Treatment of conditions such as divide by 0 may lead to an unspecified
result,
> > but must not lead to the interruption or termination of processing.
> > 
> > Is that okay?
> I think that's ok.  Unpremultiplying a buffer with 0-alpha pixels is
technically
> undefined.
> 
>
https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/tes...
> File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10535174/diff/1/gpu/command_buffer/tes...
> gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:121:
> TEST_F(GLCopyTextureCHROMIUMTest, UnpremultiplyAlpha) {
> On 2012/06/14 21:06:29, apatrick_chromium wrote:
> > How about the unpremultiply flip y variant as well?
> 
> Done.

I deferred the initialization of the CopyTextureCHROMIUMResourceManager until it
is first used because linking the programs took a fair amount of time so unless
the gpu unit tests actually use the extension the initialization code won't run.

I'll hand this over to Gregg. LGTM for my part.

[greggman, 2012-06-15 00:11:45]

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:111:
gl_FragColor.rgb /= gl_FragColor.a;
IF the spec actually says that can terminate maybe you should check  as in

if (gl_FragColor.a > 0) ...

??

Otherwise you'll have an uninitialized texture if this is a new texture.

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6367: case
GL_UNPACK_PREMULTIPLY_ALPHA_CHROMIUM:
When you add a new type here you also need to add a new type to
GLES2DecoderImpl::GetHelper and also the GLState list in
build_gles2_cmd_buffer.py

Did we forget the previous ones?

Should probably add a test

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6369:
unpack_unpremultiply_alpha_ =
This logic seems strange to me. 

If I go pre=1, un=1, un=0 now pre = 0 but I, as a user, never explicitly set it
to 0

Do you want to just set to the 2 flags here and decide which takes precedence
when you apply them?

https://chromiumcodereview.appspot.com/10535174/diff/13001/webkit/gpu/webgrap...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/13001/webkit/gpu/webgrap...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:1676: {
Does this function need an ASSERT_NOT_REACHED(); ? I don't know what the
standard thing to do is here.

[Jeff Timanus, 2012-06-18 14:50:24]

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:111:
gl_FragColor.rgb /= gl_FragColor.a;
On 2012/06/15 00:11:45, greggman wrote:
> IF the spec actually says that can terminate maybe you should check  as in
> 
> if (gl_FragColor.a > 0) ...
> 
> ??
> 
> Otherwise you'll have an uninitialized texture if this is a new texture.

Straight from the spec:  
Similarly, treatment of conditions such as divide by 0 may lead to an
unspecified result, but in no case should such a
condition lead to the interruption or termination of processing.

I'll add the conditional, to prevent undefined textures from floating through
the system.

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6367: case
GL_UNPACK_PREMULTIPLY_ALPHA_CHROMIUM:
On 2012/06/15 00:11:45, greggman wrote:
> When you add a new type here you also need to add a new type to
> GLES2DecoderImpl::GetHelper and also the GLState list in
> build_gles2_cmd_buffer.py
> 
> Did we forget the previous ones?
> 
> Should probably add a test

I think a few settings are missing from GetHelper:
GL_UNPACK_FLIP_Y_CHROMIUM
GL_UNPACK_PREMULTIPLY_ALPHA_CHROMIUM
GL_PACK_REVERSE_ROW_ORDER_ANGLE

These are also missing from the GLState list.

Adding them.

https://chromiumcodereview.appspot.com/10535174/diff/13001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6369:
unpack_unpremultiply_alpha_ =
On 2012/06/15 00:11:45, greggman wrote:
> This logic seems strange to me. 
> 
> If I go pre=1, un=1, un=0 now pre = 0 but I, as a user, never explicitly set
it
> to 0
> 
> Do you want to just set to the 2 flags here and decide which takes precedence
> when you apply them?

Yeah, I mulled how to best handle this situation.  Maybe the simplest (and most
obvious) solution would be set the precedence such that if both are enabled,
then they nullify each other.

https://chromiumcodereview.appspot.com/10535174/diff/13001/webkit/gpu/webgrap...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

https://chromiumcodereview.appspot.com/10535174/diff/13001/webkit/gpu/webgrap...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:1676: {
On 2012/06/15 00:11:45, greggman wrote:
> Does this function need an ASSERT_NOT_REACHED(); ? I don't know what the
> standard thing to do is here.

Following in the example of
WebGraphicsContext3DInProcessImpl::copyTextureToParentTextureCHROMIUM, I added
NOTIMPLEMENTED().

[greggman, 2012-06-18 16:07:57]

lgtm

[commit-bot: I haz the power, 2012-06-19 14:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/10535174/15001

[commit-bot: I haz the power, 2012-06-19 15:43:19]

Change committed as 142962