47 bytes, text/x-phabricator-request
|Details | Review|
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.
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
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
Mostly reduced/eliminated fuzz for a few affected reftests. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57dc506cef36345a30eee317a471d7b006f9a73c
Attachment #8967476 - Attachment is obsolete: true
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.
(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.
Ha. It used on to be on StackingContextHelper until https://hg.mozilla.org/mozilla-central/rev/9ebf785a75d1
Comment on attachment 8967716 [details] [diff] [review] 0001-Bug-1453747-Use-rounded-dest-rect-in-simplified-imag.patch, v2 Canceling review for now.
Let's talk about this tomorrow.
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.
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.
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.
Attachment #8967716 - Attachment is obsolete: true
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/70f87448b9e4 Use rounded dest rect in simplified image decode size calculations for WebRender. r=mstange
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.