WebGL: Fix not-clear of non-preserveDrawingBuffer.

WebGL: Fix not-clear of non-preserveDrawingBuffer.

Current WebGL relies on glDiscardFramebufferExt() to clear FBO, but the API
doesn't guarantee clearing FBO.

TEST=fast/canvas/webgl/draw-webgl-to-canvas-2d.html,
fast/canvas/webgl/webgl-composite-modes-repaint.html
BUG=423674
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183804

[dshwang, 2014-10-15 09:55:42]

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

[dshwang, 2014-10-15 09:55:43]

Could you review? It fix a bug regressed by
https://codereview.chromium.org/614883002

Currently there are not gpu bots to cover this changes. However, you can check
it in Ubuntu with intel GPU with --use-gl=egl commend line.
./out/Release/content_shell
third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-composite-modes-repaint.html
--use-gl=egl --no-sandbox

[Hongbo Min, 2014-10-15 10:03:57]

On 2014/10/15 09:55:43, dshwang wrote:
> Could you review? It fix a bug regressed by
> https://codereview.chromium.org/614883002
> 
> Currently there are not gpu bots to cover this changes. However, you can check
> it in Ubuntu with intel GPU with --use-gl=egl commend line.
> ./out/Release/content_shell
>
third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-composite-modes-repaint.html
> --use-gl=egl --no-sandbox

@dshwang, thanks for continuous work on this.

Since desktop GPU is not tiled-based, the behaviour of glDiscardFramebufferEXT
might be different from that on mobile device. On a tiled-based GPU, the
glDiscardFramebufferEXT gives a performance hint to GPU driver, and allows the
driver to set pixel value to whatever it wishes. Although
glDiscardFramebufferEXT API doc doesn't say to clear the framebuffer, can we
assume safely it does on mobile device? Again, without calling glClear, we can
get a huge performance boost. 

I am not sure whether it is suitable to test such a mobile specific feature
under desktop environment.

WDYT?

[Hongbo Min, 2014-10-15 10:05:26]

In addition, since glDiscardFramebufferEXT is just a performance hint to GPU
driver, the desktop GPU can ignore it if it doesn't need such a hint.

[dshwang, 2014-10-15 10:24:56]

On 2014/10/15 10:03:57, Hongbo Min wrote:
> On 2014/10/15 09:55:43, dshwang wrote:
> > Could you review? It fix a bug regressed by
> > https://codereview.chromium.org/614883002
> > 
> > Currently there are not gpu bots to cover this changes. However, you can
check
> > it in Ubuntu with intel GPU with --use-gl=egl commend line.
> > ./out/Release/content_shell
> >
>
third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-composite-modes-repaint.html
> > --use-gl=egl --no-sandbox
> 
> @dshwang, thanks for continuous work on this.
> 
> Since desktop GPU is not tiled-based, the behaviour of glDiscardFramebufferEXT
> might be different from that on mobile device. On a tiled-based GPU, the
> glDiscardFramebufferEXT gives a performance hint to GPU driver, and allows the
> driver to set pixel value to whatever it wishes. Although
> glDiscardFramebufferEXT API doc doesn't say to clear the framebuffer, can we
> assume safely it does on mobile device? Again, without calling glClear, we can
> get a huge performance boost. 
> 
> I am not sure whether it is suitable to test such a mobile specific feature
> under desktop environment.
> 
> WDYT?

I understand you want to keep speed-up but the glDiscardFramebufferEXT spec
specified the pixel should be undefined.
https://www.khronos.org/registry/gles/extensions/EXT/EXT_discard_framebuffer.txt
Clearing FBO is just side-effect of the device which you tested. We cann't make
sure all tiled-based mobile GPUs clear FBO by glDiscardFramebufferEXT

On 2014/10/15 10:05:26, Hongbo Min wrote:
> In addition, since glDiscardFramebufferEXT is just a performance hint to GPU
> driver, the desktop GPU can ignore it if it doesn't need such a hint.

As you mentioned, glDiscardFramebufferEXT is just hint, so we cannot replace
glClear with glDiscardFramebufferEXT

kbr@, zmo@, Hongbo WDYT?

[dshwang, 2014-10-15 10:25:09]

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

[Ken Russell (switch to Gerrit), 2014-10-15 23:44:08]

Thanks for catching this. Unfortunate that it slipped through. The
implementation must be correct; performance improvements are secondary. LGTM

[dshwang, 2014-10-16 10:46:08]

On 2014/10/15 23:44:08, Ken Russell wrote:
> Thanks for catching this. Unfortunate that it slipped through. The
> implementation must be correct; performance improvements are secondary. LGTM

Thank you for review.

Hongbo, firstly I land this CL. Let's find the way to get both speed-up and
correctness to take advantage of your technique. Thanks.

[dshwang, 2014-10-16 10:46:10]

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

[commit-bot: I haz the power, 2014-10-16 10:46:35]

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

[commit-bot: I haz the power, 2014-10-16 10:50:01]

Committed patchset #1 (id:1) as 183804

[Hongbo Min, 2014-10-17 01:25:48]

On 2014/10/16 10:46:08, dshwang wrote:
> On 2014/10/15 23:44:08, Ken Russell wrote:
> > Thanks for catching this. Unfortunate that it slipped through. The
> > implementation must be correct; performance improvements are secondary. LGTM
> 
> Thank you for review.
> 
> Hongbo, firstly I land this CL. Let's find the way to get both speed-up and
> correctness to take advantage of your technique. Thanks.

That's OK. lgtm.