Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix for race condition when caching render tasks. #2313
Conversation
When the renderer receives multiple Frames from the backend in a single render loop, it drops the old frames and just draws the most recent frame. If that old frame contained a render task to write to the texture cache, then the texture cache will never be updated. This patches tracks whether a frame has any texture cache render tasks, and whether it has been drawn before. If a frame which meets those conditions is being replaced, then do a render of that frame first, skipping the main framebuffer pass. This is not ideal since we may end up drawing offscreen render tasks that were batched but which aren't strictly necessary for the texture cache render target. However, it is very rare that the render backend is (a) producing frames faster than they are being consumed by the renderer and (b) also contain texture cache tasks.
|
r? @kvark This is a rather inelegant fix - if you have a better idea of how to solve it quickly, let me know! |
|
@jrmuizel @staktrace I don't think it's likely you'll see this in Gecko, but if you are seeing it, and Gecko has updated past #2284 then this may explain any intermittent failures with box shadows. |
| } | ||
|
|
||
| // Glyph | ||
| if !self.font_indices.is_empty() { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -1778,6 +1779,10 @@ impl FrameBuilder { | |||
| &self.clip_store, | |||
| RenderPassIndex(pass_index), | |||
| ); | |||
|
|
|||
| if let RenderPassKind::OffScreen { ref texture_cache, .. } = pass.kind { | |||
This comment has been minimized.
This comment has been minimized.
kvark
Jan 17, 2018
Member
hmm, so any non-main-FBO tasks are considered for has_texture_cache_tasks? If the problem is only about skipping the tasks that are required for future rendering, we need a distinction between re-usable and non-reusable tasks, and only checks for the former here
This comment has been minimized.
This comment has been minimized.
glennw
Jan 17, 2018
Author
Member
It checks for the texture_cache target being non-empty. So this excludes temporary render tasks.
| // (in order to update the texture cache), issue | ||
| // a render just to off-screen targets. | ||
| if self.active_documents[pos].1.frame.must_be_drawn() { | ||
| self.render_impl(None).ok(); |
This comment has been minimized.
This comment has been minimized.
kvark
Jan 17, 2018
Member
technically, we only need to render the pos indexed document, not all of them.
Perhaps, we could accumulate the documents into something like inactive_documents vector that is flushed whenever we happen to render stuff?
This comment has been minimized.
This comment has been minimized.
|
@bors-servo r+ |
|
|
Fix for race condition when caching render tasks. When the renderer receives multiple Frames from the backend in a single render loop, it drops the old frames and just draws the most recent frame. If that old frame contained a render task to write to the texture cache, then the texture cache will never be updated. This patches tracks whether a frame has any texture cache render tasks, and whether it has been drawn before. If a frame which meets those conditions is being replaced, then do a render of that frame first, skipping the main framebuffer pass. This is not ideal since we may end up drawing offscreen render tasks that were batched but which aren't strictly necessary for the texture cache render target. However, it is very rare that the render backend is (a) producing frames faster than they are being consumed by the renderer and (b) also contain texture cache tasks. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2313) <!-- Reviewable:end -->
|
|
glennw commentedJan 17, 2018
•
edited by larsbergstrom
When the renderer receives multiple Frames from the backend
in a single render loop, it drops the old frames and just
draws the most recent frame.
If that old frame contained a render task to write to the
texture cache, then the texture cache will never be updated.
This patches tracks whether a frame has any texture cache
render tasks, and whether it has been drawn before. If a
frame which meets those conditions is being replaced, then
do a render of that frame first, skipping the main framebuffer
pass.
This is not ideal since we may end up drawing offscreen render
tasks that were batched but which aren't strictly necessary
for the texture cache render target. However, it is very rare
that the render backend is (a) producing frames faster than they
are being consumed by the renderer and (b) also contain texture
cache tasks.
This change is