Fix lifetime handling of WebRenderTextureHost

RESOLVED WONTFIX

Status

()

Core
Graphics: WebRender
RESOLVED WONTFIX
a month ago
a month ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a month ago
Since Bug 1383786 fix, the way of updating display list and usage of wr::ImageKey were changed. Current lifetime handling of WebRenderTextureHost does not fit to them. We need to address the lifetime handling.
(Assignee)

Updated

a month ago
Assignee: nobody → sotaro.ikeda.g
Blocks: 1386665
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
Priority: -- → P3

Updated

a month ago
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
(Assignee)

Updated

a month ago
Blocks: 1407069
(Assignee)

Comment 1

a month ago
Created attachment 8920043 [details] [diff] [review]
patch - Fix lifetime handling of WebRenderTextureHost
(Assignee)

Comment 2

a month ago
Created attachment 8920080 [details] [diff] [review]
patch - Fix lifetime handling of WebRenderTextureHost
Attachment #8920043 - Attachment is obsolete: true
(Assignee)

Comment 3

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03c52b30371c1559dd2f6ea53017b7518d7a0e06
(Assignee)

Comment 4

a month ago
Current AsyncImagePipelineManager holds the TexureHosts based on Epoch. To update epoch, we need to update DisplayList. It is not good from performance point of view.

attachment 8920080 [details] [diff] [review] changes AsyncImagePipelineManager as to hold TextureHosts based on frame count. It fit well to Bug 1383786 change and AsyncImagePipelineManager could be simplified.
(Assignee)

Comment 5

a month ago
With attachment 8920080 [details] [diff] [review], requesting frame count could be get by return value of RenderThread::IncPendingFrameCount(). Completed frame count could be get by RenderThread::GetCompletedFrameCount(). With these frame counts values, AsyncImagePipelineManager could know how long AsyncImagePipelineManager needs to hold TextureHosts.
(Assignee)

Comment 6

a month ago
With Comment 5, only WebRenderImageHost::SetCurrentTextureHost() needs to call AsyncImagePipelineManager::HoldExternalImage().
(Assignee)

Updated

a month ago
Attachment #8920080 - Flags: review?(nical.bugzilla)
I just have one issue with basing the texture lifetimes on frame counts: we are in the process of making changes to webrender were scene building would become asynchronous, so that we can keep scrolling scene N-1 while building scene N. When we get there, display list and resource updates will come with new epochs but scroll/animation frames will not.

This is not to say that using frame counts is bad, but I just want to make sure we keep in mind how the asynchronous frame building would interact with it.
For example imagine there is an update on big (long to build) scene and some scrolling happening at the same time. What can happen is that the render backend receives the display list, schedules the scene to be built on another thread and resumes handling scrolling on the current scene for a few frames until the new scene is ready. In this situation, after calling update_texture on the compositor thread there is no guarantee that the next frame rendered will actually use that texture.
I hope the explanation isn't too confusing.
(Assignee)

Comment 8

a month ago
(In reply to Nicolas Silva [:nical] from comment #7)
> This is not to say that using frame counts is bad, but I just want to make
> sure we keep in mind how the asynchronous frame building would interact with
> it.
> For example imagine there is an update on big (long to build) scene and some
> scrolling happening at the same time. What can happen is that the render
> backend receives the display list, schedules the scene to be built on
> another thread and resumes handling scrolling on the current scene for a few
> frames until the new scene is ready. In this situation, after calling
> update_texture on the compositor thread there is no guarantee that the next
> frame rendered will actually use that texture.

:nical, in this case, epoch appearing to RendererOGL::FlushRenderedEpochs() is also deferred? How can we know when webrender stop to use a specific external image?
Flags: needinfo?(nical.bugzilla)
(Assignee)

Updated

a month ago
Attachment #8920080 - Flags: review?(nical.bugzilla)
Yes, epochs will be deferred along with the display list.

Example:

> set display list or update resources (epoch=1)
> display list ready (epoch=1)
> generate frame (flush rendered epoch 1)
> generate frame (flush rendered epoch 1)
> set display list or update resources (epoch=2)
> generate frame (flush rendered epoch 1)
> generate frame (flush rendered epoch 1)
> display list ready (epoch=2)
> generate frame (flush rendered epoch 2)


The information about whether an external image is in use should be in sync with the rendered epoch.
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 10

a month ago
Thanks, make sense:)
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → WONTFIX

Updated

a month ago
Priority: P1 → --
Whiteboard: [wr-mvp]
You need to log in before you can comment on or make changes to this bug.