Closed Bug 1594128 Opened 11 months ago Closed 10 months ago

Allow a single shader to decode several kinds of brush and segment data from the GPU cache

Categories

(Core :: Graphics: WebRender, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- disabled
firefox73 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(12 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

In order to add a brush shader that can render text images and solid colors, it has to be able to decode the data stored in the GPU cache, which currently depends on shader-specific #defines such as VECS_PER_SEGMENT and VECS_PER_SPECIFIC_BRUSH.

This chanes the shader parsing code to only inject #included shader sources once (the first time) if they are included multiple times.
This will allow some extra flexibility needed by the multi-brush shader.

This will allow the upcoming super-brush shader to select its behavior at runtime.

Depends on D53651

This is an experiment with only image and solid to see what the infrastructure can be like.
If it works out I'll extend the it with more brush types. More work will be needed to get text rendering in there as well.

The multi-brush shader includes all brushes that it potentially needs suport for. Which brushes actually get compiled in is then specified via WR_FEATURE defines.
Since brushes can't have the same names for their entry points, they specify the function to use via a macros (WR_BRUSH_VS_FUNCTION and WR_BRUSH_FS_FUNCTION).

TODO: VECS_PER_SPECIFIC_BRUSH needs to either be the same or be dynamically computed as well. That's not in the patch yet.

Depends on D53724

The 'multi-brush' shader will have to dynamically switch between different brushes. In order to support that without needing the sum of all brush varying locations, allow aliasing a number of generic slots.
This patch makes the assumption that one a vec2 and a vec4 cost the same amount of varying register space, which is suggested by the glsl specification about shader locations. If it is not the case we can add more granularity to the varying slots which are all vec4 at the moment. This also assumes that an unused varying is always optimized out.

Depends on D53725

Attachment #9109805 - Attachment description: Bug 1594128 - [WIP] dynamically choose the brush kind in the shader. r=gw → Bug 1594128 - Dynamically choose the brush kind in the shader. r=gw

Depends on D53996

In GLES the default precision for ints is only 16 bits in the fragment shader.

Depends on D53998

Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e894fff26e42
Only include shader sources once during parsing. r=gw
https://hg.mozilla.org/integration/autoland/rev/74554530a510
Encode the brush shader type in the instance attributes. r=gw
https://hg.mozilla.org/integration/autoland/rev/912718966a67
Dynamically choose the brush kind in the shader. r=gw
https://hg.mozilla.org/integration/autoland/rev/bb21ea50b1e9
Use generic slots for text, image and solid shader varyings. r=gw
https://hg.mozilla.org/integration/autoland/rev/0af881b4707a
Generic varyings for ps_text_run.glsl. r=gw
https://hg.mozilla.org/integration/autoland/rev/4077b95429c0
Add brush_blend to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/a2717dc2c87b
Add brush_mix_blend to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/a215df75f72f
Add brush_linear_gradient to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/6b339f631b3d
Add brush_radial_gradient to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/f83f356ca522
Tiddy up the multi-brush code. r=gw
https://hg.mozilla.org/integration/autoland/rev/7a29d7acf94c
Don't rely on high bits to store the brush_kind varying. r=gw
Regressions: 1598837
Regressions: 1598819
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla72 → ---
Flags: needinfo?(nical.bugzilla)

self ni to look at OSX failures

Flags: needinfo?(dmalyshau)

Found the root of confusion to be here:

int vecs_per_brush(int brush_kind) {
  return VECS_PER_SPECIFIC_BRUSH;
}

The driver (or GLSL compiler in it) gets confused by this (seemingly useless) function and produces some garbage. Hard-coding VECS_PER_SPECIFIC_BRUSH instead can be used to work around the issue. More specifically:

#ifndef WR_FEATURE_MULTI_BRUSH
#define MY_SUPER_VARIABLE VECS_PER_SPECIFIC_BRUSH
#endif

I added the corresponding entry to our driver issues.

Flags: needinfo?(dmalyshau)
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6762aab18459
Only include shader sources once during parsing. r=gw
https://hg.mozilla.org/integration/autoland/rev/6386aa316da9
Encode the brush shader type in the instance attributes. r=gw
https://hg.mozilla.org/integration/autoland/rev/550591122788
Dynamically choose the brush kind in the shader. r=gw
https://hg.mozilla.org/integration/autoland/rev/e5f6739f790d
Use generic slots for text, image and solid shader varyings. r=gw
https://hg.mozilla.org/integration/autoland/rev/ba4668d01dc5
Generic varyings for ps_text_run.glsl. r=gw
https://hg.mozilla.org/integration/autoland/rev/0d89ea8665df
Add brush_blend to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/5768997fb391
Add brush_mix_blend to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/54efcc9976cf
Add brush_linear_gradient to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/84e767a37637
Add brush_radial_gradient to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/beeeb1889588
Tiddy up the multi-brush code. r=gw
https://hg.mozilla.org/integration/autoland/rev/a8ee6dbf146b
Don't rely on high bits to store the brush_kind varying. r=gw
https://hg.mozilla.org/integration/autoland/rev/39f51911efa8
Work around Mac GLSL compiler bug. r=kvark

I can reproduce the original issue again on the 11/26 Nightly, which appears to include the workaround.

Backout by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/440eac58d324
Backed out 12 changesets because of rendering regressions a=backout

Nicolas please take a look.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla72 → ---

Can we please avoid re-landing this until next week when nightly 73 starts? Thanks.

Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1138b7e56a3
Only include shader sources once during parsing. r=gw
https://hg.mozilla.org/integration/autoland/rev/9e1443b6c17c
Encode the brush shader type in the instance attributes. r=gw
https://hg.mozilla.org/integration/autoland/rev/e55fa1d9bb89
Dynamically choose the brush kind in the shader. r=gw
https://hg.mozilla.org/integration/autoland/rev/e5dad5475df4
Use generic slots for text, image and solid shader varyings. r=gw
https://hg.mozilla.org/integration/autoland/rev/d4103eb1e6c5
Generic varyings for ps_text_run.glsl. r=gw
https://hg.mozilla.org/integration/autoland/rev/dc267111d174
Add brush_blend to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/7a8c9a267876
Add brush_mix_blend to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/2a87fc64de2f
Add brush_linear_gradient to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/ee4855adae88
Add brush_radial_gradient to the multi-brush infrastructure. r=gw
https://hg.mozilla.org/integration/autoland/rev/c54980f119ab
Tiddy up the multi-brush code. r=gw
https://hg.mozilla.org/integration/autoland/rev/576d4b85b28b
Don't rely on high bits to store the brush_kind varying. r=gw
https://hg.mozilla.org/integration/autoland/rev/1bcbb2e808a7
Work around Mac GLSL compiler bug. r=kvark

Can we please avoid re-landing this until next week when nightly 73 starts? Thanks.

My apologies, I missed this message. Feel free to back the patches out if you want.

Flags: needinfo?(nical.bugzilla)

Keep in mind that this bug caused some performance alerts:
Commit:

== Change summary for alert #24165 (as of Thu, 28 Nov 2019 10:00:45 GMT) ==

Regressions:

37% raptor-tp6-yahoo-mail-firefox-cold loadtime windows10-64-shippable-qr opt 1,009.42 -> 1,381.50
31% raptor-tp6-yahoo-mail-firefox-cold loadtime windows10-64-shippable-qr opt 1,058.33 -> 1,382.42
9% raptor-tp6-yahoo-mail-firefox-cold windows10-64-shippable-qr opt 535.48 -> 582.98
6% raptor-tp6-tumblr-firefox-cold loadtime windows10-64-shippable-qr opt 1,628.67 -> 1,718.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24165

Backout:
== Change summary for alert #24167 (as of Thu, 28 Nov 2019 10:26:45 GMT) ==

Improvements:

28% raptor-tp6-yahoo-mail-firefox-cold loadtime windows10-64-shippable-qr opt 1,381.42 -> 995.92
28% raptor-tp6-yahoo-mail-firefox-cold loadtime windows10-64-shippable-qr opt 1,380.21 -> 996.67
8% raptor-tp6-yahoo-mail-firefox-cold windows10-64-shippable-qr opt 582.42 -> 533.90
4% raptor-tp6-wikipedia-firefox-cold windows7-32-shippable opt 1,459.28 -> 1,407.80

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24167

Regressions: 1614212
No longer regressions: 1614212
You need to log in before you can comment on or make changes to this bug.