Closed Bug 1594128 Opened 5 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- disabled
firefox73 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

(Keywords: perf-alert)

Attachments

(12 files)

Bug 1594128 - Only include shader sources once during parsing. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Encode the brush shader type in the instance attributes. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Dynamically choose the brush kind in the shader. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Use generic slots for text, image and solid shader varyings. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Generic varyings for ps_text_run.glsl. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Add brush_blend to the multi-brush infrastructure. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Add brush_mix_blend to the multi-brush infrastructure. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Add brush_linear_gradient to the multi-brush infrastructure. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Add brush_radial_gradient to the multi-brush infrastructure. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Tiddy up the multi-brush code. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Don't rely on high bits to store the brush_kind varying. r=gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594128 - Work around Mac GLSL compiler bug. r=kvark
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
status-firefox72: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla72 → ---

Backed out 11 changesets (bug 1594128) for several regressions

https://hg.mozilla.org/mozilla-central/rev/0a724aa35cdbb7ceef51f7af26a6aa7b5058682f

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
status-firefox72: fixed → ---
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: 1600256
Regressions: 1614212
No longer regressions: 1614212
Keywords: perf-alert
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: