New issue
Advanced search Search tips

Issue 768976 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Remove non-immutable from ResourceProvider

Project Member Reported by danakj@chromium.org, Sep 26 2017

Issue description

It's complexity we don't need to use anymore.
 

Comment 1 by danakj@chromium.org, Sep 26 2017

piman:

We should remove non-immutable textures in ResourceProvider
there's only 2 places, and I think they can both be fixed
GLRenderer::GetBackdropTexture is trivial, we just need GetFramebufferTexture to be able to do a CopyTexSubImage2D instead of a CopyTexImage2D
the other one is VideoResourceUpdater::CopyPlaneTexture
I'm pretty sure it doesn't need to be immutable
non-immutable I mean
(triple negative for ya)
because all we do on it is CopySubTextureCHROMIUM


Project Member

Comment 2 by bugdroid1@chromium.org, Sep 26 2017

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

commit 19e92b540233952917483de886c7e5579c60b946
Author: danakj <danakj@chromium.org>
Date: Tue Sep 26 21:08:03 2017

Use immutable textures in VideoResourceUpdater.

The upload uses CopySubTextureCHROMIUM which is compatible with
immutable textures, as it does not change the texture size. It used
to use CopyTextureCHROMIUM, which is not compatible, and had a unit
test describing this. But in reality this has changed, so always make
the resource immutible, and stop checking for that when recycling.

R=piman@chromium.org

Bug:  768976 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia9a31769fcc6c513291b6e67be4dea07d1c02168
Reviewed-on: https://chromium-review.googlesource.com/685742
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504481}
[modify] https://crrev.com/19e92b540233952917483de886c7e5579c60b946/cc/resources/video_resource_updater.cc
[modify] https://crrev.com/19e92b540233952917483de886c7e5579c60b946/cc/resources/video_resource_updater.h
[modify] https://crrev.com/19e92b540233952917483de886c7e5579c60b946/cc/resources/video_resource_updater_unittest.cc

Comment 3 by danakj@chromium.org, Sep 26 2017

For the GLRenderer::GetBackdropTexture it's harder because we have to use the RGB or RGBA format if it's coming from the OutputSurface.

So probably the easiest thing is to stop using ResourceProvider at all for the backdrop texture. It's internal to GLRenderer (not used in DirectRenderer) so no need for the Resource abstraction.

This would remove one case of the "LocalResource" paradigm being proposed in other CLs.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 27 2017

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

commit 3d2a2f85d6a0da2c3f25eed982c5349658bb14be
Author: danakj <danakj@chromium.org>
Date: Wed Sep 27 19:33:57 2017

Remove use of non-immutable ResourceProvider resources in GLRenderer

GLRenderer uses a non-immutable resource when copying the current
RenderPass to a texture, as the storage format is not explicit when
copying from the OutputSurface backbuffer. In that case the buffer
can be RGB so we need to define the format of the texture as RGB
(you can't add a component when copying), but the ResourceProvider
does not support RGB textures, and adding it everywhere there would
be unneeded complexity.

Instead, we make GLRenderer stop using ResourceProvider for this use
case. Since it is isolated to the GLRenderer subclass (DirectRenderer
does not need to know about these textures), we don't need to use a
"ResourceId" abstraction.

This adds a GetFramebufferCopyTextureFormat() method to GLRenderer
that returns the GL format to use for a texture if copying from the
current Renderpasss output.

This removes the GetFramebufferTexture() method, and moves the call
to CopyTexImage2D() out to the two callsites:
- In the CopyOutputRequest code, we already have a texture id, and
just need to bind it and call CopyTexImage2D.
- In GetBackdropTexture(), we create a new texture id, set up its
filters, and bind and CopyTexImage2D.
- Both of these use the GetFramebufferCopyTextureFormat() to choose
the best format for the copy output.

Since we have a raw texture id, we replace uses ResourceProvider
locks and ScopedResource throughout, and ensure to delete the
texture ID when we set it to 0, or when we destroy the |params|
at the end of GLRenderer::CopyRenderPassDrawQuadToOverlayResource
and GLRenderer::DrawRenderPassDrawQuad.

R=piman@chromium.org

Bug:  768976 
Change-Id: I14ee01a767416da9210d80a73c39ef1d2022c58c
Reviewed-on: https://chromium-review.googlesource.com/685300
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504753}
[modify] https://crrev.com/3d2a2f85d6a0da2c3f25eed982c5349658bb14be/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/3d2a2f85d6a0da2c3f25eed982c5349658bb14be/components/viz/service/display/gl_renderer.h

Comment 5 by danakj@chromium.org, Sep 27 2017

Cc: -xing.xu@chromium.org xing...@intel.com
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 28 2017

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

commit 238671a55ba2349a56bee89afd693efd2a3cc184
Author: danakj <danakj@chromium.org>
Date: Thu Sep 28 00:44:31 2017

cc: DEFAULT is the new IMMUTABLE.

Since all resources created with ResourceProvider are now immutable,
we have no need for the texture hint to request immutability. So remove
the hint, and make the DEFAULT behaviour create immutable resources
whenever the context supports it for the format.

All clients of ResourceProvider will need to assume resources are
always immutable. Add some class-level documentation on
ResourceProvider to mention this.

R=piman@chromium.org

Bug:  768976 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I75f98f8225cbde1a38910e3319c641a3a4fb2db6
Reviewed-on: https://chromium-review.googlesource.com/688098
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504829}
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/layers/heads_up_display_layer_impl.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/layers/texture_layer_impl.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/raster/raster_buffer_provider_perftest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/raster/raster_buffer_provider_unittest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/display_resource_provider.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/resource_pool_unittest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/resource_provider.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/resource_provider.h
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/resource_provider_unittest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/scoped_resource.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/scoped_resource_unittest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/resources/video_resource_updater.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/test/fake_picture_layer_tiling_client.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/test/fake_ui_resource_layer_tree_host_impl.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/test/layer_tree_pixel_resource_test.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/test/render_pass_test_utils.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/components/viz/service/display/gl_renderer_unittest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/components/viz/service/display/renderer_pixeltest.cc
[modify] https://crrev.com/238671a55ba2349a56bee89afd693efd2a3cc184/components/viz/service/display/software_renderer_unittest.cc

Comment 7 by danakj@chromium.org, Sep 28 2017

Status: Fixed (was: Started)

Sign in to add a comment