1453747 - Correct dest rect used in ComputeImageContainerDrawingParameters

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

The dest rect we pass to WebRender is rounded, but the dest rect we use in the ComputeImageContainerDrawingParameters is not. This can cause subtle differences between the decode size and what we render at.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

We should avoid making situations like bug 1420648 worse with any changes made here.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1453747-Use-rounded-dest-rect-in-simplified-imag.patch, v1

There seems to have been a regression where Twitter smilies look bad with WebRender again (at least for me). These changes make them look good again.

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

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1453747-Use-rounded-dest-rect-in-simplified-imag.patch, v2

Mostly reduced/eliminated fuzz for a few affected reftests.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57dc506cef36345a30eee317a471d7b006f9a73c

Comment 4

[Markus Stange [:mstange]]

Oh, it sounds like I misunderstood what the patch does.

Snapping needs to happen in LayerPixel space, not in LayoutDevicePixel space. These spaces are different as soon as there is a CSS scale transform, or during pinch-zooming on Android.
So the code that obtains a scale from somewhere needs to stay, and somehow we need to pass this scale to the other places where we want to snap.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Markus Stange [:mstange] from comment #4)
> Oh, it sounds like I misunderstood what the patch does.
> 
> Snapping needs to happen in LayerPixel space, not in LayoutDevicePixel
> space. These spaces are different as soon as there is a CSS scale transform,
> or during pinch-zooming on Android.
> So the code that obtains a scale from somewhere needs to stay, and somehow
> we need to pass this scale to the other places where we want to snap.

But the dest rect passed to WebRender via PushImage is already rounded without applying the scaling, which must happen afterwards? Aside from the nsImageRenderer / background image case, I would have thought it would match more closely to what WR does today than not.

Comment 6

[Markus Stange [:mstange]]

(In reply to Andrew Osmond [:aosmond] from comment #5)
> But the dest rect passed to WebRender via PushImage is already rounded
> without applying the scaling

Oh, indeed! That's the wrong thing to do. In fact, ToRoundedLayoutRect is a function that shouldn't exist. It should be replaced with a method on StackingContextHelper, e.g. StackingContextHelper::ToSnappedLayoutRect, which can take the stacking context's scale into account.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ha. It used on to be on StackingContextHelper until https://hg.mozilla.org/mozilla-central/rev/9ebf785a75d1

Comment 8

[Markus Stange [:mstange]]

Oh, heh.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8967716 [details] [diff] [review]
0001-Bug-1453747-Use-rounded-dest-rect-in-simplified-imag.patch, v2

Canceling review for now.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Let's talk about this tomorrow.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Apparently this discussion happened but it was a while ago. From what Andrew remembers the conclusion was that the rounding should happen inside WR, but it wasn't clear exactly where, and it's outside the scope of code Andrew is working on. So I'm unassigning this from him.

The user-visible impact is that some small images (e.g. Twitter smilies) will appear blurry because they get rendered at e.g. 15px instead of 16px. On large images it should not be noticeable. I think that we can probably live with that on Nightly, so I'm downgrading this to block riding the trains instead of Nightly.

Comment 12

[Markus Stange [:mstange]]

The guiding principles with respect to snapping (and many more things) are:
 1. We should be consistent in how we snap throughout all the code that uses WebRender.
 2. WebRender snapping should be consistent with non-WebRender snapping.

These principles are not opposed to each other, but principle 1 is more important than principle 2. The current patch in this bug achieves principle 1 but gets us a bit farther away from principle 2.
I'm willing to r+ the patch in this state, once it is updated to apply on current mozilla-central.

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1453747 - Use rounded dest rect in simplified image decode size calculations for WebRender.

We are using the unrounded dest rect to calculate the image decode size
in ComputeImageContainerDrawingParameters, while passing the rounded
dest rect to WebRender. This mismatch causes images to be decoded to one
size and display at another, cause some visual distortions. Using the
correct rect seems to allow us to remove the extra snapping logic added
to work around this.

At this time, how we snap is different between WebRender and
non-WebRender in general. This patch will likely morph again once we
bring the two models closer together.

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f87448b9e4
Use rounded dest rect in simplified image decode size calculations for WebRender. r=mstange

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/70f87448b9e4