New issue
Advanced search Search tips
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 906794
issue 897214

Blocking:
issue 829435
issue 898270
issue 924592


Participants' hotlists:
shared-image-client


Sign in to add a comment
link

Issue 882513: Teach cc/ compositor to use SharedImage

Reported by backer@chromium.org, Sep 10 Project Member

Issue description

This task comes from piman@ shareable image design doc (https://docs.google.com/document/d/12qYPeN819JkdNGbPcKBA0rfPXSOIE3aIaQVrAZ4I1lM).

cc/ creates a texture, populates the texture (rastering), and passes the texture to the display compositor via mailboxes. Idea is to change the client API from explicit client side texture allocation to using SharedImageInterface instead.

WIP patch for GPU rasterization is here: https://chromium-review.googlesource.com/c/chromium/src/+/1178977

Other parts that need to changes are:

- HUD layer: uses TextureAllocation to create a texture, its storage, then export to mailbox. Change TextureAllocation to deal with Shareable Images instead (explicit image allocation + import). Scope fairly trivial (within the same function).

- UI Resources: uses TextureAllocation to create texture/storage & export. Similar to HUD layer.

- Gpu raster: uses TextureAllocation, but separates texture id creation and mailbox (on compositor thread/context) from storage (on raster worker). This is due to lifetime issues + GLES2Interface/RasterInterface thread affinity, that should disappear with Shareable Images, should be able to create on worker thread. See consumer for scope.

- One-copy raster: same as Gpu raster.

- Zero-copy raster: texture/mailbox creation next to GLImage creation. Change to create shareable image out of GMB (likely no import to GL needed).
 

Comment 1 by bugdroid1@chromium.org, Sep 20

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

commit 09c2f5373c61735db0dfedffb96608212f0d9e44
Author: Antoine Labour <piman@chromium.org>
Date: Thu Sep 20 15:36:56 2018

Remove support for ETC1 tiles

This path has been disabled since crrev.com/c/1024703, so remove dead
code.

Bug:  882513 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I2b6e076bd97b62464c3c1d9508827f7ac4be4561
Reviewed-on: https://chromium-review.googlesource.com/1234238
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592809}
[modify] https://crrev.com/09c2f5373c61735db0dfedffb96608212f0d9e44/cc/BUILD.gn
[modify] https://crrev.com/09c2f5373c61735db0dfedffb96608212f0d9e44/cc/raster/one_copy_raster_buffer_provider.cc
[modify] https://crrev.com/09c2f5373c61735db0dfedffb96608212f0d9e44/cc/raster/raster_buffer_provider.cc
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor.cc
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor.h
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor_etc1.cc
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor_etc1.h
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor_etc1_sse.cc
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor_etc1_sse.h
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor_etc1_unittest.cc
[delete] https://crrev.com/f84cf7edd6167f90ea801cfb17fdfd42d0ed337e/cc/raster/texture_compressor_perftest.cc

Comment 2 by piman@chromium.org, Sep 28

Blocking: 829435

Comment 4 by piman@chromium.org, Oct 19

Blockedon: 897214

Comment 5 by rjkroege@chromium.org, Oct 24

Blocking: 898270
Labels: vulkanize

Comment 6 by rjkroege@chromium.org, Oct 29

Labels: -vulkanize Proj-Vulkanize

Comment 7 by bugdroid1@chromium.org, Oct 29

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

commit 6363d820305bcf1d3fbeb9d031bb929e062a7de5
Author: Antoine Labour <piman@chromium.org>
Date: Mon Oct 29 22:36:01 2018

Saner fallback path when RasterBuffer failed to create a GPU resource

Currently, in particular on Mac, CreateGpuMemoryBuffer can fail, in which case
we can't rasterize the corresponding tile in ZeroCopyRasterBufferImpl, and we
just create a GL texture without binding anything to it, which ends up showing
black when we display. This is somewhat problematic to emulate with SharedImage
where it is not a valid state (SharedImages must have well-defined dimensions
and format).
Instead, we handle the failure by not creating a texture at all, keeping a zero
mailbox, and falling back to OOM rendering mode (checkerboarding) for the
corresponding tile.

Bug:  882513 , 554541
Change-Id: Iffd29a660f655c2dee8179fd90baa2a15ae42bf7
Reviewed-on: https://chromium-review.googlesource.com/c/1303393
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603661}
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/layers/heads_up_display_layer_impl.cc
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/raster/zero_copy_raster_buffer_provider.cc
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/resources/resource_pool.cc
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/resources/resource_pool.h
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/resources/resource_pool_unittest.cc
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/tiles/tile_manager.cc
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/tiles/tile_manager.h
[modify] https://crrev.com/6363d820305bcf1d3fbeb9d031bb929e062a7de5/cc/tiles/tile_manager_unittest.cc

Comment 8 by bugdroid1@chromium.org, Nov 7

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

commit e5a2101affba7a813731b399a9e102b86a7f27c6
Author: Antoine Labour <piman@chromium.org>
Date: Wed Nov 07 23:36:27 2018

Use SharedImageInterface in ZeroCopyRasterBufferProvider

Bug:  882513 ,  897214 
Change-Id: I3d7dc804db292b7a9844eda856195b7b7b1450d3
Reviewed-on: https://chromium-review.googlesource.com/c/1316347
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606210}
[modify] https://crrev.com/e5a2101affba7a813731b399a9e102b86a7f27c6/cc/raster/zero_copy_raster_buffer_provider.cc

Comment 9 by bugdroid1@chromium.org, Nov 8

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

commit 7a950ebc842cbbc4377247e01de4dbcdaf9cfaa2
Author: Antoine Labour <piman@chromium.org>
Date: Thu Nov 08 21:59:49 2018

Fix TileResourceFreedIfLostWhileExported

After switching one-copy to SharedImageInterface,
TileResourceFreedIfLostWhileExported lost its meaning as it's only
counting staging textures instead of resource textures. This restores
the behavior by making sure we ask the SharedImageInterface on the
worker context (where they are created now) instead of the
GLES2Interface on the compositor context.

Bug:  882513 
Change-Id: I1257c9e130bfa615d56602b0ce74f2830d5a55f5
Reviewed-on: https://chromium-review.googlesource.com/c/1325087
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606614}
[modify] https://crrev.com/7a950ebc842cbbc4377247e01de4dbcdaf9cfaa2/cc/trees/layer_tree_host_unittest_context.cc
[modify] https://crrev.com/7a950ebc842cbbc4377247e01de4dbcdaf9cfaa2/components/viz/test/test_context_provider.cc
[modify] https://crrev.com/7a950ebc842cbbc4377247e01de4dbcdaf9cfaa2/components/viz/test/test_context_provider.h

Comment 10 by bugdroid1@chromium.org, Nov 12

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

commit 3cb1d430ef07ac47baeca1ad607b870294ff06c1
Author: Antoine Labour <piman@chromium.org>
Date: Mon Nov 12 19:37:39 2018

MailboxManagerSync: don't try to share textures with images

We can't bind 2 GLImage to the same textures, and it wouldn't work anyway.
Ensure we don't attempt to create an EGLImage in that case as it will break if
e.g. the image is in UNBOUND state.

Bug:  882513 
Change-Id: Iea52d25771a43205a6250b0d354593af98676a86
Reviewed-on: https://chromium-review.googlesource.com/c/1330765
Reviewed-by: Jonathan Backer <backer@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607304}
[modify] https://crrev.com/3cb1d430ef07ac47baeca1ad607b870294ff06c1/gpu/command_buffer/service/mailbox_manager_sync.cc
[modify] https://crrev.com/3cb1d430ef07ac47baeca1ad607b870294ff06c1/gpu/command_buffer/service/texture_definition.cc

Comment 11 by bugdroid1@chromium.org, Nov 13

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

commit 9058f7866e74b92eeef1c30780522fba0740d2f3
Author: Antoine Labour <piman@chromium.org>
Date: Tue Nov 13 02:36:31 2018

Use SharedImageInterface for one-copy staging buffers

Bug:  882513 
Change-Id: I7643ae90356a4a98f57cb5e14691af4362ad9b83
Reviewed-on: https://chromium-review.googlesource.com/c/1323719
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607454}
[modify] https://crrev.com/9058f7866e74b92eeef1c30780522fba0740d2f3/cc/raster/one_copy_raster_buffer_provider.cc
[modify] https://crrev.com/9058f7866e74b92eeef1c30780522fba0740d2f3/cc/raster/staging_buffer_pool.cc
[modify] https://crrev.com/9058f7866e74b92eeef1c30780522fba0740d2f3/cc/raster/staging_buffer_pool.h

Comment 12 by ericrk@chromium.org, Nov 19

Blockedon: 906794
FYI, I'm taking a look at the UI Resource Uploader part of this bug. Factored into a sub-issue: issue 906794

Comment 13 by piman@chromium.org, Jan 11

I believe this is complete? The UI resource uploader part is done as far as cc/ is concerned (only missing piece is service side).

Comment 14 by backer@chromium.org, Jan 14

Status: Fixed (was: Assigned)
Yes. Let's call this done.

Comment 15 by piman@chromium.org, Jan 23

Blocking: 924592

Sign in to add a comment