Closed Bug 1526756 Opened 9 months ago Closed 8 months ago

Twitter video overlay is blurry in page content with WebRender enabled

Categories

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

67 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 --- unaffected
firefox67 --- verified
firefox68 --- verified

People

(Reporter: ke5trel, Assigned: aosmond)

References

(Blocks 2 open bugs, )

Details

(Keywords: regression, testcase)

Attachments

(6 files, 2 obsolete files)

Twitter's video overlay is blurry when video is displayed in the page content. Fullscreen video is unaffected.

Example: https://twitter.com/firefox/status/1092835112331309056

OS: Ubuntu 18.10

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fdd03ab546fa01aa3705192b2445d7311d7ed997&tochange=e32dd08ec5db619b4c66dc1504a97e0abfc27a84

Regressed by Bug 1453935.

Assignee: nobody → aosmond
Priority: -- → P4
Priority: P4 → P3

I see this too. There's a translateZ(0px) transform, a scale(1) transform and a rounded clip in the ancestors of the blurry element. I haven't been able to reproduce this effect in a reduced testcase.

Duplicate of this bug: 1528418
Attached file A somewhat reduced test case (obsolete) —

Toggling the 'transform' on the video element makes the text sharp. This strikes me as surprising.

Attached file More reduced test case (obsolete) —
Attachment #9044389 - Attachment is obsolete: true
Attached file More reduced test case
Attachment #9044414 - Attachment is obsolete: true
Keywords: testcase

Cut away even more, based on attachment 9044436 [details]; thanks Jeff.

When it uses an intermediate surface, the snapping isn't working as expected. Using the visible rect causes us to have 0 snap offsets, but it seems like we actually needed to snap to the unclipped primitive -- the actual clipping happens later for the rounded border. Let's see how many tests snapping to the primitive rect for blitted intermediate surfaces affects:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af49c1057f4aa630a76c1165f7aac72580ebdf7

Another example in the wild with the same regressor is the main reddit logo on https://new.reddit.com. Reduced test case requires an SVG in a flex element with center alignment and a top/bottom border.

What this problem ultimately boils down to is thus:

  1. Primitives have their own local rects and clip rects. They snap to the visible rect which is the intersection of its local rect and its clip rect.

  2. Pictures have a local rect which is composed of the union of local rect of its children. A picture may apply its own more restrictive clip, which was not applied to the children. This means its visible rect does not match the union of the children.

Snapping to the visible rect is how we ensure that primitives are aligned relative to each other. Snapping to just the primitive's local rect is insufficient since its origin can be considered more or less arbitrary compared to what appears on the screen.

On to the speculation....

My belief is what we are missing then is an appropriate clip rect for snapping purposes. This should be some aggregate of the children's local clip rect, just as we do the calculation for the picture's clip rect. We can continue using the picture's clip rect for actual clipping, but for snapping we should use the aggregate clip rect.

This also provides some basis as to why my original patch in comment 7 "fixed" the problem. This is because in this case, the ideal aggregate clip rect for snapping is actually the same as the picture's local rect.

To expand further:

The aggregate clip rect to snap to should actually be the union of the set of primitive local rect's intersected with their local clip rect (e.g. the union of the visible rects).

The current picture local clip rect is calculated as the union of the bounding rects of the clusters within the picture:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/gfx/wr/webrender/src/picture.rs#2790-2792,2799-2811,2828

The bounding rect of a cluster is calculated here:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/gfx/wr/webrender/src/picture.rs#2132-2140

Which is, in fact, the visible rect of the primitive. Thus we should consider snapping to the primitive's local rect for a picture without intersecting with its clip rect. This is very close to the behaviour from the patch in comment 7, much more so than I had thought (or even intentioned ;)).

A picture can have its own unique clip which was not considered by its
children when calculating their visible rects. As a result, if a picture
clips its primitive rect for snapping purposes, it may produce a
different snapping offset than expected. We should instead just snap to
the primitive rect itself for the picture, since it is the union of the
visible rects that we snapped to for the children.

Blocks: wr-snap
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00381acd841c
Snap brushes for pictures to the primitive instead of visible rect. r=kvark
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
See Also: → 1533292
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

It looks like this has regressed again, both on Twitter and the attached testcases. This is with WebRender on latest Nightly on Linux.

(In reply to Paul Stone from comment #16)

It looks like this has regressed again, both on Twitter and the attached testcases. This is with WebRender on latest Nightly on Linux.

Yes there is a new bug open for this, bug 1540200.

Regressions: 1540200
No longer regressions: 1540200
See Also: → 1540200

I successfully reproduced the issue on Firefox 67.0a1 2019-02-10 under Ubuntu 18.04 (x64) with WebRender and using the link from Comment 0 and the testcases.

The issue is no longer reproducible on Firefox 67.0b18 and latest Firefox Nightly 68.0a1 (2019-05-08).

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.