New issue
Advanced search Search tips

Issue 782044 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 738190



Sign in to add a comment

Implement local resources

Project Member Reported by xing...@intel.com, Nov 7 2017

Issue description

Local resources are resources used inside the same GL context and will not being
sent into another process. So no need to create fence and mailbox for these resources.

The detailed plan is here:
https://docs.google.com/document/d/1i_xe5dTx2oUHbQI4rSmLcehV7xlI5oOq85ZMscRZQWc/edit
 
Could it just be a struct like this?

struct RenderPassTexture {
  GLuint id;
  gfx::Size size;
}

I'm not sure we need to track all the other things along with the id?
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 9 2017

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

commit 43d70d5dd490d5bf146cbb35c79a53edcbdf3a25
Author: danakj <danakj@chromium.org>
Date: Thu Nov 09 01:12:34 2017

When calling UseRenderPass() to restore, ensure it runs.

UseRenderPass() has two use cases, either to set up a render pass
as the framebuffer for drawing at the start of the draw, or to restore
us back to that state after making some intermediate change to which
framebuffer is bound.

UseRenderPass() has some optimizations where it can return false and
not do anything, if it detects that we don't want to draw the render
pass. The first use case, in DrawRenderPass() makes use of this return
value to avoid drawing anything.

Other callers to UseRenderPass() do not check the return value because
we're already drawing the pass and they are trying to restore state. If
it early outs for these it would be unexpected and produce bugs.

Currently DecideRenderPassAllocationsForFrame modifies render passes to
try avoid such bugs, but there are other ways to encounter the bug.

This changes UseRenderPass() to never early out so the latter cases will
not avoid resetting the framebuffer. The former case can avoid
UseRenderPass() entirely by checking CanSkipRenderPass(). If it is true,
then it does not need to be drawn.

Also, we will now ensure that UseRenderPass() is called after using a
ScopedUseGrContext when drawing a RenderPassDrawQuad. Currently on errors
it would early out before restoring the framebuffer, which could cause
bugs for the next quads in the RenderPass. This was introduced when the
UseRenderPass() call was moved out of the ~ScopedUseGrContext()
destructor in https://codereview.chromium.org/2203033005.

This all came up via https://chromium-review.googlesource.com/c/chromium/src/+/757027#message-b6e905925e834419ff448e2093a392053d5d321a

R=enne@chromium.org, wutao@chromium.org

Bug:  782044 ,  738190 , 782042
Change-Id: Ib4662c5fee3f326642add12bbfe3d51e51dff1c9
Reviewed-on: https://chromium-review.googlesource.com/757049
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515043}
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/direct_renderer.h
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/gl_renderer.h
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/skia_renderer.h
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/software_renderer.cc
[modify] https://crrev.com/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25/components/viz/service/display/software_renderer.h

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 9 2017

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

commit 6dcd9ac14c9e5fbb60c659143b40bcc54309486e
Author: Ned Nguyen <nednguyen@google.com>
Date: Thu Nov 09 16:08:22 2017

Revert "When calling UseRenderPass() to restore, ensure it runs."

This reverts commit 43d70d5dd490d5bf146cbb35c79a53edcbdf3a25.

Reason for revert: cause system_health.memory_desktop.browse:media:youtube to crash

(BUG:783198)

Original change's description:
> When calling UseRenderPass() to restore, ensure it runs.
> 
> UseRenderPass() has two use cases, either to set up a render pass
> as the framebuffer for drawing at the start of the draw, or to restore
> us back to that state after making some intermediate change to which
> framebuffer is bound.
> 
> UseRenderPass() has some optimizations where it can return false and
> not do anything, if it detects that we don't want to draw the render
> pass. The first use case, in DrawRenderPass() makes use of this return
> value to avoid drawing anything.
> 
> Other callers to UseRenderPass() do not check the return value because
> we're already drawing the pass and they are trying to restore state. If
> it early outs for these it would be unexpected and produce bugs.
> 
> Currently DecideRenderPassAllocationsForFrame modifies render passes to
> try avoid such bugs, but there are other ways to encounter the bug.
> 
> This changes UseRenderPass() to never early out so the latter cases will
> not avoid resetting the framebuffer. The former case can avoid
> UseRenderPass() entirely by checking CanSkipRenderPass(). If it is true,
> then it does not need to be drawn.
> 
> Also, we will now ensure that UseRenderPass() is called after using a
> ScopedUseGrContext when drawing a RenderPassDrawQuad. Currently on errors
> it would early out before restoring the framebuffer, which could cause
> bugs for the next quads in the RenderPass. This was introduced when the
> UseRenderPass() call was moved out of the ~ScopedUseGrContext()
> destructor in https://codereview.chromium.org/2203033005.
> 
> This all came up via https://chromium-review.googlesource.com/c/chromium/src/+/757027#message-b6e905925e834419ff448e2093a392053d5d321a
> 
> R=​enne@chromium.org, wutao@chromium.org
> 
> Bug:  782044 ,  738190 , 782042
> Change-Id: Ib4662c5fee3f326642add12bbfe3d51e51dff1c9
> Reviewed-on: https://chromium-review.googlesource.com/757049
> Commit-Queue: danakj <danakj@chromium.org>
> Reviewed-by: enne <enne@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#515043}

TBR=danakj@chromium.org,enne@chromium.org,wutao@chromium.org

Change-Id: I2acd86c21290c053b1801732599c1ab355161469
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  782044 ,  738190 , 782042
Reviewed-on: https://chromium-review.googlesource.com/760557
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#515178}
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/direct_renderer.h
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/gl_renderer.h
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/skia_renderer.h
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/software_renderer.cc
[modify] https://crrev.com/6dcd9ac14c9e5fbb60c659143b40bcc54309486e/components/viz/service/display/software_renderer.h

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 10 2017

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

commit 986d87a5fada7910886f1fc07e592c7698da128f
Author: danakj <danakj@chromium.org>
Date: Fri Nov 10 19:30:54 2017

When calling UseRenderPass() to restore, ensure it runs.

UseRenderPass() has two use cases, either to set up a render pass
as the framebuffer for drawing at the start of the draw, or to restore
us back to that state after making some intermediate change to which
framebuffer is bound.

UseRenderPass() has some optimizations where it can return false and
not do anything, if it detects that we don't want to draw the render
pass. The first use case, in DrawRenderPass() makes use of this return
value to avoid drawing anything.

Other callers to UseRenderPass() do not check the return value because
we're already drawing the pass and they are trying to restore state. If
it early outs for these it would be unexpected and produce bugs.

Currently DecideRenderPassAllocationsForFrame modifies render passes to
try avoid such bugs, but there are other ways to encounter the bug.

This changes UseRenderPass() to never early out so the latter cases will
not avoid resetting the framebuffer. The former case can avoid
UseRenderPass() entirely by checking CanSkipRenderPass(). If it is true,
then it does not need to be drawn.

Also, we will now ensure that UseRenderPass() is called after using a
ScopedUseGrContext when drawing a RenderPassDrawQuad. Currently on errors
it would early out before restoring the framebuffer, which could cause
bugs for the next quads in the RenderPass. This was introduced when the
UseRenderPass() call was moved out of the ~ScopedUseGrContext()
destructor in https://codereview.chromium.org/2203033005.

When skipping a RenderPass, current_render_pass is no longer set, which
causes a crash in code that assumes it is, incorrectly. So this updates
some paths in RenderPassDrawQuad drawing to avoid crashing, by passing
the colorspace directly instead of trying to pull it off
current_render_pass, since the caller knows where to find the correct
one, and by using the root_render_pass instead for copying
RenderPassDrawQuads to textures, since they draw into the root.

This all came up via https://chromium-review.googlesource.com/c/chromium/src/+/757027#message-b6e905925e834419ff448e2093a392053d5d321a

R=enne@chromium.org, wutao@chromium.org

Bug:  782044 ,  738190 , 782042,  783087 
Change-Id: I62ea68be6b3008ec8b8370466db0e26321f022f9
Reviewed-on: https://chromium-review.googlesource.com/760558
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515633}
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/direct_renderer.h
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer.h
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer_unittest.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/renderer_pixeltest.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/skia_renderer.h
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/software_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/software_renderer.h

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 11 2017

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

commit bd829cadf22696834edf4aefc9033f5c8e272183
Author: danakj <danakj@chromium.org>
Date: Sat Nov 11 00:19:32 2017

Remove caching avoidance when there is a non-empty scissor.

See https://chromium-review.googlesource.com/c/chromium/src/+/760558#message-83efb689457770dfabb6d7371847f06f8091dfd7
and https://bugs.chromium.org/p/chromium/issues/detail?id=783087#c24
for explanations, but tl;dr this check would break caching, and
shouldn't be there. Before it was used to skip render passes
regardless of caching, but incorrectly, and the unittest in
https://chromium-review.googlesource.com/c/chromium/src/+/760558
demonstrates that, and fails with that check.

This removes the check from the caching block.

R=enne@chromium.org, wutao@chromium.org

Bug:  782044 ,  738190 , 782042,  783087 
Change-Id: Idc5baea6da5e6d663da9c1aa23ce81a55e61168d
Reviewed-on: https://chromium-review.googlesource.com/763710
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515757}
[modify] https://crrev.com/bd829cadf22696834edf4aefc9033f5c8e272183/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/bd829cadf22696834edf4aefc9033f5c8e272183/components/viz/service/display/gl_renderer_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 11 2017

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

commit 493796597d3da47f760263d6d1845bda759ed292
Author: Xu Xing <xing.xu@intel.com>
Date: Sat Nov 11 12:48:55 2017

[viz] Local resource: Move render pass textures into subclass of DirectRenderer

Other refinement:
Use texture id directly instead of ScopedReadLockGL in ApplyImageFilter.
BUG= 782044 

Change-Id: I0e357a5c5831eaf5709392e852cf4e66ba2d0253
Reviewed-on: https://chromium-review.googlesource.com/749101
Commit-Queue: Xing Xu <xing.xu@intel.com>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515840}
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/direct_renderer.h
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/gl_renderer.h
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/gl_renderer_unittest.cc
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/skia_renderer.h
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/software_renderer.cc
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/software_renderer.h
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/vulkan_renderer.cc
[modify] https://crrev.com/493796597d3da47f760263d6d1845bda759ed292/components/viz/service/display/vulkan_renderer.h

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 22 2017

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

commit 9cc56b283e51de94cc373a77214da2bb646efcbd
Author: Xu Xing <xing.xu@intel.com>
Date: Fri Dec 22 02:20:01 2017

[viz] Implement local resource in GLRenderer

Local resource is resource used inside the same GL context and will not being sent
into another process, no need to create fence and mailbox for these resources.
For RenderPass in render_pass_textures_, it uses texture-backed resources, no need
to support GpuMemoryBuffer.

Currently there are three path may possible set is_overlay_ to true:
ResourceProvider::CreateGpuMemoryBufferResource
LayerTreeResourceProvider::CreateResourceFromTextureMailbox
ResourceProvider::CreateGpuTextureResource (Used by GLRenderer::overlay_resource_pool_).
Resource for RenderPass in render_pass_textures_ is not created by above three paths.
Based on this, remove the support of overlay.

BUG= 782044 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ia334842b12b8297ae4a768979adb8b5a468b906a
Reviewed-on: https://chromium-review.googlesource.com/771130
Commit-Queue: Xing Xu <xing.xu@intel.com>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525903}
[modify] https://crrev.com/9cc56b283e51de94cc373a77214da2bb646efcbd/components/viz/service/BUILD.gn
[modify] https://crrev.com/9cc56b283e51de94cc373a77214da2bb646efcbd/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/9cc56b283e51de94cc373a77214da2bb646efcbd/components/viz/service/display/gl_renderer.h
[add] https://crrev.com/9cc56b283e51de94cc373a77214da2bb646efcbd/components/viz/service/display/scoped_render_pass_texture.cc
[add] https://crrev.com/9cc56b283e51de94cc373a77214da2bb646efcbd/components/viz/service/display/scoped_render_pass_texture.h

Comment 8 by danakj@chromium.org, Jan 23 2018

I think this bug is done?

Comment 9 by xing...@intel.com, Jan 23 2018

Status: Fixed (was: Started)

Sign in to add a comment