1676559 - Improve render task graph

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, task)

Comment 1

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 1 - Reduce render task graph usage during batching.

This patch factors out a debug check and reduces the need to pass
the render task graph around to so many parts of the code. If we
want to retain the frame_id check, we can use an atomic that is set
at the beginning of the frame in a follow up patch.

Comment 2

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 2 - Remove ClearMode enum from render tasks.

It's only used by a small subset of render tasks, it makes more
sense to specialize it for those tasks. This is part of reducing
the fields in RenderTask so it's easier to port to the graph
changes being worked on.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af9d8aa720dc
Pt 1 - Reduce render task graph usage during batching. r=nical

Comment 4

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 3 - Tidy up some interfaces.

A couple of minor changes:

• Don't pass render task graph to resource cache (it's not used).
• Support an initial_size for the texture allocator on creation.

This is a convenience for when the allocation tracker is being used
to track a single surface rather than an array.

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/af9d8aa720dc

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d0cf7ff0a20
Pt 2 - Remove ClearMode enum from render tasks. r=nical
https://hg.mozilla.org/integration/autoland/rev/dac282083922
Pt 3 - Tidy up some interfaces. r=nical

Comment 7

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1676559) for WR failures

Log:
https://treeherder.mozilla.org/logviewer?job_id=321852316&repo=autoland&lineNumber=931

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=dac2820839223cdef66487d15ba8fe8d35435986

Backout:
https://hg.mozilla.org/integration/autoland/rev/2a56fe5a8d4b4b51e24f0084545883ba0d312cff

Comment 8

[Glenn Watson [:gw]]

Fixed up a unit test that I missed - I think that covers all call sites now.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e15d1867430e
Pt 2 - Remove ClearMode enum from render tasks. r=nical
https://hg.mozilla.org/integration/autoland/rev/18474be6d389
Pt 3 - Tidy up some interfaces. r=nical

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e15d1867430e
https://hg.mozilla.org/mozilla-central/rev/18474be6d389

Comment 11

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 4 - Remove RenderPassKind

The main framebuffer pass is now always a simplified step of
constructing the CompositeState structure, rather than any
complex alpha batching (since a recent change that meant
picture caching is always enabled).

This patch doesn't contain any functional changes. It removes
the main framebuffer render pass kind, simplifying how passes
are built and rendered. A follow up patch will further simplify
this code by moving the CompositeState creation out of the
regular batching code.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eeb0b5be222f
Pt 4 - Remove RenderPassKind r=nical

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/eeb0b5be222f

Comment 14

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 5 - Refactor `RenderTask` to move creation logic to RenderTaskKind

With follow up patches, we want to retain the existing logic and
structures in RenderTaskKind, but be able to port and use them in
a new render task graph structure. This is simpler if we move as
much logic out of RenderTask as possible.

• Move simple RenderTask create methods into RenderTaskKind
  (complex ones will be done as a separate follow up)
• Move write_task_data and write_gpu_blocks into RenderTaskKind
• Move some static helper methods (e.g. to BlurTask)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46da71790703
Pt 5 - Refactor `RenderTask` to move creation logic to RenderTaskKind r=nical

Comment 16

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 6 - Reduce size of TextureSource / BatchTextures.

These structs are created and copied around many times during
batching. As PrevPassColor and PrevPassAlpha are removed, the
BatchTextures struct will contain extra fields (such as the clip
mask binding), so it's important to reduce the size of them to
avoid regressiing batching time.

Comment 17

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 7 - Remove PrevPassAlpha use for clip masks.

Use a specific texture sampler for clip masks. Although this is
always currently set to the PrevPassAlpha texture source, it does
all the plumbing to allow a follow up commit that explcitly
provides a texture id for clip masks per-primitive.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/46da71790703

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb262e0c687a
Pt 6 - Reduce size of TextureSource / BatchTextures. r=nical
https://hg.mozilla.org/integration/autoland/rev/be323364bca9
Pt 7 - Remove PrevPassAlpha use for clip masks. r=nical

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/cb262e0c687a
https://hg.mozilla.org/mozilla-central/rev/be323364bca9

Comment 21

[Worcester12345]

"PERF" key word?

Comment 22

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 8 - Move render target pool from renderer to frame building.

This is an incremental but important step to implementing render
tasks as a proper graph.

By moving the render target management to the frame building step,
we know the texture_id of all sub-passes before the batching is
done for any passes that use these as inputs. This means that
we can directly reference the texture_id during batch, rather
that the old RenderTaskCache and PrevPassAlpha / PrevPassColor
enum fields (although removal of all these will be done in the
next patch).

Another advantage of this is that we have much better knowledge
of which targets are required for rendering a given frame, so
these can be allocated up front at the start of a frame. This
may be a better allocation pattern for some drivers. We also
have better knowledge available on when a texture can be
invalidated, and the render target pool management is simpler since
it is the same as the way other texture cache textures are handled.

Comment 23

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 9 - Make clip mask sampler depend on explicit cache texture.

This is a follow up to the addition of a clip mask texture sampler.

With this patch, that sampler is no longer bound to the PrevPassAlpha
input, but is explicitly bound to any arbitrary texture that the
input render task was drawn into. This is a step towards enabling
the full render task graph.

There's a bit of complexity here in that it's now possible for individual
segments to break a batch, if they have clip masks that ended up on a
different texture. This should be an extremely rare case. However, it
does currently result in (even more) code duplication in some of the
batching code - which can be refactored once the render task graph
changes are in place.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95c9143cca21
Pt 8 - Move render target pool from renderer to frame building. r=nical,jnicol
https://hg.mozilla.org/integration/autoland/rev/1297d9265f72
Pt 9 - Make clip mask sampler depend on explicit cache texture. r=nical

Comment 25

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 10 - Remove PrevPassAlpha and PrevPassColor

Remove usage of the implicit prev pass alpha and color texture
samplers from batching / renderer / shader code. They are replaced
by explicit references to the texture ID for the source task.

Comment 26

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/95c9143cca21
https://hg.mozilla.org/mozilla-central/rev/1297d9265f72

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/451f658fce05
Pt 10 - Remove PrevPassAlpha and PrevPassColor r=nical

Comment 28

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 11 - Remove texture array usage from render task graph.

With this change, all color/alpha intermediate surfaces are individual
textures, rather than texture arrays.

This can in theory cause more batch breaks in some cases, but this
is likely to be very rare in practice.

Benefits:

• No more allocating the array at the size of the largest task / slice.
• Remove a source of many driver bugs on android devices.
• Simplify integration of future patches with render task graph.

Much of the render target array texture code is still present, since
picture cache tiles in the Draw compositor still make use of texture
arrays. However, once these are switched to normal textures, we can
remove most of the slice layer, blit workaround functionality etc.

Remove the default feature setting for selecting the image sampler
kind. Instead, this must be explicitly specified by the shader or
a dynamic feature define, which makes sampler selection less error prone.

Comment 29

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 12 - Remove array textures from picture cache tiles.

This patch makes picture cache tiles use normal textures instead
of array textures. With this and the previous patch, WR no longer
uses array textures at all (except when provided by the external
image handler trait).

Comment 30

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/451f658fce05

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out parts 8-10 for causing bug 1681244.
https://hg.mozilla.org/mozilla-central/rev/f3ebdd48906c0395b6b3782a0799475e11be7618

Comment 32

[Glenn Watson [:gw]]

Updated part 9 with a fix for the regression. I will attempt to re-land parts 8 and 9 today, and hold off on landing parts 10 - 12 until after the soft freeze period is complete.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ec48e93ed40
Pt 8 - Move render target pool from renderer to frame building. r=nical,jnicol
https://hg.mozilla.org/integration/autoland/rev/a4209b1f9fc5
Pt 9 - Make clip mask sampler depend on explicit cache texture. r=nical

Comment 34

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/0ec48e93ed40
https://hg.mozilla.org/mozilla-central/rev/a4209b1f9fc5

Comment 35

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 13 - Add RenderTaskGraphBuilder / RenderTaskGraph split

This patch splits the graph building functionality into
RenderTaskGraphBuilder and the graph querying code into
the existing RenderTaskGraph struct.

The Builder struct is retained frame to frame, which means
there is no longer a need for the RenderTaskGraphCounters
struct. The Graph struct is constructed per-frame by calling
end_pass on the Builder.

Although this doesn't do much different internally, it will
make integration with the new task graph changes simpler. It
also enforced during frame building when it is possible
to add / query render tasks.

A few unrelated tidy ups are included in this patch - mostly
removing where the task graph is passed to from a few structs
and methods that no longer require access to the graph.

Comment 36

[Glenn Watson [:gw]]

Attachment: Bug 1676559 - Pt 14 - Refactor RenderTaskLocation

Once the new graph API is in place, it becomes possible to express
an input dependency on a persistent target (for example, if wanting
to read back from a picture cache tile for a mix-blend, or marking
that a color target depends on a render task in a texture cache).

To make that simpler to express, this patch adds a specific struct
for render target locations that are persistent, and updates the
surrounding code to use it. At the same time, introduce an Unallocated
field for dynamic tasks that are not yet allocated, rather than
using an Option.

Comment 37

[Mathew Hodson]

(In reply to Glenn Watson [:gw] from comment #32)

Updated part 9 with a fix for the regression. I will attempt to re-land parts 8 and 9 today, and hold off on landing parts 10 - 12 until after the soft freeze period is complete.

If parts 10 and later are going to be part of Firefox 86 then they should probably be landed in a new bug. Otherwise, if there are regressions it will be hard to track which release is affected and the bug tags will be confusing.

Comment 38

[Glenn Watson [:gw]]

(In reply to Mathew Hodson from comment #37)

(In reply to Glenn Watson [:gw] from comment #32)

Updated part 9 with a fix for the regression. I will attempt to re-land parts 8 and 9 today, and hold off on landing parts 10 - 12 until after the soft freeze period is complete.

If parts 10 and later are going to be part of Firefox 86 then they should probably be landed in a new bug. Otherwise, if there are regressions it will be hard to track which release is affected and the bug tags will be confusing.

I've created https://bugzilla.mozilla.org/show_bug.cgi?id=1682365 and will land the remaining patches in that bug. Is it just a case of amending the commits to refer to a new bug number and re-pushing them to phabricator, or is there some other process (while ensuring the existing review comments / approvals remain).

Comment 39

[Phabricator Automation]

Comment on attachment 9191547 [details]
Bug 1676559 - Pt 10 - Remove PrevPassAlpha and PrevPassColor

Revision D98872 was moved to bug 1682365. Setting attachment 9191547 [details] to obsolete.

Comment 40

[Phabricator Automation]

Comment on attachment 9191803 [details]
Bug 1676559 - Pt 11 - Remove texture array usage from render task graph.

Revision D99006 was moved to bug 1682365. Setting attachment 9191803 [details] to obsolete.

Comment 41

[Phabricator Automation]

Comment on attachment 9191819 [details]
Bug 1676559 - Pt 12 - Remove array textures from picture cache tiles.

Revision D99013 was moved to bug 1682365. Setting attachment 9191819 [details] to obsolete.

Comment 42

[Phabricator Automation]

Comment on attachment 9192313 [details]
Bug 1676559 - Pt 13 - Add RenderTaskGraphBuilder / RenderTaskGraph split

Revision D99297 was moved to bug 1682365. Setting attachment 9192313 [details] to obsolete.

Comment 43

[Phabricator Automation]

Comment on attachment 9192322 [details]
Bug 1676559 - Pt 14 - Refactor RenderTaskLocation

Revision D99305 was moved to bug 1682365. Setting attachment 9192322 [details] to obsolete.

Comment 44

[Glenn Watson [:gw]]

Resolving as fixed, since pending patches have been moved to the new bug.

Comment 45

[Mathew Hodson]

(In reply to Glenn Watson [:gw] from comment #38)

I've created https://bugzilla.mozilla.org/show_bug.cgi?id=1682365 and will land the remaining patches in that bug. Is it just a case of amending the commits to refer to a new bug number and re-pushing them to phabricator, or is there some other process (while ensuring the existing review comments / approvals remain).

Yes, it should just need an amend, which should re-title the revision and change the Bugzilla bug ID in Phabricator. Then phab-bot should add new comments to the bug.