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

Issue 837030 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 843400

Blocking:
issue 759031



Sign in to add a comment

ParentLocalSurfaceIdAllocator should default initialize a valid LSI

Project Member Reported by cblume@chromium.org, Apr 26 2018

Issue description

In a number of places we see a pattern where we first check to see if the parent allocator contains a valid LSI. If not, allocate one.

This pattern can be replaced by always having the parent allocator default initialize to a valid LSI.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 30 2018

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

commit 9947cd2d5455a23119b35555c4ad873a3979cd30
Author: Chris Blume <cblume@chromium.org>
Date: Mon Apr 30 23:54:32 2018

Unit test to mirror actual behavior

The ParentLocalSurfaceIdAllocator unit tests have a function to get a
fake child-allocated LocalSurfaceId. It hard-codes values.

The hard-coded parent sequence number is bad but not terrible. It forces
knowledge of the calling functions. This isn't good. It can be improved.

However, the embed token part is pretty bad. It forces the unit tests to
assert the wrong behavior. A child allocation should use the parent's
embed token. Since the hard-coded value is different, the calling tests
assert that the embed tokens don't match when in reality they should.

To fix these bad behaviors, the fake child allocation should take a
parent allocator (or parent-allocated LocalSurfaceId) so the values can
be based on what the test provides.

BUG= 837030 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Iaa8e7b6226c0a6c6d67fa420183f24a36e7499ce
Reviewed-on: https://chromium-review.googlesource.com/1036530
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Chris Blume <cblume@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554928}
[modify] https://crrev.com/9947cd2d5455a23119b35555c4ad873a3979cd30/components/viz/common/surfaces/parent_local_surface_id_allocator_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 1 2018

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

commit 387823429f16759f283bddac0ecb74a3d77bb532
Author: Chris Blume <cblume@chromium.org>
Date: Tue May 01 02:08:55 2018

Default initialize LSI inside parent allocator

Currently, the ParentLocalSurfaceIdAllocator default initializes with an
invalid last-known LocalSurfaceId. However, this has no real use or
benefit.

Clients often have to check if the last-known LocalSurfaceId is not
valid before forcing the initial allocation via GenerateId().

It would make sense for the ParentLocalSurfaceIdAllocator to default
initialize with a generated LocalSurfaceId. This patch does that.

BUG= 837030 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I58c7c8af8c338675d901fa9d99e0bc157d58e935
Reviewed-on: https://chromium-review.googlesource.com/1036598
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Chris Blume <cblume@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554973}
[modify] https://crrev.com/387823429f16759f283bddac0ecb74a3d77bb532/components/viz/common/surfaces/local_surface_id.h
[modify] https://crrev.com/387823429f16759f283bddac0ecb74a3d77bb532/components/viz/common/surfaces/parent_local_surface_id_allocator.cc
[modify] https://crrev.com/387823429f16759f283bddac0ecb74a3d77bb532/components/viz/common/surfaces/parent_local_surface_id_allocator_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 1 2018

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

commit 9ba0c59ad16b330c8abe1dba69053378ecf859ce
Author: Chris Blume <cblume@chromium.org>
Date: Tue May 01 23:10:42 2018

LocalSurfaceIdProvider to use default LSI

Previously, ParentLocalSurfaceIdAllocator would default to an invalid
LocalSurfaceId. A new LSI had to be explicitly allocated before the
normal behavior could use it.

With ParentLocalSurfaceIdAllocator now default initializing a valid LSI,
the various call sites no longer need to explicitly check for the
uninitialized state.

LocalSurfaceIdProvider is one of those call sites.

BUG= 837030 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ifec19249e74d2a705b54fec4d5d350ee435d87b0
Reviewed-on: https://chromium-review.googlesource.com/1036794
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555218}
[modify] https://crrev.com/9ba0c59ad16b330c8abe1dba69053378ecf859ce/components/viz/client/local_surface_id_provider.cc

Blocking: 759031
Project Member

Comment 5 by bugdroid1@chromium.org, May 2 2018

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

commit 0a8e95b3259ed4b610d2c51a281f28d7a39217cc
Author: Chris Blume <cblume@chromium.org>
Date: Wed May 02 00:18:55 2018

Blink's platform/graphics to use default LSI

Previously, ParentLocalSurfaceIdAllocator would default to an invalid
LocalSurfaceId. A new LSI had to be explicitly allocated before the
normal behavior could use it.

With ParentLocalSurfaceIdAllocator now default initializing a valid LSI,
the various call sites no longer need to explicitly check for the
uninitialized state.

Blink's platform/graphics have some of those call sites.

BUG= 837030 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ie8ec4f6d09e357e9433e583e0a79abb72de73385
Reviewed-on: https://chromium-review.googlesource.com/1038140
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Chris Blume <cblume@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555243}
[modify] https://crrev.com/0a8e95b3259ed4b610d2c51a281f28d7a39217cc/third_party/blink/renderer/platform/graphics/offscreen_canvas_frame_dispatcher.cc
[modify] https://crrev.com/0a8e95b3259ed4b610d2c51a281f28d7a39217cc/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc

Project Member

Comment 6 by bugdroid1@chromium.org, May 2 2018

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

commit 45c5c213b352402a1feff1fb61728672fb11fe42
Author: Chris Blume <cblume@chromium.org>
Date: Wed May 02 00:43:55 2018

RenderThreadImpl to use default LSI

Previously, ParentLocalSurfaceIdAllocator would default to an invalid
LocalSurfaceId. A new LSI had to be explicitly allocated before the
normal behavior could use it.

With ParentLocalSurfaceIdAllocator now default initializing a valid LSI,
the various call sites no longer need to explicitly check for the
uninitialized state.

RenderThreadImpl is one of those call sites.

BUG= 837030 

Change-Id: Ia1ee9524027d512980f97d08bfc61e93536e3b26
Reviewed-on: https://chromium-review.googlesource.com/1038060
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Chris Blume <cblume@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555258}
[modify] https://crrev.com/45c5c213b352402a1feff1fb61728672fb11fe42/content/renderer/render_thread_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 2 2018

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

commit 932d1d0e30fbcf55afb0962298f7fff1dc26f387
Author: Chris Blume <cblume@chromium.org>
Date: Wed May 02 04:11:01 2018

viz/service & viz/test to use default LSI

Previously, ParentLocalSurfaceIdAllocator would default to an invalid
LocalSurfaceId. A new LSI had to be explicitly allocated before the
normal behavior could use it.

With ParentLocalSurfaceIdAllocator now default initializing a valid LSI,
the various call sites no longer need to explicitly check for the
uninitialized state.

viz/service & viz/test contain some of those call sites.

BUG= 837030 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Iace37fb0be4c6389fdf2c89dbd4bb77c9696036f
Reviewed-on: https://chromium-review.googlesource.com/1038124
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555306}
[modify] https://crrev.com/932d1d0e30fbcf55afb0962298f7fff1dc26f387/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/932d1d0e30fbcf55afb0962298f7fff1dc26f387/components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc
[modify] https://crrev.com/932d1d0e30fbcf55afb0962298f7fff1dc26f387/components/viz/service/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/932d1d0e30fbcf55afb0962298f7fff1dc26f387/components/viz/test/test_layer_tree_frame_sink.cc

Comment 8 by kbr@chromium.org, May 16 2018

Blockedon: 843400
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 23

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

commit 53201f58454ff34e0a76081f691b0fe6439b888d
Author: Chris Blume <cblume@chromium.org>
Date: Thu Aug 23 01:52:24 2018

Render Widget to use default LSI

Previously, ParentLocalSurfaceIdAllocator would default to an invalid
LocalSurfaceId. A new LSI had to be explicitly allocated before the
normal behavior could use it.

With ParentLocalSurfaceIdAllocator now default initializing a valid LSI,
the various call sites no longer need to explicitly check for the
uninitialized state.

Render Widget is one of those call sites.

BUG= 837030 

Change-Id: I330d2c47492f1ac577a928f20a8e8e4e14f91b29
Reviewed-on: https://chromium-review.googlesource.com/1038127
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585361}
[modify] https://crrev.com/53201f58454ff34e0a76081f691b0fe6439b888d/content/browser/renderer_host/render_widget_host_view_child_frame_unittest.cc
[modify] https://crrev.com/53201f58454ff34e0a76081f691b0fe6439b888d/content/renderer/render_widget_browsertest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 24

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

commit 797dc685db99404da576f6142e727c8f4fb8f0f1
Author: Chris Blume <cblume@chromium.org>
Date: Fri Aug 24 18:56:00 2018

services/ui/ to use default LSI

Previously, ParentLocalSurfaceIdAllocator would default to an invalid
LocalSurfaceId. A new LSI had to be explicitly allocated before the
normal behavior could use it.

With ParentLocalSurfaceIdAllocator now default initializing a valid LSI,
the various call sites no longer need to explicitly check for the
uninitialized state.

services/ui/ has some of those call sites.

BUG= 837030 

Change-Id: I1b82b0133de0ecc3e8fa71f5a40f25762732ccf3
Reviewed-on: https://chromium-review.googlesource.com/1038144
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585927}
[modify] https://crrev.com/797dc685db99404da576f6142e727c8f4fb8f0f1/services/ui/ws2/client_root.cc
[modify] https://crrev.com/797dc685db99404da576f6142e727c8f4fb8f0f1/services/ui/ws2/window_tree_client_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment