464811 - Fix anchor point snapping to guarantee that the source rect intersects the subimage region

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The current approach to anchor point snapping, which I did in bug 463204, isn't quite right. Sometimes the transform from the logical fill rect to the device pixel fill rect can be weird; for example [0.4, 0.6] snaps to [0,1], which is a scale-up by 5, but we don't want to scale an anchor offset of 10 by a factor of 5.

Also, I've been investigating how to guarantee that nsLayoutUtils::DrawImage will generate parameters to nsThebesImage::Draw so that the fill rectangle, transformed to image space, intersects the subimage rectangle.

It turns out a new approach to anchor point snapping solves both of these problems. The approach is:
1) start with the anchor point in user space
2) map it back to image space via the "ideal" transformation defined by the initial rectangle, and snap it to some image pixel corner. Any one will do, the nearest one makes most sense.
3) map that corner forward through the "ideal" transformation back to device space, and snap to the nearest device pixel corner.
4) the device-space-to-image-space transformation's translation is chosen to map the device pixel corner from step 3 to the image pixel corner from step 2.

I've updated
https://wiki.mozilla.org/Gecko:Image_Snapping_and_Rendering
with the adjusted requirements and algorithm. I've also added a proof there that the resulting algorithm ensures the source and subimage rectangles intersect. The proof is not trivial and it's fairly "tight" --- alternative choices in the algorithm would likely break the proof and fail to satisfy the requirement.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

This testcase shows the actual bug. We try to display a sliver of the image and display completely the wrong slice. The column of pixels in the second DIV should match the column of pixels above it in the first DIV.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it, as described here and in the wiki page.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v1.1

With the tests included.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v1.2

433640-1-ref.html needs to be reverted back to its pre-bug-463204 state.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

dbaron thought this should block beta in light of bug 464606.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 348132 [details] [diff] [review]
fix v1.2

I don't quite follow the whole thing, but what I understand makes sense.  r+sr=dbaron

But could you add some comments before MapToFloatImagePixels / MapToFloatUserPixels saying what they do?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v1.3

Updated for comments

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 348482 [details] [diff] [review]
fix v1.3

Another image rendering regression fix

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm, we agreed this was blocking in our triage spreadsheet yesterday; I'm not sure why roc didn't mark this one.

Beltzner says we (as the layout triage team) have the authority to mark things as beta2 blockers if we think they're really blockers (although the non-blocker approval queue is still triaged by the release drivers).  So I'm marking this as such; although I'm not sure whether roc's comment 6 counts as agreement with my assertion that it should block.  If not, he can change the target milestone back.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's cool. I'll land

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 0ced86fbf8ed

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out. Reftests failed, which is weird. Maybe some interaction between this patch and other changes that landed in the past few days.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 348482 [details] [diff] [review]
fix v1.3

>diff --git a/layout/reftests/bugs/reftest.list b/layout/reftests/bugs/reftest.list
[snip]
>+== 464811-1.html 464811-1-ref.thml

That needs a s/thml/html  (though that doesn't fix the other reftest failures)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v1.4

Because I am stupid, I lost a "anchorPoint.Round()" while rebasing the patch to trunk. This patch passes reftests.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 348686 [details] [diff] [review]
fix v1.4

And because I am doubly stupid, I attached the wrong patch.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v1.4

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Re-pushed d328fec034c5

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Both of my duplicate bugs, bug 464606 and bug 464603, are verified FIXED in the nightlies.