viz: Conflicting BeginFrameSource::source_id_ when GPU crashes |
||
Issue descriptionWhen running Linux desktop Chrome with --enable-viz there is an issue with BeginFrameSource::source_id_ when restarting the GPU process. If the GPU process crashes/restarts then all viz::BeginFrameSources are recreated. When a BeginFrameSource is created |source_id_| is set from an atomic sequence, so any new BeginFrameSources reuse the same pool of source_ids from before the crash. BeginFrameObservers expect that BeginFrameArgs::sequence_number is increasing for the same source_id. This condition isn't met which causes a couple DCHECKs to fail. This isn't a problem with the display compositor in the browser process because we can't recover from browser crashes. It seems like there are two solutions to the problem. 1. Reset all BeginFrameObservers when the GPU process crashes so they forget the last seen BeginFrameArgs. 2. Don't reuse BeginFrameSource::source_ids when the GPU process crashes. I looked into #1 but any implementation would be complicated. This reset has to happen in all processes and there is no definitive list of BeginFrameObservers per process, which means lots of plumbing. I think #2 will be a better solution as a result. The browser process would plumb some unique value to the GPU process. This value would change each time the GPU process is restarted. This value would be used when creating BeginFrameSources. brianderson@ suggested the following way to use the value: "The source_id is only 32-bits right now. If we reserve 16-bits for the atomic sequence, we could end up with aliasing after ~65k sources rather than ~4b sources. In practice this probably wont be an issue though since 1) it'll be rare that we create 65k sources before a restart, 2) I don't think we keep sources around very long where we'd have two alive that alias, and 3) even if we did alias, it'll only be a problem if an observer happens to switching between the two aliasing sources. All 3 happening are very unlikely." Another option would be to just make source_id_ a uint64_t.
,
Nov 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc94c9572ccb6fab46ae4308637dc98f1318cd94 commit bc94c9572ccb6fab46ae4308637dc98f1318cd94 Author: kylechar <kylechar@chromium.org> Date: Tue Nov 14 23:42:11 2017 Make BeginFrameSource::source_id_ uint64_t. In order to include part of the source id that changes when the GPU process restarts, make the BeginFrameSource bigger. This CL updates all usage of uint32_t for source_id to uint64_t along with some constants, ParamTraits and StructTraits. The extra 32 bits will be used as a next step. The only issue is that source_id is used to build TraceValue which only supports int. The lower 32 bits are the interesting part so continue to output only them. Trace output should be identical as a result. Bug: 782268 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I90c6232c9001bea29507d9bdd07dd309527d96d3 Reviewed-on: https://chromium-review.googlesource.com/768967 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: Eric Seckler <eseckler@chromium.org> Reviewed-by: Brian Anderson <brianderson@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Cr-Commit-Position: refs/heads/master@{#516501} [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/cc/ipc/cc_param_traits.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/common/frame_sinks/begin_frame_args.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/common/frame_sinks/begin_frame_args.h [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/common/frame_sinks/begin_frame_source.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/common/frame_sinks/begin_frame_source.h [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/service/display/display_scheduler.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/test/begin_frame_args_test.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/components/viz/test/begin_frame_args_test.h [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/headless/lib/browser/headless_web_contents_impl.h [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/services/viz/public/cpp/compositing/begin_frame_args_struct_traits.h [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/services/viz/public/cpp/compositing/struct_traits_unittest.cc [modify] https://crrev.com/bc94c9572ccb6fab46ae4308637dc98f1318cd94/services/viz/public/interfaces/compositing/begin_frame_args.mojom
,
Nov 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2794bb138af38fbcd1768361b0c10732736ef618 commit 2794bb138af38fbcd1768361b0c10732736ef618 Author: kylechar <kylechar@chromium.org> Date: Thu Nov 16 16:08:16 2017 Add restart_id to BeginFrameSource::source_id_. When running with --enable-viz the display compositor is relocated to the GPU process. When the GPU process crashes the display compositor is restarted and the BeginFrameSource for each viz::Display is recreated. This leads to a problem where |source_id_| gets reused and BeginFrameObservers DCHECK on a BeginFrameArgs with the same source_id but reset sequence_number. Combine a 32 bit atomic sequence and 32 bit |restart_id| to get the BeginFrameSource::source_id_. Only BeginFrameSources used in the GPU need this functionality, so |restart_id| is only plumbed through for DelayBasedBeginFrameSource. Other BeginFrameSources use kNotRestartableId. It will be used in a followup CL from GpuDisplayProvider. Bug: 782268 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I1ef924f43e5fc21a70a0dc96a756929db60626af Reviewed-on: https://chromium-review.googlesource.com/755893 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Brian Anderson <brianderson@chromium.org> Cr-Commit-Position: refs/heads/master@{#517097} [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/cc/scheduler/scheduler_unittest.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/common/frame_sinks/begin_frame_source.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/common/frame_sinks/begin_frame_source.h [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/common/frame_sinks/begin_frame_source_unittest.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/service/frame_sinks/primary_begin_frame_source.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/test/fake_external_begin_frame_source.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/components/viz/test/test_layer_tree_frame_sink.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/content/browser/compositor/gpu_process_transport_factory.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/content/browser/compositor/reflector_impl_unittest.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/content/browser/compositor/software_browser_compositor_output_surface_unittest.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/content/renderer/gpu/compositor_external_begin_frame_source.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/ui/android/window_android.cc [modify] https://crrev.com/2794bb138af38fbcd1768361b0c10732736ef618/ui/compositor/test/in_process_context_factory.cc
,
Nov 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9aa229ee07b44f59cff6dcc4c3ee297dd619620a commit 9aa229ee07b44f59cff6dcc4c3ee297dd619620a Author: kylechar <kylechar@chromium.org> Date: Fri Nov 17 18:06:13 2017 viz: Plumb process_restart_id for BeginFrameSources. With the display compositor running in the GPU process, we need to send a unique id from the host process to the GPU process to ensure that BeginFrameSource::source_ids_ are different after process restart. Pass this value along with the other arguments needed to create a viz::FrameSinkManagerImpl. |restart_id| is plumbed into DelayBasedBeginFrameSources that are created for each viz::Display. There are other BeginFrameSources that are created in the GPU process, but these BeginFrameSources don't produce BeginFrameArgs so their source_id isn't used and isn't important. Bug: 782268 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel Change-Id: Ie3bd4f6bf3a9ba97d370969cae3d7faa396a0f43 Reviewed-on: https://chromium-review.googlesource.com/766512 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Cr-Commit-Position: refs/heads/master@{#517468} [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/components/viz/service/display_embedder/gpu_display_provider.h [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/components/viz/service/main/viz_main_impl.cc [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/components/viz/service/main/viz_main_impl.h [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/content/browser/gpu/gpu_process_host.cc [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/ui/ws/gpu_host.cc [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/ui/ws/gpu_host.h [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/ui/ws/test_gpu_host.cc [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/ui/ws/test_gpu_host.h [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/ui/ws/window_server.cc [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/ui/ws/window_server.h [modify] https://crrev.com/9aa229ee07b44f59cff6dcc4c3ee297dd619620a/services/viz/privileged/interfaces/viz_main.mojom
,
Nov 20 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by laforge@google.com
, Nov 8 2017