942451 - preserveAspectRatio changes on SVG images don't invalidate correctly

Status: VERIFIED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Testcase should update in 3 seconds. Broken sometime between Firefox 12 and 24

Comment 2

[Daniel Holbert [:dholbert]]

If I switch tabs away & back (after 3 seconds), the testcase repaints with the correct (circle) rendering.

Looks like an invalidation bug.

Comment 3

[Robert Longson [:longsonr]]

We're doing a repaint but preserveAspectRatio changes change the image's size so we need a reflow.

Comment 4

[Robert Longson [:longsonr]]

So we started to go wrong in bug 734079 and continued in bug 839865 till things didn't work at all.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #3)
> We're doing a repaint but preserveAspectRatio changes change the image's
> size

It doesn't change the *frame* size, though, right?

We've still got <image id="img" [...] width="600" height="300">.

The image will render different pixels *into* that size, but do we actually need a reflow to realize that?  Shouldn't a repaint be sufficient?

Comment 7

[Daniel Holbert [:dholbert]]

reftest nit: the reftest probably belongs in the "svg/image" directory, I'd imagine (with the other tests for the SVG <image> element's rendering.

You can still use pass.svg as the reference case ("../pass.svg" after the move); plenty of other tests there do that, at least.

Comment 8

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> 
> It doesn't change the *frame* size, though, right?

I think it does.

> 
> We've still got <image id="img" [...] width="600" height="300">.
> 

preserveAspectRatio="none" means it's 600 x 300. if preserveAspectRatio="xMidYMid" then it's 300 x 300 as the underlying .gif is 1 x 1 and we have to preserve that.

Comment 9

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> reftest nit: the reftest probably belongs in the "svg/image" directory, I'd
> imagine (with the other tests for the SVG <image> element's rendering.
> 

Happy to do that.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #8)
> preserveAspectRatio="none" means it's 600 x 300. if
> preserveAspectRatio="xMidYMid" then it's 300 x 300 as the underlying .gif is
> 1 x 1 and we have to preserve that.

I don't think so... I think we *render* the underlying .gif into a 300x300 region, but we center it within our *frame*, which is still 600x300.

To double-check, I tried dumping frames from the layout debugger from a static testcase, with each preserveAspectRatio value (xMidYMid vs none), and in both cases I got the same frame size:
    SVGImage(image)(5)@0x7f94332bdcf8 {6000,6000,36000,18000}

So, the frame has the same (rectangular) size in both cases.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> If I switch tabs away & back (after 3 seconds), the testcase repaints with
> the correct (circle) rendering.

(aside: I can also fix the testcase's rendering by scrolling the image offscreen (even partially) and then back on, in a short browser-window. Definitely seems like an invalidation bug.)

I wonder if the "Don't invalidate (the layers code does that)" comment is just wrong? (i.e. maybe we do actually need to invalidate?)

I wonder if maybe DLBI (or whatever we're expecting to invalidate us) is just failing to detect that we need to invalidate, because the nsDisplayItem that we create is the same size and position. It looks like we might only take preserveAspectRatio into account inside of PaintSVG(), which is abstracted away from the display item sizing / positioning.  (Specifically, PaintSVG calls TransformContextForPainting, which calls GetRasterImageTransform, which calls GetViewBoxTransform and passes in element->mPreserveAspectRatio.)

Comment 12

[Robert Longson [:longsonr]]

Attachment: testcase

Beginning to think you're right as extending the image does work on refresh and it wouldn't if a reflow was needed which hadn't occurred would it?

Comment 13

[Robert Longson [:longsonr]]

Attachment: address review comments

How about this instead then?

Comment 14

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> 
> I don't think so... I think we *render* the underlying .gif into a 300x300
> region, but we center it within our *frame*, which is still 600x300.
> 
> To double-check, I tried dumping frames from the layout debugger from a
> static testcase, with each preserveAspectRatio value (xMidYMid vs none), and
> in both cases I got the same frame size:
>     SVGImage(image)(5)@0x7f94332bdcf8 {6000,6000,36000,18000}
> 
> So, the frame has the same (rectangular) size in both cases.

Excellent investigating there. :-)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8337318 [details] [diff] [review]
address review comments

This makes sense to me, but I'll punt official-review to jwatt, since he knows better than I about how SVG invalidation is supposed to be working these days. (And he added the comment here asserting that "the layers code does that" [the invalidation], so in case something fishy is going on there, he should be aware.)

>+++ b/layout/svg/nsSVGImageFrame.cpp
>     else if (aAttribute == nsGkAtoms::preserveAspectRatio) {
>-      // Don't invalidate (the layers code does that).
>-      SchedulePaint();
>+      InvalidateFrame();
>       return NS_OK;

Note that this is effectively reverting the changes to this file that we made in bug 839865:
http://hg.mozilla.org/mozilla-central/diff/dc6bd7ea66f6/layout/svg/nsSVGImageFrame.cpp
(though it's not a 100% revert -- we were previously using InvalidateBounds instead of InvalidateFrame.)

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8337318 [details] [diff] [review]
address review comments

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

Ah, right, <image> is special because we don't paint its content using display lists/layers.

::: layout/svg/nsSVGImageFrame.cpp
@@ +194,5 @@
>        nsSVGUtils::ScheduleReflowSVG(this);
>        return NS_OK;
>      }
>      else if (aAttribute == nsGkAtoms::preserveAspectRatio) {
> +      InvalidateFrame();

Please add the following comment before this call:

    // We don't paint the content of the image using display lists, therefore
    // we have to invalidate for this children-only transform changes since
    // there is no layer tree to notice that the transform changed and
    // recomposite.

Comment 17

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/512a4943a807

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/512a4943a807

Comment 19

[Robert Longson [:longsonr]]

Comment on attachment 8337318 [details] [diff] [review]
address review comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 839865
User impact if declined: broken images within SVG
Testing completed (on m-c, etc.): landed on m-i and merged to m-c, with a test
Risk to taking this patch (and alternatives if risky): very low - simply invalidates when necessary
String or IDL/UUID changes made by this patch: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b241eaa5d3b8
https://hg.mozilla.org/releases/mozilla-beta/rev/a866d15e2710

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a866d15e2710

Comment 22

[Bogdan Maris, Desktop Test Engineering [OOO until 14th April]]]

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 5.2; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on Windows XP x32, Ubuntu 13.04 x32, Mac OS X 10.9 using Firefox 26 beta 8 (buildID: 20131125215016).

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed on Aurora 27.0a2(build id 20131204004002)

Win 7
Os X 10.8.5
Ubuntu 12.10 x32

Comment 24

[Manuela Muntean [Away]]

Verified as fixed on Aurora 28.0a2 (build ID: 20131212004008) with both the testcase and http://hoffmann.bplaced.net/svgtest/preserveaspectratio03.svg on:

Win 8 64-bit
OS X 10.7.5
Ubuntu 13.04 x64