New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 649668 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

WebGL's transferToImageBitmap gives black on Mac

Project Member Reported by xidac...@chromium.org, Sep 23 2016

Issue description

We have layout test coverage for this case under:

fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html

Both of them run fine on all platforms.

However, if I just drag these two files to the browser running with "--enable-experimental-canvas-features", I see black squares, with an error message:
gl->CreateAndConsumeTextureCHROMIUM: invalid target.

I suspect something is wrong with DrawingBuffer::transferToStaticBitmapImage(), which is likely a mistake that I made earlier.

Note that this happens on Mac only, windows, linux and android are fine.
 
Cc: ccameron@chromium.org
More investigation:

I put some code in WebGLRenderingContextBase::transferToImageBitmapBase(), to do a gpu readback on the SkImage, and the pixel values are red and blue when I do "--use-gl=osmesa", but the pixel values are always (0, 0, 0, 255) when "--use-gl=desktop".
loop in danakj@.

I looked at DrawingBuffer::transferToStaticBitmapImage(), at line 451, the textureId I got is 1 and 3 when I drag this file to browser:
fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html

which indicates that the textureId is valid. Could there be something wrong with prepareTextureMailboxInternal()?
Cc: danakj@chromium.org
More investigation:

apparently this seems to be a regression. I did bisect on Mac, and it is pointing to this CL: https://codereview.chromium.org/2085573002, which removes this line on Mac:
enable_web_gl_image_chromium = false;

So I tried to run chromium with --disable-webgl-image-chromium, and then everything is fine, the layout test is correct, the gpu pixel tests for WebGL's commit() API run fine under Mac.

kbr@: I don't think we should put that line back because it will introduce more bugs, but I am not sure what should be the right fix.

Comment 5 by kbr@chromium.org, Sep 23 2016

Cc: erikc...@chromium.org
On macOS, DrawingBuffer is using an IOSurface as its backing storage -- this is the WebGLImageChromium path that erikchen@ added. This allows WebGL-rendered canvases to be composited by the OS rather than Chrome.

IOSurfaces are only compatible with the GL_TEXTURE_RECTANGLE_ARB binding target, unfortunately, and this has required some specialization of some of the code.

I think that the best approach here is to go down the old, non-WebGLImageChromium, code path for OffscreenCanvases. This will avoid the knowledge of GL_TEXTURE_RECTANGLE_ARB type textures being introduced into more areas of the code. While this means that the destination canvas for the OffscreenCanvas will have to be composited by Chrome on macOS, I think that's reasonable, and it could be optimized later if it becomes a bottleneck.

Does this explain what's going on? We want to instantiate the DrawingBuffer just for the WebGL-rendered OffscreenCanvas with the WebGLImageChromium code path disabled. I think it's reasonable to do this for all operating systems, not just Mac.

kbr's proposal sounds good.

Notes for myself:
DrawingBuffer::transferToStaticBitmapImage returns a AcceleratedStaticBitmapImage, which is just a wrapper around a texture-backed SkImage. Skia doesn't handle rectangle textures [yet/ever?], so there's no way to pass through an IOSurface. 

Based on https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas, it seems like the main point of an OffscreenCanvas is to do some rendering, and then do something with the result that isn't just display it on the screen. [pass ImageContext to another canvas, save to Blob, etc.]. As such, it's not particularly important to use an IOSurface for the backing storage.


Project Member

Comment 7 by bugdroid1@chromium.org, Sep 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ca72b27d628adf45c298f313869f7b20a7e83aa

commit 3ca72b27d628adf45c298f313869f7b20a7e83aa
Author: xidachen <xidachen@chromium.org>
Date: Tue Sep 27 12:30:58 2016

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

Review-Url: https://codereview.chromium.org/2365653005
Cr-Commit-Position: refs/heads/master@{#421181}

[add] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_main.html
[add] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/content/test/data/gpu/pixel_offscreenCanvas_transferToImageBitmap_worker.html
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/content/test/gpu/gpu_tests/pixel_expectations.py
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/content/test/gpu/page_sets/pixel_tests.py
[delete] https://crrev.com/e6a38be833c08272f2aa356601ff663078876553/third_party/WebKit/LayoutTests/fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-expected.html
[delete] https://crrev.com/e6a38be833c08272f2aa356601ff663078876553/third_party/WebKit/LayoutTests/fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker-expected.html
[delete] https://crrev.com/e6a38be833c08272f2aa356601ff663078876553/third_party/WebKit/LayoutTests/fast/canvas/webgl/offscreenCanvas-transferToImageBitmap-in-worker.html
[delete] https://crrev.com/e6a38be833c08272f2aa356601ff663078876553/third_party/WebKit/LayoutTests/fast/canvas/webgl/offscreenCanvas-transferToImageBitmap.html
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp
[modify] https://crrev.com/3ca72b27d628adf45c298f313869f7b20a7e83aa/third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1d6bec29e841217f8fef71aaacad6a458d4a1e5

commit a1d6bec29e841217f8fef71aaacad6a458d4a1e5
Author: xidachen <xidachen@chromium.org>
Date: Tue Sep 27 16:53:01 2016

Remove some failure entry in gpu pixel test expectations

In a previous CL: https://codereview.chromium.org/2365653005/, we fixed
gpu pixel test for WebGL's commit API on Mac, so we mark those tests as
failure. We have also added two tests to transferToImageBitmap API in the
same CL. Now that the CL has been picked up by all GPU bots, and I have
checked all reference images, it is time to remove the failure entry.

TBR=kbr@chromium.org, zmo@chromium.org, bajones@chromium.org
BUG= 649668 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2376533003
Cr-Commit-Position: refs/heads/master@{#421235}

[modify] https://crrev.com/a1d6bec29e841217f8fef71aaacad6a458d4a1e5/content/test/gpu/gpu_tests/pixel_expectations.py

Status: Fixed (was: Assigned)

Sign in to add a comment