1151016 - Investigate possible pixel conversion bug in ComputeSnappedImageDrawingParameters

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Follow-up from bug 1149222 comment 30

Here's the suggested fix that we decided not to land in bug 1149222:
https://bugzilla.mozilla.org/attachment.cgi?id=8585603&action=diff

Comment 1

[Mats Palmgren (inactive)]

Note that the patch linked above did pass tests on Try, so it appears
we don't have any tests for this branch of the code.

Comment 2

[Daniel Holbert [:dholbert]]

I'm curious why we don't just set svgViewportSize directly to intImageSize.

It's not clear to me why (in current mozilla-central) the SVG image's viewport size should depend on the current gfxContext matrix or the dev pixel conversion factor.

Ideally, I'd imagine it really *should* just depend on the size of the destination rect in the host document, in CSS pixels.  (Or, if we're clipping the image, the size [in CSS pixels] of the destination rect that the full SVG document *would* be given in the host document.)

(In reply to Mats Palmgren (:mats) from comment #1)
> Note that the patch linked above did pass tests on Try, so it appears
> we don't have any tests for this branch of the code.

Yeah, per bug 1149222 comment 30, it looks like the "svgViewportSize" that we compute here may only be used when we're called via nsTreeBodyFrame.cpp's call to DrawSingleUnscaledImage, for the twisty and the checkbox.

Maybe that means this svgViewportSize that we compute is sort of theoretical, for a scenario that we aren't actually in most of the time, which is why it looks wrong? If so, we should document that scenario (and its rareness) close to where we compute this svgViewportSize.

needinfo=seth for thoughts here, since I think he wrote this code & can maybe shine some light on this. :)

Comment 3

[u459114]

Attachment: wrong viewport

Comment 4

[u459114]

Attachment: viewport_bug1151016.html

Comment 5

[u459114]

While fixing Bug 619500, I notice this change (https://bugzilla.mozilla.org/attachment.cgi?id=8585603&action=diff) cause border-svg-image drawing incorrectly.

Attached correct/ incorrect rendering result.
Using chrome to access viewport_bug1151016.html to get correct result

Comment 6

[Ethan Lin[:ethlin]]

I encountered this problem in the test[1] when I tried to enable webrender color layer. Without webrender, gecko also can reproduce the problem with the pref "layers.force-active;false" on. Looks like the patches in bug 1106602 added a check to see if the current gfxContext matrix is identity. I'm not sure why the gfxContext matrix could change the viewport size since we may set different gfxContext matrix in different rendering paths.
:dholbert, do you have any idea for this problem?

[1] layout/reftests/border-image/svg-as-border-image-2.html

Comment 7

[Daniel Holbert [:dholbert]]

Can you provide a DXR/searchfox link to the gfxContext identity check you're wondering about?

Comment 8

[Daniel Holbert [:dholbert]]

(I'm not sure I understand comment 6 entirely.  But if you're asking why a gfxContext's matrix might depend on layerization: perhaps we have some scaling that gets applied *inside* of the layer if we're layerizing, but gets applied more directly as part of the gfxContext's transform if we're not layerizing?

In any case, it might be a bug that we're depending on the gfxContext matrix here - I probably couldn't say for sure without poking around for a while in gdb to see how/why we end up with these different values & how/why that ends up producing the observed behavior.  I won't dive in at this point, because it sounds like you've already started that gdb'ing, and I'm also recovering from a large backlog in a brief time between two chunks of PTO.)

Comment 9

[Ethan Lin[:ethlin]]

(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #7)
> Can you provide a DXR/searchfox link to the gfxContext identity check you're
> wondering about?

Sure, https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6402.

Comment 10

[Daniel Holbert [:dholbert]]

Sorry, looks like I never followed up on comment 7 - comment 9. Looks like we ended up OK, though, given that we've got WebRender as our only rendering backend now, and the test referenced in comment 6 is passing everywhere too.

I'm not sure whether the issue described in comment 0 is still a problem, but given that it was referencing bug 1149222 comment 30 where I had noted "So the bug here may only be possible to expose in XUL", it's probably not a high priority.

--> closing this bug as incomplete since we didn't end up with a testcase to demonstrate any actual defect here (and to the extent that a defect remains, it's not especially noteworthy if it's XUL-specific and hasn't been causing any trouble up to this point).