Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

This change will also fix the failure of gpu pixel test for commit() on
Mac because commit() method depends on DrawingBuffer()'s transferToStaticImage()
as well.

Moreover, since the layout test doesn't really compare the composited
result, we should probably turn the above two layout tests into pixel
tests.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa
Cr-Commit-Position: refs/heads/master@{#421181}

[xidachen, 2016-09-23 19:08:31]

Description was changed from

==========
Fix failing transferToImageBitmap layout test

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

BUG=649668
==========

to

==========
Fix failing transferToImageBitmap layout test

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xidachen, 2016-09-23 19:29:16]

Description was changed from

==========
Fix failing transferToImageBitmap layout test

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

This change will also fix the failure of gpu pixel test for commit() on
Mac because commit() method depends on DrawingBuffer()'s transferToStaticImage()
as well.

Moreover, since the layout test doesn't really compare the composited
result, we should probably turn the above two layout tests into pixel
tests.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xidachen, 2016-09-23 20:18:30]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org, kbr@chromium.org

[xidachen, 2016-09-23 20:18:31]

Ken, could you take a look what did wrong here?

The current approach doesn't work. I still see a black box locally on a Mac.

Even if I move this line:
RuntimeEnabledFeatures::setWebGLImageChromiumEnabled(false);

to the commit() function, it still gives me the same result. I am confused.

[Ken Russell (switch to Gerrit), 2016-09-25 02:34:41]

On 2016/09/23 20:18:31, xidachen wrote:
> Ken, could you take a look what did wrong here?
> 
> The current approach doesn't work. I still see a black box locally on a Mac.
> 
> Even if I move this line:
> RuntimeEnabledFeatures::setWebGLImageChromiumEnabled(false);
> 
> to the commit() function, it still gives me the same result. I am confused.

That's definitely not the desired direction. The goal is to leave this turned on
for most DrawingBuffer instances, but allow it to be disabled for individual
instances, when they're created for OffscreenCanvases. Also, I don't know
whether disabling runtime features on the fly actually works.

You should add a new parameter to the create() method like:

  enum ChromiumImageUsage {
    AllowChromiumImage,
    DisallowChromiumImage
  };

and then add a new method to DrawingBuffer like:

  bool shouldUseChromiumImage() {
    return RuntimeEnabledFeatures::webGLImageChromiumEnabled() &&
m_chromiumImageUsage == AllowChromiumImage;
  }

then replace all calls to RuntimeEnabledFeatures::webGLImageChromiumEnabled()
with shouldUseChromiumImage().

You can hack this for the moment to see if it'll work by replacing *all* calls
to RuntimeEnabledFeatures::webGLImageChromiumEnabled() with "false". Don't try
to turn it off like this.

[xidachen, 2016-09-26 15:18:07]

I have tested locally on both Linux and Mac, PS#10 works fine. The bots also
seem to agree with that.

PTAL

[Ken Russell (switch to Gerrit), 2016-09-26 18:21:47]

Mostly looks good. Only one significant comment.

https://codereview.chromium.org/2365653005/diff/200001/content/test/data/gpu/...
File content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_main.html
(right):

https://codereview.chromium.org/2365653005/diff/200001/content/test/data/gpu/...
content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_main.html:11:
<title>OffscreenCanvas transferToImageBitmap on main thread: green and blue
square on white background.</title>
square -> squares

https://codereview.chromium.org/2365653005/diff/200001/content/test/data/gpu/...
File
content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_worker.html
(right):

https://codereview.chromium.org/2365653005/diff/200001/content/test/data/gpu/...
content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_worker.html:11:
<title>OffscreenCanvas transferToImageBitmap on worker thread: green and blue
square on white background.</title>
Same here.

https://codereview.chromium.org/2365653005/diff/200001/content/test/data/gpu/...
content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_worker.html:17:
</style>
This extra closing "style" tag looks wrong.

https://codereview.chromium.org/2365653005/diff/200001/content/test/gpu/page_...
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/2365653005/diff/200001/content/test/gpu/page_...
content/test/gpu/page_sets/pixel_tests.py:194: expectations=expectations))
Could you add these two new pages in __init__, above, instead of _AddAllPages?
The pages in _AddAllPages are supposed to run both with and without
--enable-unsafe-es3-apis, but that requires the use of the incoming
shared_page_state_class rather than overriding it here.

You can just add these right after the call to:

     self._AddAllPages(expectations, base_name, False)

with a comment like

# Pages requiring the use of --enable-experimental-canvas-features.

It'd be great if you could also move the two entries below for
OffscreenCanvasWebGLGreenBox and OffscreenCanvasWebGLRedBoxWorker. Thanks.

(Your new tests don't need to run in both modes, so you don't need the
es3_suffix. You can also remove the test expectations entries for the ES3
variants of your tests.)

[xidachen, 2016-09-26 19:24:03]

Addressed comments. PTAL

https://codereview.chromium.org/2365653005/diff/200001/content/test/gpu/page_...
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/2365653005/diff/200001/content/test/gpu/page_...
content/test/gpu/page_sets/pixel_tests.py:194: expectations=expectations))
On 2016/09/26 18:21:47, Ken Russell wrote:
> Could you add these two new pages in __init__, above, instead of _AddAllPages?
> The pages in _AddAllPages are supposed to run both with and without
> --enable-unsafe-es3-apis, but that requires the use of the incoming
> shared_page_state_class rather than overriding it here.
> 
> You can just add these right after the call to:
> 
>      self._AddAllPages(expectations, base_name, False)
> 
> with a comment like
> 
> # Pages requiring the use of --enable-experimental-canvas-features.
> 
> It'd be great if you could also move the two entries below for
> OffscreenCanvasWebGLGreenBox and OffscreenCanvasWebGLRedBoxWorker. Thanks.
> 
> (Your new tests don't need to run in both modes, so you don't need the
> es3_suffix. You can also remove the test expectations entries for the ES3
> variants of your tests.)

Thank you for pointing it out. That makes perfect sense. In the new patch, I
move all these tests to the __init__ part, and remove the es3 suffix.

[Justin Novosad, 2016-09-26 20:20:37]

lgtm for the tests

[Ken Russell (switch to Gerrit), 2016-09-26 21:47:19]

LGTM with one caveat. Not necessary to block this CL on fixing it.

https://codereview.chromium.org/2365653005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2365653005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6185:
buffer = createDrawingBuffer(std::move(contextProvider),
DrawingBuffer::AllowChromiumImage);
Is this correct in all cases, or does the context restoration path need to check
whether this WebGLRenderingContextBase was created for an OffscreenCanvas? It
looks like there are other places in this code that would need to change, too.

I wonder how dispatch of context lost/restored events would work to an
OffscreenCanvas.

The WEBGL_lose_context extension could be used to test this.

It's at least worth a TODO if not a follow-on bug.

[xidachen, 2016-09-27 02:14:42]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 02:15:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 03:41:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 03:41:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xidachen, 2016-09-27 11:09:21]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 11:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xidachen, 2016-09-27 11:40:16]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-09-27 11:40:21]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-09-27 11:40:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2365653005/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-27 11:40:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 12:31:17]

Description was changed from

==========
Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

This change will also fix the failure of gpu pixel test for commit() on
Mac because commit() method depends on DrawingBuffer()'s transferToStaticImage()
as well.

Moreover, since the layout test doesn't really compare the composited
result, we should probably turn the above two layout tests into pixel
tests.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

This change will also fix the failure of gpu pixel test for commit() on
Mac because commit() method depends on DrawingBuffer()'s transferToStaticImage()
as well.

Moreover, since the layout test doesn't really compare the composited
result, we should probably turn the above two layout tests into pixel
tests.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-09-27 12:31:19]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-09-27 12:32:46]

Description was changed from

==========
Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

This change will also fix the failure of gpu pixel test for commit() on
Mac because commit() method depends on DrawingBuffer()'s transferToStaticImage()
as well.

Moreover, since the layout test doesn't really compare the composited
result, we should probably turn the above two layout tests into pixel
tests.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Fix failing transferToImageBitmap layout tests and commit pixel test on Mac

Right now these two tests:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
are not working properly on Mac if we drag them to the browser. And the
reason is that DrawingBuffer()'s transferToStaticImage() returns a
StaticBitmapImage that is always black because the WebGLImageChromium
flag is enabled.

This CL disable this flag when creating a DrawingBuffer() inside the
WebGLRenderingContextBase's constructor, if the creation is requested
by a offscreenCanvas.

This change will also fix the failure of gpu pixel test for commit() on
Mac because commit() method depends on DrawingBuffer()'s transferToStaticImage()
as well.

Moreover, since the layout test doesn't really compare the composited
result, we should probably turn the above two layout tests into pixel
tests.

BUG=649668
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa
Cr-Commit-Position: refs/heads/master@{#421181}
==========

[commit-bot: I haz the power, 2016-09-27 12:32:47]

Patchset 14 (id:??) landed as
https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa
Cr-Commit-Position: refs/heads/master@{#421181}