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

Issue 809692 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

SkColorType SkSurface creation change broke oop raster

Project Member Reported by enne@chromium.org, Feb 6 2018

Issue description

After https://chromium-review.googlesource.com/889861 oop raster (--enable-gpu-rasterization --enable-oop-rasterization) no longer works.  However, tests still appear to work.

When running with these flags after this patch, this spew happens indefinitely:

[162769:162769:0206/133040.518847:ERROR:gles2_cmd_decoder.cc(20453)] [.RenderWorker-0x3e12c356ec20]GL ERROR :GL_INVALID_OPERATION : glBeginRasterCHROMIUM: failed to create surface
[162769:162769:0206/133040.519042:ERROR:gles2_cmd_decoder.cc(20487)] [.RenderWorker-0x3e12c356ec20]GL ERROR :GL_INVALID_OPERATION : glRasterCHROMIUM: RasterCHROMIUM without BeginRasterCHROMIUM
[162769:162769:0206/133040.519261:ERROR:gles2_cmd_decoder.cc(20527)] [.RenderWorker-0x3e12c356ec20]GL ERROR :GL_INVALID_OPERATION : glBeginRasterCHROMIUM: EndRasterCHROMIUM without BeginRasterCHROMIUM

 

Comment 1 by enne@chromium.org, Feb 6 2018

https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/gl/GrGLCaps.cpp?l=2506 looks like the line that's making the surface fail creation on Linux.  It's asking for BGRA8888 but is using kGL_GrGLStandard and so doesn't get a pixel config.

oop unit tests seem to be using RGBA8888, which is why they are unaffected.
I see. Is the texture format really GL_BGRA? I thought that wasn't a valid internal format in non-ES GL.

Comment 3 by enne@chromium.org, Feb 6 2018

Yes, that's what it's asking for: https://cs.chromium.org/chromium/src/components/viz/common/resources/platform_color.h?sq=package:chromium&l=24 is the code that picks the format.

This works out for gpu raster on tot because it's behind the gles command buffer, but when creating surfaces in the gpu service for oop raster, then it's talking to gl directly.

Does validate_sized_format need to consider extensions?
That function supposed to allow anything that is present in GL/GLES or could be by an extension (recognized by Ganesh). However, AFAIK there is no extension to non-ES GL that adds a GL_BGRA base internal format (or GL_BGRA8 sized internal format). There is GL_EXT_bgra but it just adds GL_BGRA as a <format> param to calls like glTexImage where it refers to the CPU data in calls that transfer pixels in/out of GL objects. It would typically be used when creating a texture like this:

glTexImage2D(GL_TEXTURE_2D, 0, /* internalFormat = */ GL_RGBA, w, h, 0,  /* format = */ GL_BGRA, GL_UNSIGNED_BYTE, ptrToCPUData);


I made a change that will specifies the GrGLTextureInfo's fFormat field as GL_RGBA8 when the context is not ES. It seems to fix the issue on my Linux machine.
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 15 2018

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

commit 432fea88a41d74b9848eff7a43ce074df2d166d9
Author: Brian Salomon <bsalomon@google.com>
Date: Thu Feb 15 18:35:45 2018

Specify OOP raster SkSurface's format as GL_RGBA8 on desktop GL when SkColorType is kBGRA_8888

Bug:  809692 
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
Change-Id: I3796906eb7bcb64a919591c1e05f97a2ff7e44ee
Reviewed-on: https://chromium-review.googlesource.com/905484
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537083}
[modify] https://crrev.com/432fea88a41d74b9848eff7a43ce074df2d166d9/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/432fea88a41d74b9848eff7a43ce074df2d166d9/ui/gl/gl_gl_api_implementation.cc
[modify] https://crrev.com/432fea88a41d74b9848eff7a43ce074df2d166d9/ui/gl/gl_gl_api_implementation.h

Comment 7 by bsalo...@google.com, Feb 15 2018

Status: Fixed (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 21 2018

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

commit 848aaee2c40cd2c5dfa416c1dacebb44f4b6fa2e
Author: Khushal <khushalsagar@chromium.org>
Date: Wed Feb 21 01:10:29 2018

content: Add basic integration test for OOP raster.

R=enne@chromium.org

Bug:  809692 
Change-Id: Ib22f6f452046d9989ec9a1ec98fe09acba24c624
Reviewed-on: https://chromium-review.googlesource.com/906184
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537973}
[add] https://crrev.com/848aaee2c40cd2c5dfa416c1dacebb44f4b6fa2e/content/browser/oop_browsertest.cc
[modify] https://crrev.com/848aaee2c40cd2c5dfa416c1dacebb44f4b6fa2e/content/test/BUILD.gn

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 21 2018

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

commit 31ae52a0bf9cd65d2af0f471a26f236bfebc5abe
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Wed Feb 21 12:13:31 2018

Revert "content: Add basic integration test for OOP raster."

This reverts commit 848aaee2c40cd2c5dfa416c1dacebb44f4b6fa2e.

Reason for revert: broke OOPBrowserTest.Basic 
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20ChromiumOS%20MSan%20Tests

 #0 0x000000634a81 in __interceptor_backtrace /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:3955:13
    #1 0x00000be3764f in base::debug::StackTrace::StackTrace(unsigned long) ./../../base/debug/stack_trace_posix.cc:808:41
    #2 0x00000ab5b871 in content::(anonymous namespace)::DumpStackTraceSignalHandler(int) ./../../content/public/test/browser_test_base.cc:86:5
    #3 0x00000065aff9 in SignalHandler(int) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/msan/msan_interceptors.cc:1049:3
    #4 0x7f9e01eaccb0 in killpg ??:?
    #5 0x7f9e01eaccb0 in ?? ??:0
    #6 0x7f9e01f74693 in epoll_wait /build/eglibc-SvCtMH/eglibc-2.19/misc/../sysdeps/unix/syscall-template.S:81:0
    #7 0x00000061d970 in __interceptor_epoll_wait /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/msan/msan_interceptors.cc:891:13
    #8 0x00000c1936a7 in epoll_dispatch ./../../base/third_party/libevent/epoll.c:198:8
    #9 0x00000c186a2c in event_base_loop ./../../base/third_party/libevent/event.c:512:9
    #10 0x00000beb8dce in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) ./../../base/message_loop/message_pump_libevent.cc:257:9
    #11 0x00000bf5c000 in base::RunLoop::Run() ./../../base/run_loop.cc:133:14
    #12 0x00000abdc529 in RunThisRunLoop ./../../content/public/test/test_utils.cc:124:13
    #13 0x00000abdc529 in content::MessageLoopRunner::Run() ./../../content/public/test/test_utils.cc:241:0
    #14 0x00000ab26d69 in content::NavigateToURLBlockUntilNavigationsComplete(content::Shell*, GURL const&, int) ./../../content/public/test/content_browser_test_utils.cc:49:21
    #15 0x0000017ed89f in content::(anonymous namespace)::OOPBrowserTest_Basic_Test::RunTestOnMainThread() ./../../content/browser/oop_browsertest.cc:67:3
    #16 0x00000ab5ad40 in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() ./../../content/public/test/browser_test_base.cc:353:5
    #17 0x00000ad8063f in Run ./../../base/callback.h:124:12
    #18 0x00000ad8063f in content::ShellBrowserMainParts::PreMainMessageLoopRun() ./../../content/shell/browser/shell_browser_main_parts.cc:208:0
    #19 0x000008847e8c in content::BrowserMainLoop::PreMainMessageLoopRun() ./../../content/browser/browser_main_loop.cc:1105:13
    #20 0x000009ce2bd1 in Run ./../../base/callback.h:124:12
    #21 0x000009ce2bd1 in content::StartupTaskRunner::RunAllTasksNow() ./../../content/browser/startup_task_runner.cc:45:0
    #22 0x00000883f64c in content::BrowserMainLoop::CreateStartupTasks() ./../../content/browser/browser_main_loop.cc:986:25
    #23 0x000008855c2b in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) ./../../content/browser/browser_main_runner.cc:139:17
    #24 0x00000acfaaf1 in ShellBrowserMain(content::MainFunctionParams const&, std::__1::unique_ptr<content::BrowserMainRunner, std::__1::default_delete<content::BrowserMainRunner> > const&) ./../../content/shell/browser/shell_browser_main.cc:23:32
    #25 0x00000acc3f93 in content::ShellMainDelegate::RunProcess(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&) ./../../content/shell/app/shell_main_delegate.cc:322:16
    #26 0x000008382651 in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) ./../../content/app/content_main_runner.cc:410:35
    #27 0x0000083852d0 in content::ContentMainRunnerImpl::Run() ./../../content/app/content_main_runner.cc:713:12
    #28 0x0000118257b1 in service_manager::Main(service_manager::MainParams const&) ./../../services/service_manager/embedder/main.cc:456:29
    #29 0x000004e857a5 in content::ContentMain(content::ContentMainParams const&) ./../../content/app/content_main.cc:19:10
    #30 0x00000ab596db in content::BrowserTestBase::SetUp() ./../../content/public/test/browser_test_base.cc:309:3
    #31 0x00000ab2527f in content::ContentBrowserTest::SetUp() ./../../content/public/test/content_browser_test.cc:89:20
    #32 0x00000271190c in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest-internal-inl.h:0:10
    #33 0x0000027158c9 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2645:11
    #34 0x0000027172ea in testing::TestCase::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2763:28
    #35 0x00000273ad4f in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:4638:43
    #36 0x000002739766 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #37 0x00000ac499cb in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2297:46
    #38 0x00000ac499cb in base::TestSuite::Run() ./../../base/test/test_suite.cc:272:0
    #39 0x00000ab3c758 in content::ContentTestLauncherDelegate::RunTestSuite(int, char**) ./../../content/test/content_test_launcher.cc:108:48
    #40 0x00000abca59a in content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) ./../../content/public/test/test_launcher.cc:631:31
    #41 0x00000ab3c5f8 in main ./../../content/test/content_test_launcher.cc:138:10
    #42 0x7f9e01e97f45 in __libc_start_main /build/eglibc-SvCtMH/eglibc-2.19/csu/libc-start.c:287:0
    #43 0x0000005f84aa in _start ??:0:0

Original change's description:
> content: Add basic integration test for OOP raster.
> 
> R=​enne@chromium.org
> 
> Bug:  809692 
> Change-Id: Ib22f6f452046d9989ec9a1ec98fe09acba24c624
> Reviewed-on: https://chromium-review.googlesource.com/906184
> Commit-Queue: Khushal <khushalsagar@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Reviewed-by: enne <enne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#537973}

TBR=enne@chromium.org,khushalsagar@chromium.org,piman@chromium.org

Change-Id: Iefb06aeb5a6055f34babfd83794cfccf809d7389
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  809692 
Reviewed-on: https://chromium-review.googlesource.com/928581
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538071}
[delete] https://crrev.com/6d3cd7563365aed7ea2aba1aff254fe42edece51/content/browser/oop_browsertest.cc
[modify] https://crrev.com/31ae52a0bf9cd65d2af0f471a26f236bfebc5abe/content/test/BUILD.gn

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 22 2018

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

commit c3090730f17666355c48f4fd7ba7c882ffac87a4
Author: Khushal <khushalsagar@chromium.org>
Date: Thu Feb 22 00:07:36 2018

Reland content: Add basic integration test for OOP raster.

This reverts commit 31ae52a0bf9cd65d2af0f471a26f236bfebc5abe. The test
fails on MSAN bots because it uses system GL which is not instrumented
with MSAN.

R=piman@chromium.org

Bug:  809692 
Change-Id: I4db4b749fa03afbf7b92a2c66ebbe9332098f3cd
Reviewed-on: https://chromium-review.googlesource.com/930029
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538266}
[add] https://crrev.com/c3090730f17666355c48f4fd7ba7c882ffac87a4/content/browser/oop_browsertest.cc
[modify] https://crrev.com/c3090730f17666355c48f4fd7ba7c882ffac87a4/content/test/BUILD.gn

Sign in to add a comment