1149222 - Make nsLayoutUtils::DrawBackgroundImage and SVGImageContext use CSSIntSize instead of unit-less nsIntSize.

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1

I've marked a few places in the code with XXX that I think might be bugs.
This patch is idempotent though.  I'm essentially just casting int values
to be of the right type.  I'll try to address the bugs as separate patches.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2, fix bug in ComputeSnappedImageDrawingParameters

This part fixes the bug in ComputeSnappedImageDrawingParameters successfully.

With part 1 only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10560430ba3f

With part 1 + 2:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f033c734b1b0

Comment 3

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

Are these CSS pixels or image pixels?

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3, alt A

Here's my attempt to fix the bug in nsSVGImageFrame::PaintSVG, which
treats 'width'/'height' as both LayoutDevicePixels and CSS pixels
in the same piece of code.

This attempt treats them as LayoutDevicePixels which is then correctly
converted into CSS pixels for the SVGImageContext use, and converts it
to app units for the nsLayoutUtils::DrawSingleImage use.

It fails several reftests in layout/reftests/svg/image/

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3, alt B

Here's a second attempt, now treating 'width'/'height' as CSS pixels instead.
It fails the same reftests.

Comment 6

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #3)
> Are these CSS pixels or image pixels?

They are float results coming from:

  float x, y, width, height;
  SVGImageElement *imgElem = static_cast<SVGImageElement*>(mContent);
  imgElem->GetAnimatedLengthValues(&x, &y, &width, &height, nullptr);

http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGImageFrame.cpp#304

Comment 7

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

Also, when we have an SVG image, image pixels to the outer document are something (what?) for the inner SVG document.

Comment 8

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

(You probably want seth or dholbert to review these changes, I think.)

Comment 9

[Mats Palmgren (inactive)]

dholbert, jwatt, can you explain the use of 'width'/'height' in
nsSVGImageFrame::PaintSVG please?  It seems to use them both as
LayoutDevicePixels and CSS pixels at the same time.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8585599 [details] [diff] [review]
part 1

(cancel the review for now until we sort this out)

Comment 11

[Jonathan Watt [:jwatt]]

'width'/'height' are initially in CSS px. The multiplication by appUnitsPerDevPx looks like a bug to me.

Comment 12

[Daniel Holbert [:dholbert]]

I think the appUnitsPerDevPx multiplication has something to do with staying crisp, even when zoomed in with full-page-zoom.

Suppose you have <image xlink:href="foo.svg" width="100" height="100"> rendered crisply, at a 1x zoom level. If you then do a 2x full-page-zoom[1], we do *not* want to just scale up the 100x100 rendering to have larger pixels -- we want to render at twice the resolution.  I think this is what we're trying to do here.

This might be the wrong way to do it, though... As I recall, VectorImage::Draw should also receive a gfxContext that knows the actual resolution of the target surface, so I'd expect we should be able to account for this there, too.

[1] (which IIRC increases AppUnitsPerDevPixel() but does not increase AppUnitsPerCSSPixel())

Comment 13

[Seth Fowler [:seth] [:s2h]]

Mats, we don't have a unit for image pixels, and I think we need one. Some of these values are in image space.

Comment 14

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> I think the appUnitsPerDevPx multiplication has something to do with staying
> crisp, even when zoomed in with full-page-zoom.

Yes. I'm pretty sure that's the case.  Keep in mind that one of the purposes of this code is to ensure that we draw everything at 1:1 in device pixels (actually layer pixels) if at all possible. We're deliberately modifying other values based upon the size in device pixels that we intend to draw, and given that the code was originally developed without strongly typed units, I think that you have to be careful when assuming that a unit mismatch is a bug. It may just be the lack of a cast.

I confess that I don't remember the details here perfectly, but if you want to change this, I think you should save it for a followup. I'm not convinced that you wouldn't be introducing a bug, rather than fixing one.

> This might be the wrong way to do it, though... As I recall,
> VectorImage::Draw should also receive a gfxContext that knows the actual
> resolution of the target surface, so I'd expect we should be able to account
> for this there, too.

I'd prefer to keep all of this code together, rather than split the logic between here and VectorImage::Draw. I've had bad experiences trying to do it the other way.

That said, if we moved this entire computation into ImageLib we could probably factor it more nicely. I don't think that would be a bad idea.

Comment 15

[Mats Palmgren (inactive)]

OK, I filed bug 1149515 on creating a separate unit type for image pixels.
I'll leave these image related pixels as is for now.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: part 3, alt c

I thought about this a bit more -- per bug 1149515 comment 2, I'm not sure we actually want a different unit type (at least, we don't need it to solve this issue).

We just need to decouple the usages. part 3 "alt A" and "alt B" has them being treated as dev pixels and then *converted* to css pixels, or vice versa - but really we need to treat them as one for one purpose, and as the other for another purpose. Specifically:
 - We treat width & height as CSS pixels *inside the SVG-as-an-image document* (and we signal that with the SVGImageContext) -- this determines what region of the document is visible.
 - We treat width & height as Dev Pixels in the *host* document - this ensures we can actually draw the image crisply, if we're zoomed in or have a HiDPI device. This ensures that a given "width" attribute actually produces a large (and crisp) enough region on-screen.

This patch, "alt C", is a variant of "alt A" that decouples these usages and adds explanatory comments. As noted in an XXX comment, we may want to make SVGImageContext require a "CSSIntSize" input arg, too; I haven't vetted the other SVGImageContext usages enough to be sure if that's appropriate yet, so I'm just leaving that as-is (nsIntSize) with a better comment for now.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: part 3, alt C (v2)

Minor improvements: I updated the patch to remove an unused function that I'd copied over from 'part a', I tweaked the spacing, and I filed bug 1149824 & mentioned it here in my XXX SVGImageContext comment.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Hmmm... I actually thought we were talking about part 2 when I made my comments above. ¯\_(ツ)_/¯

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8586438 [details] [diff] [review]
part 3, alt C (v2)

Heh, I also made a patch that does precisely that but figured I should wait
until bug 1149515 settled.  So this seems reasonable to me :-)

Your XXX comment is what part 1 does, perhaps you could review that?
(it's an idempotent change)

You can ignore my XXX comment in nsSVGImageFrame::PaintSVG, I think
that one is correct.  I'm not sure about part 2 though, if we agree
that 'svgViewportSize' should be CSS pixels then it's not clear to me
what the current code is doing with aAppUnitsPerDevPixel there.

Comment 20

[Mats Palmgren (inactive)]

Also, should the 2nd sentence here be a XXX comment perhaps?
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#5939

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8585599 [details] [diff] [review]
part 1

Review of attachment 8585599 [details] [diff] [review]:
-----------------------------------------------------------------

Could you file a followup on digging into the XXX comments in this commit, to see if the code needs fixing (and if tests need to be added to cover those cases)?   (Probably in imagelib, & CC me and seth)

r=me with that bug filed & with the following:

::: image/src/VectorImage.cpp
@@ +725,5 @@
>      , opacity(aSVGContext ? aSVGContext->GetGlobalOpacity() : 1.0)
> +  {
> +    if (aSVGContext) {
> +      CSSIntSize sz = aSVGContext->GetViewportSize();
> +      viewportSize = nsIntSize(sz.width, sz.height); // XXX losing unit

This should be simplified -- SVGDrawingParameters::viewportSize should just have type "CSSIntSize", too.

(The 'aSize' argument -- which we use to set up viewportSize, if there's no aSVGContext -- is a bit more complex. So aSize should probably stay as-is for now. I'd want to audit its sources a bit more before converting it -- that might be one place where we need a new unit. Seth would have a better handle on that.)

::: layout/svg/nsSVGImageFrame.cpp
@@ +365,5 @@
>  
>      if (mImageContainer->GetType() == imgIContainer::TYPE_VECTOR) {
>        // Package up the attributes of this image element which can override the
>        // attributes of mImageContainer's internal SVG document.
> +      SVGImageContext context(CSSIntSize(width, height), // XXX BUG?

Per my XXX comment in part 3, this is actually correct, so let's drop the "XXX BUG?" comment here. :)

Comment 23

[Daniel Holbert [:dholbert]]

(Ah, I guess 'part 2' addresses one set of XXX comments, so the 'CanvasRenderingContext2D' XXX comment is really the only one that remains. I do think that's probably a bug, but it needs more testing to be sure, & that doesn't need to block the type-conversion step, as you note in comment 1.)

Comment 24

[Daniel Holbert [:dholbert]]

And if we convert SVGDrawingParameters::viewportSize, I suppose we should also convert its downstream consumers -- SVGDocumentWrapper::UpdateViewportBounds() and the SVGDrawingCallback() constructor.

If you'd like to do that conversion in a separate patch & leave this review-feedback in 'part 1' un-addressed [until that separate patch], that's fine with me. 

(In reply to Mats Palmgren (:mats) from comment #20)
> Also, should the 2nd sentence here be a XXX comment perhaps?
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#5939

That's probably a question for Seth -- I don't know that ComputeSnappedImageDrawingParameters code particularly well.  (I don't think it's making any assumption about CSS pixels mapping directly to device pixels, though, which I think is what you're getting at -- though I might be wrong.)

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: part 3, alt C (v3) [r=mats]

Here's an updated version of part 3, with:
 - commit message added
 - my XXX comment removed
 - rebased on top of a tweaked version of part 1 (tweaked to remove the "// XXX BUG?" from this code, per end of comment 22).

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: part 3, alt C (v4) [r=mats]

er, and one more tweak to change patch-author to me, so it doesn't land with author == reviewer. :)

(Though, the code here is mostly yours, modulo my tweaks and clarifying-comment-insertions -- so if you'd prefer, I'm also happy to have this land with author=you reviewer=me. I suspect you don't really care, since it's a pretty small patch anyway -- that's why I just went ahead and posted the tweaked patch in the first place.)

Comment 27

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #24)
> And if we convert SVGDrawingParameters::viewportSize, I suppose we should
> also convert its downstream consumers --
> SVGDocumentWrapper::UpdateViewportBounds() and the SVGDrawingCallback()
> constructor.
> 
> If you'd like to do that conversion in a separate patch & leave this
> review-feedback in 'part 1' un-addressed [until that separate patch], that's
> fine with me. 

Yeah, let's do that separately.  In this series of patches I'm mostly
addressing Layout code.

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 8585603 [details] [diff] [review]
part 2, fix bug in ComputeSnappedImageDrawingParameters

It seems you agree with this change, but let's do the review formally.

I'll file a follow-up bug on creating a test for this branch.
Apparently we don't have one since this was green on Try.

Comment 29

[Mats Palmgren (inactive)]

Part 2 is optional though.  I can file a follow-up bug instead if you guys
want to investigate and fix it separately.

Comment 30

[Daniel Holbert [:dholbert]]

I think part 2 might be technically incorrect (and the current code might be technically buggy, in that it should just drop the ternary statement).

It looks like the svgViewportSize that we (incorrectly?) compute here is only used if we're being called via DrawSingleUnscaledImage, which is only called from XUL "nsTreeBodyFrame.cpp". (There's also a call in nsSVGImageFrame.cpp, but only for raster images). So the bug here may only be possible to expose in XUL.

Anyway -- yeah, let's spin that off to another bug for further investigation.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8585603 [details] [diff] [review]
part 2, fix bug in ComputeSnappedImageDrawingParameters

Canceling review on part 2, per previous comment.

(I'd probably prefer to have Seth review changes there, since IIRC he wrote the ComputeSnappedImageDrawingParameters code, but it probably makes sense to consider it in a followup regardless of who reviews it.)

Comment 32

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> Anyway -- yeah, let's spin that off to another bug for further investigation.

Filed bug 1151016.

Comment 33

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6cb2322a9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/952d78b923c7

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fc6cb2322a9e
https://hg.mozilla.org/mozilla-central/rev/952d78b923c7