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

commit c36340833135933c5934ece592fb41b82743e2d8
Author: xiangze.zhang <xiangze.zhang@intel.com>
Date: Wed Feb 22 11:41:10 2017

Make disabling 2d canvas GPU acceleration for getImageData less aggressive.

Make canvas switch out of gpu acceleration only when getImageData is
called in successive frames. This will avoid disabling gpu acceleration
when getImageData is only called once or is called very rarely.

BUG= 690122 

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

[modify] https://crrev.com/c36340833135933c5934ece592fb41b82743e2d8/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/c36340833135933c5934ece592fb41b82743e2d8/third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h
[modify] https://crrev.com/c36340833135933c5934ece592fb41b82743e2d8/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
[modify] https://crrev.com/c36340833135933c5934ece592fb41b82743e2d8/third_party/WebKit/Source/platform/graphics/ImageBuffer.h

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

commit c9b8e77314a6de607cddd12813265638d13b5355
Author: kbr <kbr@chromium.org>
Date: Fri Feb 24 22:47:30 2017

Revert of Make disabling 2d canvas GPU acceleration for getImageData less aggressive. (patchset #4 id:60001 of https://codereview.chromium.org/2687073003/ )

Reason for revert:
Caused  http://crbug.com/695471  -- regressions of 2D/WebGL canvas test on Linux AMD.

Original issue's description:
> Make disabling 2d canvas GPU acceleration for getImageData less aggressive.
>
> Make canvas switch out of gpu acceleration only when getImageData is
> called in successive frames. This will avoid disabling gpu acceleration
> when getImageData is only called once or is called very rarely.
>
> BUG= 690122 
>
> Review-Url: https://codereview.chromium.org/2687073003
> Cr-Commit-Position: refs/heads/master@{#451998}
> Committed: https://chromium.googlesource.com/chromium/src/+/c36340833135933c5934ece592fb41b82743e2d8

TBR=junov@chromium.org,xiangze.zhang@intel.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 690122 

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

[modify] https://crrev.com/c9b8e77314a6de607cddd12813265638d13b5355/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/c9b8e77314a6de607cddd12813265638d13b5355/third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h
[modify] https://crrev.com/c9b8e77314a6de607cddd12813265638d13b5355/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
[modify] https://crrev.com/c9b8e77314a6de607cddd12813265638d13b5355/third_party/WebKit/Source/platform/graphics/ImageBuffer.h

Attached are all of the logs from the failing webgl_conformance_tests run from this build:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28AMD%20R7%20240%29/builds/1591

on this bot:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28AMD%20R7%20240%29/

Here are the key lines from the log:

WebglConformance_conformance_canvas_canvas_test (gpu_tests.webgl_conformance_integration_test.WebGLConformanceIntegrationTest) ... [18221:18221:0222/065042.185014:ERROR:gl_surface.cc(137)] Not implemented reached in virtual gl::GLSurfaceFormat gl::GLSurface::GetFormat()
[18221:18221:0222/065042.257735:ERROR:gl_surface.cc(137)] Not implemented reached in virtual gl::GLSurfaceFormat gl::GLSurface::GetFormat()
[18174:18174:0222/065042.430179:INFO:CONSOLE(11)] "unable to fill 2d context.", source:  (11)
[18174:18174:0222/065042.430488:INFO:CONSOLE(11)] "FAIL unable to fill 2d context.", source:  (11)


Either the 2D canvas fill operation failed or the getImageData operation did.

I've added more logging to this test in https://github.com/KhronosGroup/WebGL/pull/2320 to try to diagnose what's going on. Xiangze, please work with your colleagues to roll forward the WebGL conformance tests using src/tools/roll_webgl_conformance.py ; then the logs will have that information if you re-land. But it would be best if you can try to first guess what might be going wrong. I would suspect the getImageData path is what was broken by your patch. Note that our Linux AMD machine is a pretty recent card and driver.

Deleted: stdout.txt 2.0 MB

stdout.txt 2.0 MB View Download

I finally reproduced the webgl conformance test failure using an AMD HD 6570 card. After some debugging, I think this should be a gpu driver issue. The following is my finding.

1. The test does not fail when using default open source mesa driver. It only fails when I installed AMD Catalyst 15.201.1151 driver. (OS is Ubuntu 14.04)

I also do not see the failure using AMD RX 480 card with amdgpu driver. (OS is Ubuntu 16.04)

2. The failure is related to fillRect. The test fails only when fillRect covers the whole 2d canvas. In this situation, skia uses glclear to draw the rect. If I change the test case to not fill the whole canvas, or make skia not to use glclear to draw the rect, the test does not fail anymore.

3. Skia has a workaround of a driver bug that "some drivers will ignore a clear if it is the only thing rendered to a target before the target is read" ( https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrRenderTargetContext.cpp?rcl=9c10df3b60f4a7d50c1070a5d8c4aaadb79ba9b7&l=366 ). This situation is very similar to this test case. So maybe amd catalyst driver has the same issue.

@#5: Thank you for your in-depth investigation.  For chrome, it would probably best not to rely on skia's driver bug workaround framework. Instead we should apply a fix to chromium's central gpu infrastructure.  I see that there is already a workaround for a known driver bug called "gl_clear_broken".  It is possible that the existing workaround would fix your problem.  If you're lucky, all you have to do is add an entry to src/gpu/config/gpu_driver_bug_list_json.cc
to identify the AMD Catalyst driver as having this bug.

The workaround for gl_clear_broken does not work on Linux AMD. I think VAO is needed if we want to use this workaround. A previous patch tried to use VAO(https://codereview.chromium.org/2080943002), but it is reverted.

AMD driver also fail to compile the shaders of the workaround with error "Implicit version number 110 not supported by GL3 forward compatible context".

@piman, cwallez, zmo:  Any ideas what to do here? (See comments 5-7)

Can you try in skia where the clear caused the problem and just clear again and see if it fixes your problem?

In general, we will want to reduce this to a smaller test case so we could file a driver bug or figure out a workaround.

Although we don't blacklist AMD linux drivers, but they are known to have bad conformance.  We simply decided to allow users to play it rather than forbidden it, assuming whoever uses AMD on their linux knows their way around.

From #7 it sounds like the shaders for the gl_clear_broken workaround need to be updated when running on the Core Profile. xiangze@, if you have already identified those shaders then it would be helpful if you'd post a pull request updating them if running in this configuration, while leaving them alone if running on the Compatibility Profile.

#9: Clear again does not fix this problem.

#11: I think gl_clear_broken workaround is not meant to used on Linux before. Only add shader version and make it compile is not enough, VAO is also needed in order to run on the Core Profile.

I see there were already some failures in AMD Linux webgl conformance test and they are suppressed in webgl_conformance_expectations.py. Maybe we can also suppress this one? 

Xiangze, I'm sorry, I don't support simply suppressing this failure. The nature of the failure is very strange, and indicates that the interoperability path between accelerated 2D canvas and WebGL is broken with your change to 2D canvas's acceleration heuristics. This must be diagnosed.

My change only exposed the driver bug. Disabling GetImageDataForcesNoAcceleration heuristic can also make this test fail.

Can we use the workaround in skia? The workaround in skia can fix this issue properly. If not, I'll try to make gl_clear_broken workaround work on core profile context.

Skia already has the workaround for the glclear bug for several drivers.Reference the code snippet in the file GrRenderTargetContext.cpp:
    if (fContext->caps()->useDrawInsteadOfClear()) {
        // This works around a driver bug with clear by drawing a rect instead.
        // The driver will ignore a clear if it is the only thing rendered to a
        // target before the target is read.
        SkIRect clearRect = SkIRect::MakeWH(this->width(), this->height());
        if (isFull) {
            this->discard();
        } else if (!clearRect.intersect(clip.scissorRect())) {
            return;
        }

        GrPaint paint;
        paint.setColor4f(GrColor4f::FromGrColor(color));
        paint.setXPFactory(GrPorterDuffXPFactory::Get(SkBlendMode::kSrc));

        this->drawRect(clip, std::move(paint), GrAA::kNo, SkMatrix::I(), SkRect::Make(clearRect));
    }

This test case just runs into the situation only issues a glClear command before pixel read out. So it seems that it will be easy to workaround in Skia?

If we want to workaround it in chromium, we need also VAO patch(developed by @cwallez but reverted, not sure how much efforts if we want to make it work perfectly. @cwallez, can you share your comments?)

Jin, Xiangze: if Skia is responsible for doing this glClear call, and not for example GLES2DecoderImpl::DoClear, then it does sound acceptable to use Skia's fUseDrawInsteadOfClear workaround in src/third_party/skia/src/gpu/gl/GrGLCaps.cpp .

You'd need to adjust that code to set the workaround. Please limit the workaround to be used in as few cases as possible -- specifically, just Linux with AMD's Catalyst driver. That will be a Skia CL, so please work with bsalomon@ and others on the Skia team to get that landed and rolled in. Then you could try your https://codereview.chromium.org/2687073003 again.

Skia doesn't see the underlying driver's GL strings (glGetString) but rather the command buffer's strings. So Chrome code must explicitly enable the workaround. The way to expose the workaround through the Skia API is to add a bool to GrContextOptions and then update GrCaps::applyOptionsOverrides to set GrCaps::fUseDrawInsteadOfClear to true when the option is set.

Thanks Brian for the pointer. Jin, Xiangze, could you try making the necessary code changes to both Chromium and Skia (adding a GPU driver bug workaround to src/gpu/config/gpu_driver_bug_list_json.cc), make sure they work on your setup, and then submitting patches first to Skia and then to Chromium to enable this driver bug workaround there?

Thanks for your suggestions. We will try to submit patches to enable this workaround.

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

commit 6ca5d0d3ad671604d9a028e71402e9e5ee25f110
Author: xiangze.zhang <xiangze.zhang@intel.com>
Date: Wed Mar 29 05:05:19 2017

Make gl_clear_broken workaround support core profile and use it under AMD Linux Catalyst driver

Make ClearFrameBuffer use a VAO and add version to shaders in core
profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
because it ignores clear if it's the only thing rendered to the target
before the target is read.

BUG= 690122 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

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

[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_clear_framebuffer.h
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_decoder.h
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_decoder_mock.h
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.h
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/command_buffer/tests/gl_clear_framebuffer_unittest.cc
[modify] https://crrev.com/6ca5d0d3ad671604d9a028e71402e9e5ee25f110/gpu/config/gpu_driver_bug_list_json.cc

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

commit e618b6971a699f5d25d9f1b8c5d3011b8118feab
Author: sugoi <sugoi@chromium.org>
Date: Thu Mar 30 15:41:00 2017

Revert of Make gl_clear_broken workaround support core profile and use it under AMD Linux Catalyst driver (patchset #7 id:120001 of https://codereview.chromium.org/2764833003/ )

Reason for revert:
This cl broke the "Builder: Linux Release (AMD R7 240)" bot. See:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20(AMD%20R7%20240)

Original issue's description:
> Make gl_clear_broken workaround support core profile and use it under AMD Linux Catalyst driver
>
> Make ClearFrameBuffer use a VAO and add version to shaders in core
> profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
> because it ignores clear if it's the only thing rendered to the target
> before the target is read.
>
> BUG= 690122 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
>
> Review-Url: https://codereview.chromium.org/2764833003
> Cr-Commit-Position: refs/heads/master@{#460282}
> Committed: https://chromium.googlesource.com/chromium/src/+/6ca5d0d3ad671604d9a028e71402e9e5ee25f110

TBR=kbr@chromium.org,zmo@chromium.org,xiangze.zhang@intel.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 690122 

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

[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_clear_framebuffer.h
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_decoder.h
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_decoder_mock.h
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.h
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/command_buffer/tests/gl_clear_framebuffer_unittest.cc
[modify] https://crrev.com/e618b6971a699f5d25d9f1b8c5d3011b8118feab/gpu/config/gpu_driver_bug_list_json.cc

To clarify: from one of the builds, e.g.:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28AMD%20R7%20240%29/builds/1868

The following DCHECK failure was seen in the failing tests:

[ RUN      ] GLClearFramebufferTestWithParam/GLClearFramebufferTest.ClearColorWithScissor/1
[7848:7848:0330/073310.433579:12694979104:FATAL:gl_clear_framebuffer_unittest.cc(44)] Check failed: !gl_.workarounds().gl_clear_broken. 
#0 0x0000004cefe7 base::debug::StackTrace::StackTrace()
#1 0x0000004d564b logging::LogMessage::~LogMessage()
#2 0x00000044b0bd gpu::GLClearFramebufferTest::SetUp()
#3 0x000000586b4e testing::Test::Run()
#4 0x000000587650 testing::TestInfo::Run()
#5 0x0000005879c7 testing::TestCase::Run()
#6 0x00000058ec17 testing::internal::UnitTestImpl::RunAllTests()
#7 0x00000058e897 testing::UnitTest::Run()
#8 0x000000535fd1 base::TestSuite::Run()
#9 0x00000048e314 (anonymous namespace)::RunHelper()
#10 0x0000005373e0 base::(anonymous namespace)::LaunchUnitTestsInternal()
#11 0x00000053790d base::LaunchUnitTestsSerially()
#12 0x00000048e211 main
#13 0x7fcce05bef45 __libc_start_main
#14 0x0000004244b1 <unknown>

[150/233] GLClearFramebufferTestWithParam/GLClearFramebufferTest.ClearColorWithScissor/1 (CRASHED)

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

commit 9b1ac7f2a38585c066bc6523e0c08aeb1adbf171
Author: xiangze.zhang <xiangze.zhang@intel.com>
Date: Fri Mar 31 21:14:25 2017

Reland of "Make disabling 2d canvas GPU acceleration for getImageData less aggressive."

Original patch https://codereview.chromium.org/2687073003/ reverted due to
test regression. The test failure does not happen now.

Make canvas switch out of gpu acceleration only when getImageData is
called in successive frames. This will avoid disabling gpu acceleration
when getImageData is only called once or is called very rarely.

BUG= 690122 

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

[modify] https://crrev.com/9b1ac7f2a38585c066bc6523e0c08aeb1adbf171/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/9b1ac7f2a38585c066bc6523e0c08aeb1adbf171/third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h
[modify] https://crrev.com/9b1ac7f2a38585c066bc6523e0c08aeb1adbf171/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
[modify] https://crrev.com/9b1ac7f2a38585c066bc6523e0c08aeb1adbf171/third_party/WebKit/Source/platform/graphics/ImageBuffer.h

Note: 6ca5d0d3ad671604d9a028e71402e9e5ee25f110 likely caused Issue 707161, which was seen on Android devices in the wild. Must be careful when attempting to reland it.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot