Enable WebGL rendering into ImageCHROMIUM, backed by a scanout buffer to enable overlay suppor…

Enable WebGL rendering into ImageCHROMIUM, backed by a scanout buffer to enable overlay support.

BUG=372449
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174402

[alexst (slow to review), 2014-05-08 18:00:50]

Hi Ken,

This patch is part of my hardware overlay support work. The basic idea is to
change the backing storage for WebGL from texImage2D to a scanout buffer, where
possible.

This buffer will then be bound to texture via EGLImage behind the scenes and can
either be used as a regular texture or presented to the display controller where
supported.

I'd like to keep this behind a #define or a flag (to add some tests) in the
drawing buffer for now since this is still experimental.

Please take a look and tell me what you think.

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

https://codereview.chromium.org/274833002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1037: blink::WGC3Duint newImage =
m_context->createImageCHROMIUM(size.width(), size.height(), GL_RGBA8_OES);
This will have a scanout usage once deps roll.

[alexst (slow to review), 2014-05-09 19:20:27]

Antoine, please have a look. Plumbing overlay buffers into WebGL.

[piman, 2014-05-09 19:34:14]

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

https://codereview.chromium.org/274833002/diff/40001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:150: #ifdef
ENABLE_WEBGL_IMAGE_CHROMIUM
Why an ifdef? Should we, say, turn it on with a command-line flag?

[alexst (slow to review), 2014-05-09 21:23:44]

On 2014/05/09 19:34:14, piman wrote:
>
https://codereview.chromium.org/274833002/diff/40001/Source/platform/graphics...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/274833002/diff/40001/Source/platform/graphics...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:150: #ifdef
> ENABLE_WEBGL_IMAGE_CHROMIUM
> Why an ifdef? Should we, say, turn it on with a command-line flag?

Ah, hast I looked, experimental stuff was still under ifdef. I found the new
way, I'll follow up with content patch to wire it to a command line flag once
this is reviewed.

[piman, 2014-05-09 22:01:03]

lgtm

[Ken Russell (switch to Gerrit), 2014-05-14 22:11:33]

Sorry for the delay. Looks good overall. A couple of thoughts.

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

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1038: if (newImage) {
Under what conditions will createImageCHROMIUM fail? Does the compositor still
render things properly if that happens?

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1040:
m_textureToImageChromiumMap.add(textureId, newImage);
The bookkeeping associated with these ImageChromium objects adds more complexity
to already-complex code. Could you think of a way to encapsulate things more?
Maybe add an inner class to DrawingBuffer which would wrap up the texture ID and
any associated ImageChromium object?

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBufferTest.cpp (right):

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBufferTest.cpp:410:
RuntimeEnabledFeatures::setWebGLImageChromiumEnabled(true);
Does this get set back to false somehow when the test exits? If not, could other
tests break because this flag is set?

https://codereview.chromium.org/274833002/diff/170001/public/platform/WebExte...
File public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/274833002/diff/170001/public/platform/WebExte...
public/platform/WebExternalTextureMailbox.h:39: bool allowOverlay;
Should this be "useOverlay"? "allow" gives the impression that an overlay might
be allocated or might not be allocated.

[alexst (slow to review), 2014-05-14 22:29:54]

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

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1038: if (newImage) {
On 2014/05/14 22:11:33, Ken Russell wrote:
> Under what conditions will createImageCHROMIUM fail? Does the compositor still
> render things properly if that happens?

There may be a limit of memory available to scanout buffers on some hardware,
for example.

But it should fail over gracefully since overlay decisions are made on a per
buffer basis and if no buffer is available above, we'll just make a regular
texture and go on about our business by compositing in GL intead.

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1040:
m_textureToImageChromiumMap.add(textureId, newImage);
On 2014/05/14 22:11:33, Ken Russell wrote:
> The bookkeeping associated with these ImageChromium objects adds more
complexity
> to already-complex code. Could you think of a way to encapsulate things more?
> Maybe add an inner class to DrawingBuffer which would wrap up the texture ID
and
> any associated ImageChromium object?

Let me have a look.

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBufferTest.cpp (right):

https://codereview.chromium.org/274833002/diff/170001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBufferTest.cpp:410:
RuntimeEnabledFeatures::setWebGLImageChromiumEnabled(true);
On 2014/05/14 22:11:33, Ken Russell wrote:
> Does this get set back to false somehow when the test exits? If not, could
other
> tests break because this flag is set?

Good point, I'll force it back off on teardown.

https://codereview.chromium.org/274833002/diff/170001/public/platform/WebExte...
File public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/274833002/diff/170001/public/platform/WebExte...
public/platform/WebExternalTextureMailbox.h:39: bool allowOverlay;
On 2014/05/14 22:11:33, Ken Russell wrote:
> Should this be "useOverlay"? "allow" gives the impression that an overlay
might
> be allocated or might not be allocated.

"allow" is actually closer to reality because we are enabling a possibility of
an overlay. We may not use one if we run out of available planes or if webgl
stops painting (more efficient to not re-scan the same static buffer)

[alexst (slow to review), 2014-05-15 15:54:04]

Please have another look, Ken.

I made texture->image tracking more explicit.

[Ken Russell (switch to Gerrit), 2014-05-20 00:25:11]

Thanks, this looks clearer to me. LGTM

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

https://codereview.chromium.org/274833002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1048:
m_context->destroyImageCHROMIUM(info.imageId);
It might be clearer if this took a non-const TextureInfo* as argument and set
imageId to 0 after calling "destroy".

[alexst (slow to review), 2014-05-20 03:06:36]

+darin for owner review of

Source/web/WebRuntimeFeatures.cpp
public/platform/WebExternalTextureMailbox.h
public/web/WebRuntimeFeatures.h

[darin (slow to review), 2014-05-20 04:15:54]

LGTM

[alexst (slow to review), 2014-05-20 16:03:18]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-20 16:03:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/274833002/210001

[commit-bot: I haz the power, 2014-05-20 16:54:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8829)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8392)

[alexst (slow to review), 2014-05-20 16:57:09]

The CQ bit was unchecked by alexst@chromium.org

[alexst (slow to review), 2014-05-20 17:27:14]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:27:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/274833002/230001

[commit-bot: I haz the power, 2014-05-20 18:59:14]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9014)

[commit-bot: I haz the power, 2014-05-20 20:31:38]

Change committed as 174402