Unnecessary render target switches when rendering picture cache tiles
Categories
(Core :: Graphics: WebRender, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox155 | --- | fixed |
People
(Reporter: jnicol, Assigned: jnicol, NeedInfo)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxpe])
Attachments
(1 file)
See this profile using Metal ANGLE: https://share.firefox.dev/3RIWMVF. This is scrolling up and down on a simple page, focused on Renderer::draw_frame().
There are quite a lot of calls to ANGLE's endEncoding(), which calls in to the metal driver.
The most common occurence of these fall within the glClear() call within draw_render_target(). I think these are expected and unavoidable: the glClear() is the first call that writes to a new render target, so we must end the encoding for the previous render target.
The next biggest chunk is within glInvalidateFramebuffer() at the end of draw_picture_cache_target(). Again, expected. Here we are invalidating the depth attachment at the end of the render pass. If we omit that, these samples will simply move somewhere else, and we'll additionally have to pay the cost of resolving the depth attachment contents to main memory.
The ones that stand out to me are within the glClear() call at the beginning of draw_picture_cache_target(). We should have already ended encoding for the previous picture cache target in glInvalidateFramebuffer(), so what encoding is there to end that has occurred since then?
The culprit is here. Before even calling draw_picture_cache_target() we call compositor.bind() to bind the compositor surface as the target. The C++ side then blits the dirty region from the previous frame's surface to the current frame's surface, so that webrender only has to render the current frame's dirty region.
That shouldn't matter, as ANGLE should be able to handle performing a clear after performing a blit to the same render target. The issue is that we use GLBlitHelper::BlitFramebufferToFramebuffer(), which binds/unbinds the FBO before and after blitting. So the sequence of calls looks like this:
- FBO X is bound
- compositor.bind()
a) glBindFramebuffer(Y) // bind picture cache tile's framebuffer
b) glBlitFramebuffer() // blit from previous frame's surface to current frame's surface
c) glBindFramebuffer(X) // restore previously bound framebuffer - draw_picture_cache_target()
a) glBindFramebuffer(Y) // bind picture cache tile's framebuffer
b) glClear() // ends encoding containing previous blit, starts new encoding
ANGLE isn't smart enough to realise that 2b and 3b are drawing to the same framebuffer. It just knows that the draw framebuffer was changed in between those calls, so step 3b ends the previous encoding and starts a new one.
Here's a profile with avoiding changing the framebuffer binding between the blit and the clear: https://share.firefox.dev/4cmhuBB
endEncoding() within draw_picture_cache_target()'s glClear() has fallen from ~50 samples to 19. I think the remaining samples must be cases where we did not perform any blits prior to clearing, and where this is the first picture cache tile being rendered, and therefore there was not already a glInvalidateFramebuffer to end the previous pass' encoding.
|
||
Updated•1 month ago
|
|
Assignee | |
Updated•1 month ago
|
|
|
Assignee | |
Comment 1•1 month ago
|
||
When webrender binds a native picture cache tile, the compositor will
in some cases blit regions from the tile surface used for the previous
frame to the tile surface being used for the current frame. This
ensures webrender only needs to render the current frame's dirty
region to the tile.
Both NativeLayerCA and NativeLayerWayland do so by calling
GLBlitHelper::BlitFramebufferToFramebuffer(). This binds the required
draw and read framebuffers, and then restores the previous bindings
after performing the blit. draw_picture_cache_target() will then
immediately bind the tile surface's FBO again, before rendering the
tile.
On Metal ANGLE, we observe that the first draw performed in
draw_picture_cache_target() (usually glClear()) causes ANGLE to end
encoding the previous command buffer (containing the blits) and start
encoding a new one. This is because the draw framebuffer was changed
in between the blits and the clear, even though it was immediately
changed back again.
We prevent this by manually binding the required draw and read
framebuffers when performing the partial update blits, and calling
GLBlitHelper::BlitFramebuffer() which does not change the
framebuffer bindings. This ensures the same draw framebuffer remains
bound throughout, allowing ANGLE to use a single command buffer per
tile.
Backed out for causing build bustages in NativeLayerWayland.cpp.
Backout link
Push with failures
Failure log(s)
|
||
Comment 6•1 month ago
|
||
| bugherder | ||
|
||
Updated•15 days ago
|
Description
•