This change is ready for review.
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
+bsalomon as reviewer
Please check that I got the resetContext bits right in gpu/skia_bindings/gles2_implementation_with_grcontext_support.cc
I did this based on my reading of GrGLGpu.cpp, but I may have missed some subtleties.
Justin Novosad would like Brian Salomon to review this change.
Automate calls to GrContext::resetContext
The purpose of this change is to prevent the re-occurrence of flaky
rendering bugs caused by missing calls to GrContext::resetContext. This
change uses a new subclass of GLES2Implementation called
GLES2ImplementationWithGrContextSupport, which takes care of calling
GrContext::resetContext() whenever the gl state is changed. These
calls are lightweight: they just perform an 'or' on an integer.
To avoid calling GrContext::resetContext() when gl calls are made from
within skia, the bindings were modified in CreateGLES2InterfaceBindings
in order to signal the GLES2ImplementationWithGrContextSupport so that
it knows the the current GL call is made from skia.
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;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: I59470a8188df235a36d6de0e9aaff57b69ff9d6a
Bug: 807974, 810159
---
M cc/raster/gpu_raster_buffer_provider.cc
M cc/raster/raster_buffer_provider_perftest.cc
M cc/raster/scoped_gpu_raster.cc
M cc/test/test_in_process_context_provider.cc
M cc/test/test_in_process_context_provider.h
M cc/tiles/gpu_image_decode_cache.cc
M components/viz/common/gl_helper.cc
M components/viz/common/gl_helper.h
M components/viz/common/gpu/context_provider.h
M components/viz/common/gpu/in_process_context_provider.cc
M components/viz/common/gpu/in_process_context_provider.h
M components/viz/common/gpu/raster_context_provider.h
M components/viz/test/test_context_provider.cc
M components/viz/test/test_context_provider.h
M content/renderer/media_capture_from_element/canvas_capture_handler.cc
M content/renderer/pepper/video_decoder_shim.cc
M content/renderer/webgraphicscontext3d_provider_impl.cc
M content/renderer/webgraphicscontext3d_provider_impl.h
M gpu/command_buffer/client/BUILD.gn
A gpu/command_buffer/client/gles2_interface.cc
M gpu/command_buffer/client/gles2_interface.h
M gpu/skia_bindings/BUILD.gn
M gpu/skia_bindings/gl_bindings_skia_cmd_buffer.cc
A gpu/skia_bindings/gles2_implementation_with_grcontext_support.cc
A gpu/skia_bindings/gles2_implementation_with_grcontext_support.h
M gpu/skia_bindings/grcontext_for_gles2_interface.cc
M gpu/skia_bindings/grcontext_for_gles2_interface.h
M media/renderers/paint_canvas_video_renderer.cc
M services/ui/public/cpp/gpu/context_provider_command_buffer.cc
M services/ui/public/cpp/gpu/context_provider_command_buffer.h
M third_party/WebKit/Source/platform/graphics/CanvasResource.cpp
M third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h
M third_party/WebKit/Source/platform/graphics/gpu/GraphicsContext3DUtils.cpp
M third_party/WebKit/Source/platform/graphics/test/FakeWebGraphicsContext3DProvider.h
M third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h
M ui/compositor/test/in_process_context_provider.cc
M ui/compositor/test/in_process_context_provider.h
37 files changed, 503 insertions(+), 143 deletions(-)
I have no idea what
gpu/skia_bindings/gl_bindings_skia_cmd_buffer.cc is doing. But apart from that LGTM.
Patch set 8:Code-Review +1
5 comments:
File gpu/command_buffer/client/BUILD.gn:
Patch Set #10, Line 149: "gles2_interface.cc",
That causes issues I think because gles2_interface is a source_set, i.e. it's included in several components that only care about the interface, and it doesn't have the //gpu:gpu_implementation config needed to make GPU_EXPORT work (nor do we want to make this part of the GPU component, *maybe* the gles2 one, though I'd prefer if we could avoid it). See comment in gles2_interface.h though, maybe we can move the functionality on ContexSupport
File gpu/command_buffer/client/gles2_interface.h:
Patch Set #10, Line 42: virtual void SetGrContext(GrContext* gr) {}
Can we put these on ContextSupport instead (also implemented by GLES2Implementation and accessible from the ContextProvider)? They don't really belong in the GLES2 part of the API.
It's just a matter of passing the ContextSupport to GrContextForGLES2Interface. Or even the concrete GLES2ImplementationWithGrContextSupport for that matter.
File gpu/skia_bindings/gl_bindings_skia_cmd_buffer.cc:
Patch Set #10, Line 31: gpu::gles2::GLES2Interface* gles2Interface) {
nit: gles2_interface
Patch Set #10, Line 34: static_cast<GLES2ImplementationWithGrContextSupport*>(gles2Interface);
I think we should pass the concrete class to CreateGLES2InterfaceBindings (and here). See comments in earlier files.
File services/ui/public/cpp/gpu/context_provider_command_buffer.cc:
Patch Set #10, Line 282: skia_bindings::GLES2ImplementationWithGrContextSupport>(
So, only a subset of instances of this class will ever call skia (essentially only the Display context, the compositor worker context and the Canvas2D context, but not e.g. webgl contexts, compositor context, video context, pepper). Would it make sense to only pay the overhead if we will? I.e. add a flag at creation time, that would decide here whether we create a gpu::gles2::GLES2Implementation or a skia_bindings::GLES2ImplementationWithGrContextSupport, and DCHECK'ed in ContextProviderCommandBuffer::GrContext() ?
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
New patch to review.
5 comments:
File gpu/command_buffer/client/BUILD.gn:
Patch Set #10, Line 149: "gles2_interface.h",
That causes issues I think because gles2_interface is a source_set, i.e. […]
Done
File gpu/command_buffer/client/gles2_interface.h:
Patch Set #10, Line 42: #include "gpu/command_buffer/client/gles2_interface_autogen.h"
Can we put these on ContextSupport instead (also implemented by GLES2Implementation and accessible f […]
Done
File gpu/skia_bindings/gl_bindings_skia_cmd_buffer.cc:
Patch Set #10, Line 31: gpu::gles2::GLES2Interface* gles2_interface,
nit: gles2_interface
Done
Patch Set #10, Line 34: GLES2ImplementationWithGrContextSupport* gl_for_gr_context =
I think we should pass the concrete class to CreateGLES2InterfaceBindings (and here). […]
Actually, we have to pass the interface and the context_support separately because these are not always the same object (in unit tests).
File services/ui/public/cpp/gpu/context_provider_command_buffer.cc:
Patch Set #10, Line 282: constexpr bool support_client_side_arrays = false;
So, only a subset of instances of this class will ever call skia (essentially only the Display conte […]
Done. Resolving this added a lot of files to the CL because of the plumbing of the support_grcontext argument.
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
I was conservative about setting the support_grcontext argument to true, so we might get a few test crashes in DCHECKs. Waiting to see...
2 comments:
File content/browser/compositor/viz_process_transport_factory.cc:
Patch Set #15, Line 523: constexpr bool kSharedWorkerContextSupportsGrContext = false;
Any particular reason why the shared worker context in GpuProcessTransportFactory has GrContext support, but not the one here?
Arguably it is not needed unless we enable GPU raster for UI, but either way the decision is orthogonal between Viz or not, so I think both should be consistent.
Ack on the compositor context though, because in GPTF it's also used for Display (which needs a GrContext for filters) but not here.
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #15, Line 1128: constexpr bool support_grcontext = true;
How come, isn't this context for WebGL?
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
2 comments:
Patch Set #15, Line 523: constexpr bool kSharedWorkerContextSupportsGrContext = false;
Any particular reason why the shared worker context in GpuProcessTransportFactory has GrContext supp […]
I still don't think we should enable GrContext support here because this context does not even have GLES2 support.
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #15, Line 1128: constexpr bool support_grcontext = true;
How come, isn't this context for WebGL?
It's because this code path is also used by SharedGpuContext when it is not on the main thread.
This means it can be used by 2d contexts in workers (via OffscreenCanvas). Therefore a GrContext is required.
I see confusion here though... the context type is being set to WEBGL* even though this code path is also used by other things.
In the latest patch, I disentangled this situation by adding a new field to Platform::ContextAttributes to specify whether GrContext support is required. Also, to clarify the code, I changed the webgl_version attribute into a context_type attribute. To fix that I could have create a new enum, but instead, I simply used gpu::ContextType. Is that kosher? I think it is okay now to reference a gpu:: type in Platform.h, right?
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
Okay, turns out I can't use gpu:: from blink module code. It fails a presubmit check (disallowed identifier). I am going to add an intermediate enum. Stay tuned...
Couple of things then LGTM.
3 comments:
Patch Set #15, Line 523: constexpr bool kSharedWorkerContextSupportsGrContext = false;
I still don't think we should enable GrContext support here because this context does not even have […]
Ack. Should we make the GPTF version false as well, then?
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #15, Line 1128: // Prefer discrete GPU for WebGL.
It's because this code path is also used by SharedGpuContext when it is not on the main thread. […]
Oh, wow, that's a little scary, WEBGL* contexts have some different semantics which could confuse skia, I'm surprised we haven't run into it.
Thanks a lot for clarifying!
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #16, Line 1138: constexpr bool support_grcontext = true;
Did you mean to use web_attributes.support_grcontext?
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
3 comments:
Patch Set #15, Line 523: constexpr bool kSharedWorkerContextSupportsGrContext = false;
Ack. […]
Done.
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #15, Line 1128: // Prefer discrete GPU for WebGL.
Oh, wow, that's a little scary, WEBGL* contexts have some different semantics which could confuse sk […]
Maybe we have run into problems with this and just didn't know it. There's a bunch of suppressed OffscreenCanvas in workers tests. I'll check them out after this lands.
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #16, Line 1138: constexpr bool support_grcontext = web_attributes.support_grcontext;
Did you mean to use web_attributes. […]
OMG. Good catch!
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
Adding more reviewers for specific areas touched by this CL
hubbe:
media/renderers/paint_canvas_video_renderer.cc
sadrul:
services/ui/public/cpp/gpu/context_provider_command_buffer.cc
services/ui/public/cpp/gpu/context_provider_command_buffer.h
services/ui/public/cpp/gpu/gpu.cc
boliu:
android_webview/browser/aw_render_thread_context_provider.cc
android_webview/browser/aw_render_thread_context_provider.h
dcheng:
third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.cpp
third_party/WebKit/public/platform/Platform.h
third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h
media: lgtm
Patch set 20:Code-Review +1
Patch Set 19:
Adding more reviewers for specific areas touched by this CL
hubbe:
media/renderers/paint_canvas_video_renderer.cc
sadrul:
services/ui/public/cpp/gpu/context_provider_command_buffer.cc
services/ui/public/cpp/gpu/context_provider_command_buffer.h
services/ui/public/cpp/gpu/gpu.cc
boliu:
android_webview/browser/aw_render_thread_context_provider.cc
android_webview/browser/aw_render_thread_context_provider.h
stampidy stamp
dcheng:
third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.cpp
third_party/WebKit/public/platform/Platform.h
third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h
Patch set 19:Code-Review +1
1 comment:
File services/ui/public/cpp/gpu/context_provider_command_buffer.h:
Patch Set #20, Line 69: bool support_grcontext,
Is it time to introduce an InitParams for all this?
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patch Set #20, Line 69: bool support_grcontext,
Is it time to introduce an InitParams for all this?
Probably. We could fold these boolean args into gpu::ContextCreationAttribs? Anything more involved than that should probably be in a CL of its own IMHO.
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 20:Code-Review +1
1 comment:
Patch Set #20, Line 69: bool support_grcontext,
Probably. […]
Separate CL sounds good. Thanks!
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
LGTM with nits
Patch set 21:Code-Review +1
5 comments:
File content/renderer/renderer_blink_platform_impl.cc:
Patch Set #21, Line 222: default:
Nit: remove this
File gpu/command_buffer/client/context_support.h:
Patch Set #21, Line 120: true the
true if the?
File third_party/WebKit/public/platform/Platform.h:
Patch Set #21, Line 520: enum ContextType {
Please add some documentation for these values if possible.
Patch Set #21, Line 538: // or GrContext.
Similarly, some more details here would be useful, such as in what situations should RasterInterface or GrContext be needed or not needed, and why. For example, it seems always setting support_grcontext to true is likely to lead to fewer rendering bugs. So in what circumstances is it safe to set support_grcontext to false? Or is it never safe, and code just has to be extra careful (i.e. the state of the world before this CL)?
File ui/compositor/test/in_process_context_provider.cc:
Patch Set #21, Line 142: // Purposely not calling GrContext()
Nit: this comment isn't very helpful to someone who doesn't already understand this code well (i.e. me =)
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 21:Code-Review +1
1 comment:
Patch Set #20, Line 69: bool support_grcontext,
Separate CL sounds good. […]
I'd rather not include those in gpu::ContextCreationAttribs, because they mostly refer to client-side stuff, whereas ContextCreationAttribs is really meant for service-side stuff
(separate CL SG either way)
To view, visit change 924616. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 24:Commit-Queue +2
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix image encoding layout tests" https://chromium-review.googlesource.com/c/924616/24
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/924616/24
Bot data: {"action": "start", "triggered_at": "2018-03-16T21:46:29.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "4704861fe15d8db0594b115d97b7b9c8eecb293e"}
Patch set 24:Commit-Queue +1
Patch set 25:Commit-Queue +2
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"dcheng nits" https://chromium-review.googlesource.com/c/924616/25
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/924616/25
Bot data: {"action": "start", "triggered_at": "2018-03-16T22:39:57.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "d9f480efec90f161e1b55412cd66721c0cc7b661"}
Try jobs failed on following builders:
linux_optional_gpu_tests_rel on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_optional_gpu_tests_rel/227)
Patch set 26:Commit-Queue +2
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"fix crash in cc unit test" https://chromium-review.googlesource.com/c/924616/26
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/924616/26
Bot data: {"action": "start", "triggered_at": "2018-03-17T06:38:27.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "aa92393c2b161f6375fef6e43a1f5f41795135fb"}
Try jobs failed on following builders:
win-msvc-rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win-msvc-rel/builds/74441)
Patch set 27:Commit-Queue +2
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"fix test_context_support" https://chromium-review.googlesource.com/c/924616/27
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/924616/27
Bot data: {"action": "start", "triggered_at": "2018-03-18T04:57:49.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "61b0347a7e32d74a850cfa09691dfa8c3792793b"}
Try jobs failed on following builders:
fuchsia_x64 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/fuchsia_x64/builds/89242)
great and good job
Patch set 28:Commit-Queue +2
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"ContextSupportStub build fix" https://chromium-review.googlesource.com/c/924616/28
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/924616/28
Bot data: {"action": "start", "triggered_at": "2018-03-18T06:31:56.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "41ac4a2a9564baded65a569587857705b549deed"}
Commit Bot merged this change.
Automate calls to GrContext::resetContext
The purpose of this change is to prevent the re-occurrence of flaky
rendering bugs caused by missing calls to GrContext::resetContext. This
change uses a new subclass of GLES2Implementation called
GLES2ImplementationWithGrContextSupport, which takes care of calling
GrContext::resetContext() whenever the gl state is changed. These
calls are lightweight: they just perform an 'or' on an integer.
To avoid calling GrContext::resetContext() when gl calls are made from
within skia, the bindings were modified in CreateGLES2InterfaceBindings
in order to signal the GLES2ImplementationWithGrContextSupport so that
it knows the the current GL call is made from skia.
Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;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: I59470a8188df235a36d6de0e9aaff57b69ff9d6a
Bug: 807974, 810159
Reviewed-on: https://chromium-review.googlesource.com/924616
Commit-Queue: Justin Novosad <ju...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Antoine Labour <pi...@chromium.org>
Reviewed-by: Fredrik Hubinette <hu...@chromium.org>
Reviewed-by: Brian Salomon <bsal...@chromium.org>
Reviewed-by: Sadrul Chowdhury <sad...@chromium.org>
Reviewed-by: Bo <bo...@chromium.org>
Reviewed-by: Fernando Serboncini <fs...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543955}
---
M android_webview/browser/aw_render_thread_context_provider.cc
M android_webview/browser/aw_render_thread_context_provider.h
M cc/raster/gpu_raster_buffer_provider.cc
M cc/raster/raster_buffer_provider_perftest.cc
M cc/raster/scoped_gpu_raster.cc
M cc/test/test_in_process_context_provider.cc
M cc/test/test_in_process_context_provider.h
M cc/tiles/gpu_image_decode_cache.cc
M cc/trees/layer_tree_host_impl.cc
M components/viz/common/gl_helper.cc
M components/viz/common/gl_helper.h
M components/viz/common/gpu/context_provider.h
M components/viz/common/gpu/in_process_context_provider.cc
M components/viz/common/gpu/in_process_context_provider.h
M components/viz/common/gpu/raster_context_provider.h
M components/viz/test/test_context_provider.cc
M components/viz/test/test_context_provider.h
M components/viz/test/test_context_support.cc
M components/viz/test/test_context_support.h
M content/browser/compositor/gpu_process_transport_factory.cc
M content/browser/compositor/gpu_process_transport_factory.h
M content/browser/compositor/viz_process_transport_factory.cc
M content/browser/gpu/gpu_ipc_browsertests.cc
M content/browser/renderer_host/compositor_impl_android.cc
M content/renderer/media_capture_from_element/canvas_capture_handler.cc
M content/renderer/pepper/video_decoder_shim.cc
M content/renderer/render_thread_impl.cc
M content/renderer/renderer_blink_platform_impl.cc
M content/renderer/webgraphicscontext3d_provider_impl.cc
M content/renderer/webgraphicscontext3d_provider_impl.h
M content/shell/test_runner/test_plugin.cc
M content/test/layouttest_support.cc
M gpu/command_buffer/client/context_support.h
M gpu/command_buffer/client/implementation_base.cc
M gpu/command_buffer/client/implementation_base.h
M gpu/command_buffer/client/raster_implementation_gles_unittest.cc
M gpu/ipc/BUILD.gn
M gpu/ipc/gl_in_process_context.cc
M gpu/skia_bindings/BUILD.gn
M gpu/skia_bindings/gl_bindings_skia_cmd_buffer.cc
M gpu/skia_bindings/gl_bindings_skia_cmd_buffer.h
A gpu/skia_bindings/gles2_implementation_with_grcontext_support.cc
A gpu/skia_bindings/gles2_implementation_with_grcontext_support.h
M gpu/skia_bindings/grcontext_for_gles2_interface.cc
M gpu/skia_bindings/grcontext_for_gles2_interface.h
M media/renderers/paint_canvas_video_renderer.cc
M services/ui/public/cpp/gpu/context_provider_command_buffer.cc
M services/ui/public/cpp/gpu/context_provider_command_buffer.h
M services/ui/public/cpp/gpu/gpu.cc
M third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.cpp
M third_party/WebKit/Source/platform/graphics/CanvasResource.cpp
M third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
M third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h
M third_party/WebKit/Source/platform/graphics/gpu/GraphicsContext3DUtils.cpp
M third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp
M third_party/WebKit/Source/platform/graphics/test/FakeWebGraphicsContext3DProvider.h
M third_party/WebKit/public/platform/Platform.h
M third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h
M ui/compositor/test/in_process_context_provider.cc
M ui/compositor/test/in_process_context_provider.h
60 files changed, 999 insertions(+), 422 deletions(-)