WebGL: paintRenderingResultsToImageData() must restore FBO.

WebGL: paintRenderingResultsToImageData() must restore FBO.

WebGLRenderingContextBase::paintRenderingResultsToImageData() relies on
DrawingBuffer::paintRenderingResultsToImageData() restoring FBO, but
DrawingBuffer does it only if premultipliedAlpha is true.
paintRenderingResultsToImageData() must restore FBO by itself.

TEST=fast/canvas/webgl/framebuffer-bindings-affected-by-to-data-url.html
BUG=445848
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188059

[dshwang, 2015-01-05 16:32:16]

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

[dshwang, 2015-01-05 16:34:17]

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

[dshwang, 2015-01-05 16:34:18]

@zmo, @bajones, could you review this beta M41 P1 bug? @kbr is ooo now.
@junov, could you give me lgtm after @zmo or @bajones approve it?

[dshwang, 2015-01-05 16:44:27]

https://codereview.chromium.org/805273005/diff/1/Source/core/html/canvas/WebG...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/805273005/diff/1/Source/core/html/canvas/WebG...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:955: if
(!drawingBuffer()->paintRenderingResultsToImageData(width, height, sourceBuffer,
contents))
drawingBuffer()->commit() binds DrawingBuffer::m_fbo but this code rely on
paintRenderingResultsToImageData() restoring DrawingBuffer::m_multisampleFBO or
m_framebufferBinding.

[dshwang, 2015-01-07 19:40:28]

Could any owners review this beta blocker?
This must be resolved until 9th Jan.

following is the mail from Chrome PMO
============================
Time is running out. Our Beta branch for M41 will be cut this Friday January
9th. Please help us by fixing, closing, or punting any blockers that you have
assigned before then. The following blocker is assigned to you: 

445848(Assigned) - Regression: Image on WebGL experiments flickers on clicking
'copy image' context menu.

We greatly appreciate your help!

Chrome PMO

[Justin Novosad, 2015-01-07 20:18:44]

Could you add a test, please?

Reading the bug, I think it is would be very hard to write a layout test or a
chrome UI test for this.  You should at least have a manual test.  Just a simple
page with a WebGL canvas, and some text that says: "When you right click on the
canvas, and select "Save Image As...", the canvas should not flicker.

[Zhenyao Mo, 2015-01-07 20:31:40]

On 2015/01/07 20:18:44, junov wrote:
> Could you add a test, please?
> 
> Reading the bug, I think it is would be very hard to write a layout test or a
> chrome UI test for this.  You should at least have a manual test.  Just a
simple
> page with a WebGL canvas, and some text that says: "When you right click on
the
> canvas, and select "Save Image As...", the canvas should not flicker.

The code itself looks good.

but I leave the final review to junov.

[Justin Novosad, 2015-01-07 20:40:27]

On 2015/01/07 20:31:40, Zhenyao Mo wrote:
> On 2015/01/07 20:18:44, junov wrote:
> > Could you add a test, please?
> > 
> > Reading the bug, I think it is would be very hard to write a layout test or
a
> > chrome UI test for this.  You should at least have a manual test.  Just a
> simple
> > page with a WebGL canvas, and some text that says: "When you right click on
> the
> > canvas, and select "Save Image As...", the canvas should not flicker.
> 
> The code itself looks good.
> 
> but I leave the final review to junov.

Yes code looks good to me too. Just needs a test.

[dshwang, 2015-01-08 11:58:06]

On 2015/01/07 20:40:27, junov wrote:
> On 2015/01/07 20:31:40, Zhenyao Mo wrote:
> > On 2015/01/07 20:18:44, junov wrote:
> > > Could you add a test, please?
> > > 
> > > Reading the bug, I think it is would be very hard to write a layout test
or
> a
> > > chrome UI test for this.  You should at least have a manual test.  Just a
> > simple
> > > page with a WebGL canvas, and some text that says: "When you right click
on
> > the
> > > canvas, and select "Save Image As...", the canvas should not flicker.
> > 
> > The code itself looks good.
> > 
> > but I leave the final review to junov.
> 
> Yes code looks good to me too. Just needs a test.

Thank you for review. I'll add a test soon.

[dshwang, 2015-01-08 15:44:59]

I managed to make a layout test :)
@junov, could you review this beta blocker again?

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

https://codereview.chromium.org/805273005/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:950: return nullptr;
add optimization not to bind fbo unnecessary.

[dshwang, 2015-01-08 15:54:35]

@zmo, framebuffer-bindings-affected-by-to-data-url.html uses only standard api
so it can be applied to webgl conformance test suites
However this bug looks very chromium specific.
Do you think it should be copied to WebGL suites?

[Justin Novosad, 2015-01-08 16:37:49]

LGTM

I am not really familiar with the WebGL conformance suites but I think the test
would probably make a reasonable addition there. Verifying the interaction
between canvas and WebGL APIs should not be out of scope.

[dshwang, 2015-01-08 17:03:10]

On 2015/01/08 16:37:49, junov wrote:
> LGTM
> 
> I am not really familiar with the WebGL conformance suites but I think the
test
> would probably make a reasonable addition there. Verifying the interaction
> between canvas and WebGL APIs should not be out of scope.

Thank you for reviewing. After I listen to @zmo's opinion, who is developer of
WCT, I'll apply it to WCT also.

[dshwang, 2015-01-08 17:03:14]

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

[commit-bot: I haz the power, 2015-01-08 17:03:46]

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

[commit-bot: I haz the power, 2015-01-08 17:10:31]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188059

[Zhenyao Mo, 2015-01-08 17:28:45]

On 2015/01/08 15:54:35, dshwang wrote:
> @zmo, framebuffer-bindings-affected-by-to-data-url.html uses only standard api
> so it can be applied to webgl conformance test suites
> However this bug looks very chromium specific.
> Do you think it should be copied to WebGL suites?

I think it should be added to WebGL conformance test suite.

Can you do that?  The instructions are here:
https://www.khronos.org/webgl/wiki/Using_Github_To_Contribute

[dshwang, 2015-01-08 17:51:15]

On 2015/01/08 17:28:45, Zhenyao Mo wrote:
> On 2015/01/08 15:54:35, dshwang wrote:
> > @zmo, framebuffer-bindings-affected-by-to-data-url.html uses only standard
api
> > so it can be applied to webgl conformance test suites
> > However this bug looks very chromium specific.
> > Do you think it should be copied to WebGL suites?
> 
> I think it should be added to WebGL conformance test suite.
> 
> Can you do that?  The instructions are here:
> https://www.khronos.org/webgl/wiki/Using_Github_To_Contribute

Yes, I can. I have experience already :)

[dshwang, 2015-01-08 18:24:23]

@zmo I submit it to WCT. Could you review
https://github.com/KhronosGroup/WebGL/pull/815 ?

@junov, I need to merge it to beta. I couldn't find any good document. could you
explain or merge?

[dshwang, 2015-01-08 18:29:29]

> @junov, I need to merge it to beta. I couldn't find any good document. could
you
> explain or merge?

never mind. @zmo marked the issue to Merge-Requested. thx.