New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 906166

Blocking:
issue 898270
issue 910274



Sign in to add a comment
link

Issue 902904: Decouple RasterDecoder from virtual contexts

Reported by penghuang@chromium.org, Nov 7 Project Member

Issue description

RasterDecoder does not really use virtual contexts. It has a virtual context to change how virtual context state is restored. Specifically, when RasterDecoder is made current after a different virtual context was current, it resets it's state. When a virtual context is made current after a RasterDecoder was current, that virtual context does a full state reset.

We need to do something similar for SkiaRenderer using SkDDL (SkiaOutputSurfaceImplOnGpu). As per comments #7 and #8 below, we could modify GLContext to permit this sort of thing without directly relying on GLContextVirtual.
 

Comment 1 by backer@chromium.org, Nov 7

Labels: -Pri-3 Pri-2
I think that this could be an important performance optimization in the common case when switching between SkiaRenderer w/ SkDDL and OOP-R. Specifically, these already share the same GrContext. By using the same virtual context, we will

(a) avoid unnecessary software reset to ground state (e.g. ContextState::RestoreState(nullptr) )

(b) avoid resetting GrContext state tracking

(c) potentially avoid calls to GLContext::MakeCurrent (e.g. RasterDecoder shouldn't care what surface it is made current with)


Peng dug into this a bit, and I believe that it is feasible.

Comment 2 by backer@chromium.org, Nov 7

To be more concrete, I think that we would push the GLVirtualContext up to the RasterDecoderContextState for sharing.

One thing I'm unsure is dealing with different GetCompatibilityKey on Linux. Maybe we would have to have a different RasterDecoderContextState per compatibility key?

Comment 3 by piman@chromium.org, Nov 7

Do we even need GLVirtualContext for RasterDecoder and SkiaRenderer?
What's important is that we use the same driver-level context, but do we actually need a GLVirtualContext, meaning, one that automatically resets all state on MakeCurrent? Both SkiaRenderer and RasterDecoder can reset the exact state they need (or invalidate Skia state).

Comment 4 by piman@chromium.org, Nov 7

Re: multiple RasterDecoderContextState, I don't think it's desirable, because that would mean multiple GrContexts, meaning sharing of SkImages etc. would be problematic, duplicate skia caches etc.
One option to look into is to have 1 context per "compatibility key" in the RasterDecoderContextState, but I don't know if Skia would behave sanely with multiple contexts under one GrContext (e.g. for unshared resources such as framebuffer objects, vertex array objects, queries, etc.). Another option is to see if we could just get rid of this. It's only for linux, and at the end of the day, for an optimization (avoiding 1 full-screen blit on presentation when not using a compositing manager). I don't think linux optimizations punishing other platforms would be the right tradeoff at this point.

Comment 5 by piman@chromium.org, Nov 7

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=902943 dumping thoughts there, I think we could remove the compatibility key concept, maybe even without a perf hit.

Comment 6 by backer@chromium.org, Nov 8

Re #3: You are correct that the main advantage is that we are using the same driver-level context.

I like your idea of not using a virtual context at all, because we could avoid unnecessary virtual context switching.

We would have to audit the RasterDecoder. Here are the issues I am aware of that are unaddressed by SharedImage.

We are using CopyTexImageResourceManager and CopyTextureCHROMIUMResourceManager
for CopySubTextureCHROMIUM in RasterDecoder. This is used for partial raster on the one copy raster path. Both of these managers call back into the Restore* hooks typically just used for virtual context state restoration.

RasterDecoder::CopySubTexture calls TextureManager::ClearTextureLevel, which calls back into RasterDecoder::ClearLevel, which does a ScopedPixelUnpackState.

Comment 7 by backer@chromium.org, Nov 8

Peng and I were just chatting about this and I'd like to share our thoughts so that you all can give feedback.

Here are the main use cases that we are trying to address when virtual contexts are active:

(1) SkiaRenderer,SkDDL and RasterDecoder,OOP-R interleaved: we don't want to reset GrContext and we want to ignore virtual context state; we do need to make the screen surface current with the context when SkDDL runs; we don't need to make offscreen surface current when OOP-R runs (except when screen surface is destroyed)

(2) SkiaRenderer,SkDDL interleaved with WebGL: when we transition from WebGL to SkDDL, we need to reset the GrContext but we don't need to reset to ground context state; when we transition from SkDDL to WebGL, we need to do a hard reset of the WebGL virtual context state

piman@ is correct that SkDLL and RasterDecoder don't have to have a virtual context to do this. RasterDecoder is currently using it's virtual context (one per RasterDecoder) to hook into the transitions between other clients (like WebGL) by looking for calls on RasterDecoderImpl::GetContextState (called when transitioning to WebGL) and RasterDecoderImpl::RestoreState (called when transitioning from WebGL).

The current implementation has a disadvantage that we do a ContextState::RestoreState in RasterDecoder when we transition from WebGL to OOP-R. This RestoreState is still necessary for CopySubTexture, but that is only called on the one copy raster path. As per comment #6, we can separately audit RasterDecoder and remove the resetting to ground state.

We were thinking of two paths forward:
(1) Give RasterDecoderContextState a GLContextVirtual and have RDCS be called during virtual context switches (e.g. give it GetContextState and RestoreState). This way RDCS can reset GrContext when WebGL becomes current. Initially, there would be one ground ContextState shared by all RasterDecoders and SkDDL. We would restore it when transitioning from WebGL (at least until we audit RasterDecoder).

(2) Remove GLContextVirtual from RasterDecoder and add hooks into GLContext so that 
(a) SkDDL,RasterDecoder can detect when WebGL has made a virtual context current and GrContext needs a reset and
(b) SkDDL,RasterDecoder can force WebGL to do a hard reset (something like GLContext::OnReleaseVirtuallyCurrent).

Comment 8 by piman@chromium.org, Nov 8

Sounds pretty good. I like option 2. One thing that I was thinking about is to replace the GLContextVirtual + GLStateRestorer logic by a simple "state key" that various clients could tag on a shared GLContext when making current, as well as querying the previous key. This would be to detect the conditions in (2.a) and (2.b). GLES2DecoderImpl would keep its ContextState, but do the state restore explicitly rather than implicitly in GLES2DecoderImpl::MakeCurrent, tagging the GLContext with a key corresponding to the ContextState. Skia use would have its own key (for SkDDL and RasterDecoder::*RasterCHROMIUM), and so would RasterDecoder for the GL use cases (CopySubTexture + whatever else we might want to add as we port other parts of the pipeline). We'd want some encoding of the key so that switching between 2 GLES2DecoderImpl can reset only the subset of the ContextState that's needed (as is today). So the key would be something like enum (GLES2/Skia/GL-for-RasterDecoder) + pointer to object (ContextState/...).

Not sure if Option 1 is more expedient. It certainly has a higher cost.

Comment 9 by bugdroid1@chromium.org, Nov 9

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

commit bea0d6b3c467e05183fb88d06a829a6d7e7d136a
Author: Peng Huang <penghuang@chromium.org>
Date: Fri Nov 09 10:03:03 2018

Move context state related methods out of DecoderContext

This CL adds a new GLContextVirtualDelegate interface to contain all
context state related methods from DecoderContext. After this one, I am
going to implement GLContextVirtualDelegate in RasterDecoderContextState
and only create one GLContextVirtual for all raster decoder and display
compositor.

Bug:  900941 ,  902904 
Change-Id: I10d80bea8531885c449a2e676ce0552e9363b34c
Reviewed-on: https://chromium-review.googlesource.com/c/1312338
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606777}
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/BUILD.gn
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/decoder_context.h
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/gl_context_virtual.cc
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/gl_context_virtual.h
[add] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/gl_context_virtual_delegate.h
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/gl_state_restorer_impl.cc
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/gl_state_restorer_impl.h
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/raster_decoder_context_state.cc
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/raster_decoder_context_state.h
[modify] https://crrev.com/bea0d6b3c467e05183fb88d06a829a6d7e7d136a/gpu/command_buffer/service/wrapped_sk_image.cc

Comment 10 by bugdroid1@chromium.org, Nov 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83884c4057b6da2959d519a7e1f7b91b7e54fa99

commit 83884c4057b6da2959d519a7e1f7b91b7e54fa99
Author: Peng Huang <penghuang@chromium.org>
Date: Wed Nov 14 00:16:06 2018

Remove vertex attrib related code in RasterDecoder.

RatserDecoder uses Skia to do rendering, and skia can restore vertex
attribs. So we don't need maintain and restore it in RasterDecoder.

Bug:  902904 
Change-Id: I3e85d3960e0b782e41ae6d45c07ddf3494e63ca9
Reviewed-on: https://chromium-review.googlesource.com/c/1334201
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607826}
[modify] https://crrev.com/83884c4057b6da2959d519a7e1f7b91b7e54fa99/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/83884c4057b6da2959d519a7e1f7b91b7e54fa99/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/83884c4057b6da2959d519a7e1f7b91b7e54fa99/gpu/command_buffer/service/raster_decoder_unittest_base.cc

Comment 11 by bugdroid1@chromium.org, Nov 15

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

commit c52da365c963bca29b850ceeb36a4a0614bf3ee4
Author: Peng Huang <penghuang@chromium.org>
Date: Thu Nov 15 12:57:49 2018

Remove indexed uniform buffer related code in RasterDecoder

RatserDecoder uses Skia to do rendering, and skia can restore indexed
uniform buffer, So we don't need maintain and restore it in RasterDecoder.

Bug:  902904 
Change-Id: I234611646625a412d65bd632011ddd8e6647ca51
Reviewed-on: https://chromium-review.googlesource.com/c/1335733
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608338}
[modify] https://crrev.com/c52da365c963bca29b850ceeb36a4a0614bf3ee4/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/c52da365c963bca29b850ceeb36a4a0614bf3ee4/gpu/command_buffer/service/raster_decoder.cc

Comment 12 by bugdroid1@chromium.org, Nov 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/61eccfe452e2fc818d90c1fa253c315f4de8e23b

commit 61eccfe452e2fc818d90c1fa253c315f4de8e23b
Author: Peng Huang <penghuang@chromium.org>
Date: Thu Nov 15 22:10:13 2018

Decouple gles2::ContextState and gles2::Logger from DecoderClient

We would like to share one ContextState for all raster decoders and
display compositor. To do that, we need make ContextState work without
a decoder client.

Bug:  902904 
Change-Id: I0ebabbf084e864f18b5e5e1bc5d069a3d436718e
Reviewed-on: https://chromium-review.googlesource.com/c/1338383
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608539}
[modify] https://crrev.com/61eccfe452e2fc818d90c1fa253c315f4de8e23b/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/61eccfe452e2fc818d90c1fa253c315f4de8e23b/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc
[modify] https://crrev.com/61eccfe452e2fc818d90c1fa253c315f4de8e23b/gpu/command_buffer/service/logger.cc
[modify] https://crrev.com/61eccfe452e2fc818d90c1fa253c315f4de8e23b/gpu/command_buffer/service/logger.h
[modify] https://crrev.com/61eccfe452e2fc818d90c1fa253c315f4de8e23b/gpu/command_buffer/service/raster_decoder.cc

Comment 13 by penghuang@chromium.org, Nov 16

Blocking: 898270

Comment 14 by penghuang@chromium.org, Nov 16

Labels: Proj-Vulkanize

Comment 15 by kbr@chromium.org, Nov 16

Blockedon: 906166

Comment 16 by bugdroid1@chromium.org, Nov 27

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

commit c6a76075a1712c9c28f2a427948946884406ed8f
Author: Peng Huang <penghuang@chromium.org>
Date: Tue Nov 27 23:17:31 2018

Remove texture and sampler related code in RasterDecoder.

RasterDecoder uses skia to do rendering, and skia can restore texure
and sampler bindings by itself. So we don't need use ContextState to
do it.

Bug:  902904 
Change-Id: I5ce2510b4616aab9087cd61bfbfb88368c6e34ab
Reviewed-on: https://chromium-review.googlesource.com/c/1338159
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611396}
[modify] https://crrev.com/c6a76075a1712c9c28f2a427948946884406ed8f/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/c6a76075a1712c9c28f2a427948946884406ed8f/gpu/command_buffer/service/context_state.h
[modify] https://crrev.com/c6a76075a1712c9c28f2a427948946884406ed8f/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/c6a76075a1712c9c28f2a427948946884406ed8f/gpu/command_buffer/service/raster_decoder_unittest_base.cc

Comment 17 by penghuang@chromium.org, Nov 28

Owner: penghuang@chromium.org
Status: Started (was: Available)

Comment 18 by backer@chromium.org, Nov 30

Blocking: 910274

Comment 19 by backer@chromium.org, Dec 3

Summary: Decouple RasterDecoder from virtual contexts (was: Share one GLContextVirtual with all RasterDecoder)
Changing summary as per comment #7 and #8 to remove confusion.

Comment 20 by backer@chromium.org, Dec 3

Description: Show this description

Comment 22 by bugdroid1@chromium.org, Dec 5

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

commit b4ed185bebb4d8167ab4b03e29d8068e19fa9813
Author: Peng Huang <penghuang@chromium.org>
Date: Wed Dec 05 03:35:29 2018

Decouple ErrorState and ContextState.

Bug:  902904 
Change-Id: Idaf970c574597d2436befccc4f0979392c125d89
Reviewed-on: https://chromium-review.googlesource.com/c/1362220
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613851}
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/buffer_manager.cc
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/buffer_manager.h
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/context_state.h
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_state.cc
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/texture_manager.cc
[modify] https://crrev.com/b4ed185bebb4d8167ab4b03e29d8068e19fa9813/gpu/command_buffer/service/texture_manager.h

Comment 23 by bugdroid1@chromium.org, Dec 7

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1808a743048d147521eaffb386dc9f29b4ecae0e

commit 1808a743048d147521eaffb386dc9f29b4ecae0e
Author: Peng Huang <penghuang@chromium.org>
Date: Fri Dec 07 14:40:58 2018

Share one GLContext for all RasterDecoder when virtual context is used.

With this CL, we will create a GLContextVirtual in RasterDecoderContextState,
and share it with all raster decoders, display compositoer, etc. It is a
temporary solution. In follow up CLs, we will share the one GLContext with
GLES2 as well. In that case, we will use GLContext directly instead of
GLContextVirtual.

Change-Id: If9b8d1f32e58b32a9d9f8cd07aa10c30432e3183
Bug:  902904 
Reviewed-on: https://chromium-review.googlesource.com/c/1357463
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614694}
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/components/viz/service/gl/gpu_service_impl.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/decoder_context.h
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/gl_context_virtual_delegate.h
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/gl_state_restorer_impl.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/gr_cache_controller.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder_context_state.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder_context_state.h
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder_unittest.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder_unittest_base.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder_unittest_base.h
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/raster_decoder_unittest_context_lost.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/command_buffer/service/wrapped_sk_image.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/ipc/in_process_command_buffer.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/ipc/service/gpu_channel_manager.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/ipc/service/raster_command_buffer_stub.cc
[modify] https://crrev.com/1808a743048d147521eaffb386dc9f29b4ecae0e/gpu/ipc/service/shared_image_stub.cc

Comment 24 by bugdroid1@chromium.org, Dec 7

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

commit be615a8821d04fb4bf6d371be95bd323d2d4155c
Author: Peng Huang <penghuang@chromium.org>
Date: Fri Dec 07 18:00:38 2018

Revert "Share one GLContext for all RasterDecoder when virtual context is used."

This reverts commit 1808a743048d147521eaffb386dc9f29b4ecae0e.

Reason for revert: Null-dereference READ in gpu::gles2::ContextState::api
 https://crbug.com/912981 

Original change's description:
> Share one GLContext for all RasterDecoder when virtual context is used.
> 
> With this CL, we will create a GLContextVirtual in RasterDecoderContextState,
> and share it with all raster decoders, display compositoer, etc. It is a
> temporary solution. In follow up CLs, we will share the one GLContext with
> GLES2 as well. In that case, we will use GLContext directly instead of
> GLContextVirtual.
> 
> Change-Id: If9b8d1f32e58b32a9d9f8cd07aa10c30432e3183
> Bug:  902904 
> Reviewed-on: https://chromium-review.googlesource.com/c/1357463
> Commit-Queue: Peng Huang <penghuang@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#614694}

TBR=penghuang@chromium.org,piman@chromium.org

Change-Id: I2eb66f211d408988f30bfb870425da80e0eb7a4b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  902904 
Reviewed-on: https://chromium-review.googlesource.com/c/1368284
Reviewed-by: Peng Huang <penghuang@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614738}
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/components/viz/service/gl/gpu_service_impl.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/decoder_context.h
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/gl_context_virtual_delegate.h
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/gl_state_restorer_impl.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/gr_cache_controller.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder_context_state.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder_context_state.h
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder_unittest.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder_unittest_base.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder_unittest_base.h
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/raster_decoder_unittest_context_lost.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/command_buffer/service/wrapped_sk_image.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/ipc/in_process_command_buffer.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/ipc/service/gpu_channel_manager.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/ipc/service/raster_command_buffer_stub.cc
[modify] https://crrev.com/be615a8821d04fb4bf6d371be95bd323d2d4155c/gpu/ipc/service/shared_image_stub.cc

Comment 25 by bugdroid1@chromium.org, Dec 7

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66a7a37669a359059140065d35dc324191f85ab8

commit 66a7a37669a359059140065d35dc324191f85ab8
Author: Peng Huang <penghuang@chromium.org>
Date: Fri Dec 07 20:05:38 2018

Reland "Share one GLContext for all RasterDecoder when virtual context is used."


Original change's description:
> Revert "Share one GLContext for all RasterDecoder when virtual context is used."
>
> This reverts commit 1808a743048d147521eaffb386dc9f29b4ecae0e.
>
> Reason for revert: Null-dereference READ in gpu::gles2::ContextState::api
>  https://crbug.com/912981 
>
> Original change's description:
> > Share one GLContext for all RasterDecoder when virtual context is used.
> >
> > With this CL, we will create a GLContextVirtual in RasterDecoderContextState,
> > and share it with all raster decoders, display compositoer, etc. It is a
> > temporary solution. In follow up CLs, we will share the one GLContext with
> > GLES2 as well. In that case, we will use GLContext directly instead of
> > GLContextVirtual.
> >
> > Change-Id: If9b8d1f32e58b32a9d9f8cd07aa10c30432e3183
> > Bug:  902904 
> > Reviewed-on: https://chromium-review.googlesource.com/c/1357463
> > Commit-Queue: Peng Huang <penghuang@chromium.org>
> > Reviewed-by: Antoine Labour <piman@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#614694}
>
> TBR=penghuang@chromium.org,piman@chromium.org
>
> Change-Id: I2eb66f211d408988f30bfb870425da80e0eb7a4b
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  902904 
> Reviewed-on: https://chromium-review.googlesource.com/c/1368284
> Reviewed-by: Peng Huang <penghuang@chromium.org>
> Commit-Queue: Peng Huang <penghuang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#614738}

TBR=penghuang@chromium.org,piman@chromium.org

Change-Id: Ie409d5d631509efa978a8741381cedbbbcdb3904
Bug:  902904 
Reviewed-on: https://chromium-review.googlesource.com/c/1368031
Reviewed-by: Peng Huang <penghuang@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614796}
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/components/viz/service/gl/gpu_service_impl.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/decoder_context.h
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/gl_context_virtual_delegate.h
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/gl_state_restorer_impl.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/gr_cache_controller.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/gr_cache_controller_unittest.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder_context_state.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder_context_state.h
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder_unittest.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder_unittest_base.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder_unittest_base.h
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/raster_decoder_unittest_context_lost.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/shared_image_backing_factory_ahardwarebuffer_unittest.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/shared_image_backing_factory_gl_texture_unittest.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/service/wrapped_sk_image.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/command_buffer/tests/fuzzer_main.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/ipc/in_process_command_buffer.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/ipc/service/gpu_channel_manager.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/ipc/service/raster_command_buffer_stub.cc
[modify] https://crrev.com/66a7a37669a359059140065d35dc324191f85ab8/gpu/ipc/service/shared_image_stub.cc

Comment 27 by penghuang@chromium.org, Jan 16

Status: Fixed (was: Started)
I think this one has been fixed. Next, we will remove GLContextVirtual and let GLES2Decoder, RasterDecoder, display compositor and etc to use GLContext directly. I will file a new issue for it.

Sign in to add a comment