Closed Bug 942451 Opened 7 years ago Closed 7 years ago

preserveAspectRatio changes on SVG images don't invalidate correctly

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- verified
firefox27 --- verified
firefox28 --- verified
b2g-v1.2 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

No description provided.
Testcase should update in 3 seconds. Broken sometime between Firefox 12 and 24
If I switch tabs away & back (after 3 seconds), the testcase repaints with the correct (circle) rendering.

Looks like an invalidation bug.
Summary: preserveAspectRatio changes on images don't refresh any more → preserveAspectRatio changes on SVG images don't invalidate correctly
We're doing a repaint but preserveAspectRatio changes change the image's size so we need a reflow.
Assignee: nobody → longsonr
So we started to go wrong in bug 734079 and continued in bug 839865 till things didn't work at all.
Attached patch patch (obsolete) — Splinter Review
Attachment #8337282 - Flags: review?(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?
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.
(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.
(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.
(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.
(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.)
Attached image 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?
How about this instead then?
Attachment #8337282 - Attachment is obsolete: true
Attachment #8337282 - Flags: review?(dholbert)
Attachment #8337318 - Flags: review?(dholbert)
(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 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.)
Attachment #8337318 - Flags: review?(jwatt)
Attachment #8337318 - Flags: review?(dholbert)
Attachment #8337318 - Flags: feedback+
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.
Attachment #8337318 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/mozilla-central/rev/512a4943a807
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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
Attachment #8337318 - Flags: approval-mozilla-beta?
Attachment #8337318 - Flags: approval-mozilla-aurora?
Attachment #8337318 - Flags: approval-mozilla-beta?
Attachment #8337318 - Flags: approval-mozilla-beta+
Attachment #8337318 - Flags: approval-mozilla-aurora?
Attachment #8337318 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: needs landing on beta/aurora using hg qimport
Keywords: verifyme
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).
Verified as fixed on Aurora 27.0a2(build id 20131204004002)

Win 7
Os X 10.8.5
Ubuntu 12.10 x32
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.