WebGL: clarify which Front or Back buffer is used by each API.

WebGL: clarify which Front or Back buffer is used by each API.

WebGL has double buffers. Front buffer is the buffer used by the compositor and
back buffer is the target buffer of WebGL. Following list shows what each API needs.
Front buffer
 - Blink renderer, e.g. HTMLCanvasElement::paint(), CSSCanvasValue::image(), Editor::copyImage(), HTMLCanvasElement::imageSourceURL()
 - the Compositor, i.e. DrawingBuffer::prepareMailbox()
Back buffer
 - Canvas, e.g. WebGLRenderingContextBase::texImage2D(canvas),
                CanvasRenderingContext2D::drawImage(canvas),
                CanvasRenderingContext2D::createPattern(canvas),
                HTMLCanvasElement::toDataURL()

First of all, fix the bug of HTMLCanvasElement::imageSourceURL() when premultipliedAlpha == false.
Currently, HTMLCanvasElement::imageSourceURL() always returns back buffer.

Simplify code of texImage2D() and texSubImage2D() as copiedImage() can copy both Front and Back buffer.

TEST=fast/canvas/webgl/canvas-to-data-url.html
BUG=331181
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186032

[dshwang, 2014-11-20 21:14:27]

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

[dshwang, 2014-11-20 21:14:28]

kbr@, could you review this further clean-up?

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

https://codereview.chromium.org/749653002/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:394: // Try to get ImageData first, as
that may avoid lossy conversions.
comment is wrong. buffer() contains not-lossy contents.
Both ImageDataToDataURL() and buffer()->toDataURL() do exactly same encoding.

[Ken Russell (switch to Gerrit), 2014-11-21 01:28:27]

I note there is a test failure on all GPU bots with this CL. Could you please
investigate and resolve it before we review this? Thanks.

[dshwang, 2014-11-21 09:05:40]

After digging into webconformance test failure, I realize
paintRenderingResultsToImageData() is needed when premultipliedAlpha == false.

WebGL uses premultipliedAlpha SkBitmap as buffer. When toDataURL is called,
buffer()->toDataURL() convert from drawingBuffer to SkBitmap and encode SkBitmap
to url.
skia causes inaccurate results due to premultipliedAlpha conversion involved.

I change this CL's goal. "clarify which Front or Back buffer is used by each
API."
In addition, during investigation I found a bug of
HTMLCanvasElement::imageSourceURL(), which is used by hit test, imageloader,
WebView::saveImage. This CL fixes it.

https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (left):

https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:401: RefPtrWillBeRawPtr<ImageData>
imageData = getImageData();
getImageData() is implemented by only webgl, so I put this logic inside is3d if
statement.

https://codereview.chromium.org/749653002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/749653002/diff/20001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:910:
m_context->framebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
GL_TEXTURE_2D, m_frontColorBuffer.texInfo.textureId, 0);
new test, webgl/canvas-to-data-url.html, covers this code.

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

It looks like this new CL breaks several layout tests of Canvas.toDataURL. Could
you please investigate?

[dshwang, 2014-11-22 19:32:18]

Thank you for review. I fixed the broken toDataURL.

https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:404: if (m_size.isEmpty() || !buffer() ||
!m_context)
I changed a behavior of toDataURL without context. I fixed it in the next patch
set.

[Ken Russell (switch to Gerrit), 2014-11-24 02:55:19]

On 2014/11/22 19:32:18, dshwang wrote:
> Thank you for review. I fixed the broken toDataURL.
> 
>
https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
> File Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
> Source/core/html/HTMLCanvasElement.cpp:404: if (m_size.isEmpty() || !buffer()
||
> !m_context)
> I changed a behavior of toDataURL without context. I fixed it in the next
patch
> set.

It looks like toDataURL tests are still failing against the most recent patch
set.

[dshwang, 2014-11-24 14:06:09]

https://codereview.chromium.org/749653002/diff/20001/Source/core/html/HTMLCan...
> It looks like toDataURL tests are still failing against the most recent patch
> set.

Sorry for repeated noise. fast/canvas/webgl/canvas-to-data-url.html, which I add
in this CL, failed because of my mistake. It fixed in new patch set.

https://codereview.chromium.org/749653002/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/749653002/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:411: RefPtrWillBeRawPtr<ImageData>
imageData =
toWebGLRenderingContext(m_context.get())->paintRenderingResultsToImageData(CanvasRenderingContext::Back);
I don't know why I did hard code here. Fixed it.

[Ken Russell (switch to Gerrit), 2014-11-26 00:49:10]

Thanks for doing this cleanup. Nice work in particular on the test. LGTM with a
couple of minor comments.

https://codereview.chromium.org/749653002/diff/80001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/749653002/diff/80001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.h:122: enum SourceBuffer { Front, Back };
There are now three copies of this enum. Could you please refactor it either
into a new header in Source/platform/graphics/ or find another one in that
directory where it could live? HTMLCanvasElement, CanvasRenderingContext and
DrawingBuffer are all allowed to access code in platform/graphics/.

https://codereview.chromium.org/749653002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/749653002/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:920:
m_context->deleteFramebuffer(fbo);
Please bind either the 0 framebuffer or framebuffer() after deleting this
temporary FBO.

[dshwang, 2014-11-26 13:37:30]

Thank you for review. I fix all nits.

https://codereview.chromium.org/749653002/diff/80001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/749653002/diff/80001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.h:122: enum SourceBuffer { Front, Back };
On 2014/11/26 00:49:10, Ken Russell wrote:
> There are now three copies of this enum. Could you please refactor it either
> into a new header in Source/platform/graphics/ or find another one in that
> directory where it could live? HTMLCanvasElement, CanvasRenderingContext and
> DrawingBuffer are all allowed to access code in platform/graphics/.

Nice suggestion. I thought it's not pretty but I don't know why I didn't
refactor it.
I move this enum to GraphicsTypes3D.h because this enum is related to WebGL and
GraphicsTypes.h includes all enums related to canvas 2d.

I change the name to SourceDrawingBuffer because there is already
blink::SourceBuffer class for web audio.

https://codereview.chromium.org/749653002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/749653002/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:920:
m_context->deleteFramebuffer(fbo);
On 2014/11/26 00:49:10, Ken Russell wrote:
> Please bind either the 0 framebuffer or framebuffer() after deleting this
> temporary FBO.

I intentionally don't bind FBO because
WebGLRenderingContextBase::paintRenderingResultsToImageData() restores FBO,
which is only one caller.
However, after reading your comment, I think
DrawingBuffer::paintRenderingResultsToImageData() should clean up by itself. So
I move the FBO restoration code from WebGLRenderingContextBase to DrawingBuffer.

[dshwang, 2014-11-26 13:40:50]

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

[commit-bot: I haz the power, 2014-11-26 13:41:24]

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

[commit-bot: I haz the power, 2014-11-26 14:44:35]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186032

[Yuki, 2014-11-27 04:30:21]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2014-11-27 04:30:22]

https://codereview.chromium.org/749653002/diff/100001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/749653002/diff/100001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:908: GLint fbo =
m_context->createFramebuffer();
Hi dongseong.hwang,

It seems that you're shadowing |fbo| variable here, so the other |fbo| on line
906 will not be updated.  It seems a if-clause on line 918 never be true.

Is this right?  Is this what you intended?

[dshwang, 2014-11-27 09:46:31]

https://codereview.chromium.org/749653002/diff/100001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/749653002/diff/100001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:908: GLint fbo =
m_context->createFramebuffer();
On 2014/11/27 04:30:22, Yuki wrote:
> Hi dongseong.hwang,
> 
> It seems that you're shadowing |fbo| variable here, so the other |fbo| on line
> 906 will not be updated.  It seems a if-clause on line 918 never be true.
> 
> Is this right?  Is this what you intended?

Thank you very much for noting! it's a mistake.