Correct dest rect used in ComputeImageContainerDrawingParameters

RESOLVED FIXED in Firefox 66

Status

()

P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

(Blocks: 1 bug)

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
We should avoid making situations like bug 1420648 worse with any changes made here.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Priority: -- → P3
See Also: → bug 1420648
Whiteboard: [gfx-noted]
(Assignee)

Updated

a year ago
Priority: P3 → P1
(Assignee)

Comment 2

a year ago
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
(Assignee)

Updated

a year ago
Attachment #8967716 - Flags: review?(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.
(Assignee)

Comment 5

a year ago
(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.
(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.
Oh, heh.
Comment on attachment 8967716 [details] [diff] [review]
0001-Bug-1453747-Use-rounded-dest-rect-in-simplified-imag.patch, v2

Canceling review for now.
Attachment #8967716 - Flags: review?(mstange)
Let's talk about this tomorrow.
Flags: needinfo?(aosmond)
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.
Assignee: aosmond → nobody
Blocks: 1386669
No longer blocks: 1386665
Status: ASSIGNED → NEW
Flags: needinfo?(aosmond)
Priority: P1 → P2
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.
Priority: P2 → P3
Assignee: nobody → aosmond
(Assignee)

Comment 13

3 months ago
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.
(Assignee)

Updated

3 months ago
Attachment #8967716 - Attachment is obsolete: true

Comment 15

2 months ago
Pushed by aosmond@gmail.com:
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

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.