WebGL: accelerated texImage2D and texSubImage2D for canvas

WebGL: accelerated texImage2D and texSubImage2D for canvas

Currently, we accelerates subset of texImage2D due to glCopyTextureCHROMIUM limitation.
Otherwise, we readback canvas content from GPU and then upload the pixel to GPU.

glCopyTextureCHROMIUM requires destFormat is GL_RGB/GL_RGBA and level is 0
and destType is GL_UNSIGNED_BYTE.

If we use a temporary texture, whose format is GL_RGBA, level is 0 and type is
GL_UNSIGNED_BYTE, we can copy others from canvas texture to temporary texture
to dest texture. Creating temporary texture is pretty lighter than readback and upload.

In addition, this CL fixes a potential bug that HTMLCanvasElement::copiedImage(FrontBuffer)
returns BackBuffer. It's because HTMLCanvasElement::copiedImage() always returns
FrontBuffer after this CL.

Perf measurement on Intel Haswell i7.
texSubImage2D(Canvas) speeds up by 12 times
93.82 run/s -> 1207.67 run/s	

TEST=Covered by
WebglConformance.conformance_extensions_oes_texture_float_with_canvas
WebglConformance.conformance_extensions_oes_texture_half_float_with_canvas
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_canvas_rgb565
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_canvas_rgba4444
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_canvas_rgba5551
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_webgl_canvas_rgb565
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_webgl_canvas_rgba4444
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_webgl_canvas_rgba5551
virtual/gpu/fast/canvas/webgl/texture-npot.html
virtual/gpu/fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgb565.html
virtual/gpu/fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba4444.html
virtual/gpu/fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba5551.html

BUG=349871, 420357
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187151

[dshwang, 2014-11-27 21:29:55]

dongseong.hwang@intel.com changed reviewers:
+ bajones@chromium.org, junov@chromium.org, kbr@chromium.org, zmo@chromium.org

[dshwang, 2014-11-27 21:31:10]

@kbr, could you review?

This CL makes WebGL never readback and upload from other WebGL and Acccelerated
2D Canvas.

https://codereview.chromium.org/735623003/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/735623003/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:682: Image*
HTMLCanvasElement::copiedImage() const
@junov, now it's fixed, as you mentioned in
https://codereview.chromium.org/758493004/#msg8

[Ken Russell (switch to Gerrit), 2014-12-06 02:27:04]

I'm sorry for taking so long to start reviewing this. It's a fairly large change
and needs thought, and there have been a lot of other reviews recently.

A couple of initial high-level comments:

 - Please double-check that *all* of the affected code paths are covered by the
existing tests. If not, let's extend the WebGL conformance tests.
 - As we introduce WebGL 2.0 support and add sRGB back buffers to
WebGLRenderingContext and DrawingBuffer, this code path will be directly
impacted. Please give a little thought to how this code can be made robust to
changes like that.

Thanks. I'll review this next week.

[dshwang, 2014-12-08 14:08:14]

On 2014/12/06 02:27:04, Ken Russell wrote:
> I'm sorry for taking so long to start reviewing this. It's a fairly large
change
> and needs thought, and there have been a lot of other reviews recently.

Yep, I'm looking forward to reviewing :)

I found this CL fixes https://code.google.com/p/chromium/issues/detail?id=420357
also.
WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_webgl_*
fails in Windows because software fallback needs to use skia ganesh and skia
ganesh uses glBlitFramebuffer, and glBlitFramebufferANGLE generates GL error due
to restriction of color conversion.
This CL make texImage2D(WebGLCanvas) be always accelerated, so the bug
disappears while skia ganesh and gpu process still have this potential bug.

> A couple of initial high-level comments:
> 
>  - Please double-check that *all* of the affected code paths are covered by
the
> existing tests. If not, let's extend the WebGL conformance tests.

All code changes are covered by existing tests.
tex-image-and-sub-image-2d-with-* covers both texImage2D and texSubImage2D.

>  - As we introduce WebGL 2.0 support and add sRGB back buffers to
> WebGLRenderingContext and DrawingBuffer, this code path will be directly
> impacted. Please give a little thought to how this code can be made robust to
> changes like that.

This CL doesn't affect how to support sRGB, because the acceleration code
already exist via CopyTextureCHROMIUM.
This CL just extends the acceleration code to all cases.
In addition, I think we should extend CopyTextureCHROMIUM to handle sRGB target
format in order to support sRGB in WebGL 2.0

[dshwang, 2014-12-08 14:09:24]

https://codereview.chromium.org/735623003/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/735623003/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:682: Image*
HTMLCanvasElement::copiedImage() const
On 2014/11/27 21:31:10, dshwang wrote:
> @junov, now it's fixed, as you mentioned in
> https://codereview.chromium.org/758493004/#msg8

I drop this fix because ImageBitmap still use copiedImage(BackBuffer) :'( I'll
fix it in another CL.

[dshwang, 2014-12-09 14:40:38]

I measure performance on Haswell i7 using Canvas/upload-webgl-to-texture.html
with some change as follow:

texSubImage2D speed up by 12 times
93.82 run/s -> 1207.67 run/s	

+++ b/PerformanceTests/Canvas/upload-webgl-to-texture.html
@@ -36,10 +36,11 @@ function renderWebGL(gl) {
 function preRun() {
     tex = destCtx.createTexture();
     destCtx.bindTexture(destCtx.TEXTURE_2D, tex);
+    destCtx.texImage2D(destCtx.TEXTURE_2D, 0, destCtx.RGBA, destCtx.RGBA,
destCtx.UNSIGNED_BYTE, sourceCanvas3D);
 }
 
 function doRun() {
-    destCtx.texImage2D(destCtx.TEXTURE_2D, 0, destCtx.RGBA, destCtx.RGBA,
destCtx.UNSIGNED_BYTE, sourceCanvas3D);
+  destCtx.texSubImage2D(destCtx.TEXTURE_2D, 0, 0, 0, destCtx.RGBA,
destCtx.UNSIGNED_BYTE, sourceCanvas3D);
 }

[Ken Russell (switch to Gerrit), 2014-12-11 01:11:14]

Thanks for this contribution.

The code changes look good, but I'd like to ask you to please re-review the test
cases in the WebGL conformance suite and make sure that all of the new code
paths are being tested. There are several ASSERTs and ASSERT_NOT_REACHEDs in the
new code, and historically, changes in this area have always triggered such
asserts.

I am sure that there are gaps in the WebGL conformance suite -- for example, I'm
not sure that uploading a 2D or WebGL canvas to a face of a cube map is tested
-- so please help either point out the gaps in the tests, or better yet, submit
pull requests against the WebGL repository on Github enhancing the test suite.
I'll be happy to help you with this.

[dshwang, 2014-12-12 17:30:02]

On 2014/12/11 01:11:14, Ken Russell wrote:
> Thanks for this contribution.

Thank you for good opinion!
 
> The code changes look good, but I'd like to ask you to please re-review the
test
> cases in the WebGL conformance suite and make sure that all of the new code
> paths are being tested.

This CL changes two methods
1. texSubImage2D(Canvas)
It's covered by tex-image-and-sub-image-2d-with-(canvas|webgl)*

2. texImage2D(Canvas, CUBE_MAP)
It's covered by virtual/gpu/fast/canvas/webgl/texture-npot.html only for
accelerated-2d-canvas, not WebGL.

After rethinking, It's desirable to expand
tex-image-and-sub-image-2d-with-(canvas|webgl) to cover CUBE_MAP.
texture-npot.html check CUBE_MAP but I feel it's not enough.

> There are several ASSERTs and ASSERT_NOT_REACHEDs in the
> new code, and historically, changes in this area have always triggered such
> asserts.

I understand your worry. I made some mistakes until now also. However, I believe
aggressive ASSERT is better than slightly return. If it comes out to something
wrong, we can fix them at that time, which makes more solid code base.
I explain why I add all ASSERTs below.


> I am sure that there are gaps in the WebGL conformance suite -- for example,
I'm
> not sure that uploading a 2D or WebGL canvas to a face of a cube map is tested
> -- so please help either point out the gaps in the tests, or better yet,
submit
> pull requests against the WebGL repository on Github enhancing the test suite.
> I'll be happy to help you with this.

Thank you. I'll submit patch to WebGL conformance suite to improve
tex-image-and-sub-image-2d-with-(canvas|webgl)* soon.

https://codereview.chromium.org/735623003/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/735623003/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:3626: targetLevel,
m_unpackPremultiplyAlpha, m_unpackFlipY)) {
Accelerated canvas must succeed it. This fails if gpu context is lost or canvas
2d is not accelereated. We already check isContextLost() above. If it fails, we
must fix this bug, instead of slightly return.

https://codereview.chromium.org/735623003/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:3632: if
(!gl->drawingBuffer()->copyToPlatformTexture(webContext(), targetTexture,
targetInternalformat, targetType,
It cannot fail because DrawingBuffer::copyToPlatformTexture fails only if
Extensions3DUtil::canUseCopyTextureCHROMIUM() fails. This method always passes
good internalFormat, destType and level.

    if (!Extensions3DUtil::canUseCopyTextureCHROMIUM(internalFormat, destType,
level))
        return false;

https://codereview.chromium.org/735623003/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:3662: ASSERT(texture);
validateHTMLCanvasElement() already check texture exists.

https://codereview.chromium.org/735623003/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:3665:
ASSERT(!canvas->renderingContext() || canvas->renderingContext()->is2d());
It's rephrase of above if condition.

[Ken Russell (switch to Gerrit), 2014-12-13 00:22:16]

On 2014/12/12 17:30:02, dshwang wrote:
> Thank you. I'll submit patch to WebGL conformance suite to improve
> tex-image-and-sub-image-2d-with-(canvas|webgl)* soon.

Thanks. Please do help us improve the test coverage.

Given that this passes the existing tests, and given the answers to the above
comments, LGTM.

[dshwang, 2014-12-15 16:23:41]

On 2014/12/13 00:22:16, Ken Russell wrote:
> On 2014/12/12 17:30:02, dshwang wrote:
> > Thank you. I'll submit patch to WebGL conformance suite to improve
> > tex-image-and-sub-image-2d-with-(canvas|webgl)* soon.
> 
> Thanks. Please do help us improve the test coverage.
> 
> Given that this passes the existing tests, and given the answers to the above
> comments, LGTM.

Thank you for reviewing!

I'm changing tex-image-and-sub-image-2d-with-webgl-canvas* to cover cube map in
https://github.com/KhronosGroup/WebGL/pull/811
And I verified this change to pass above changes.

[dshwang, 2014-12-15 16:23:48]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-12-15 16:25:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735623003/20001

[commit-bot: I haz the power, 2014-12-15 16:31:56]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187151