823964 - reftest-analyzer screen refresh broken on 2012-12-21 Nightly on Mac hi-dpi (retina) display, or with full-page-zoom on other platforms

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Kew [:jfkthame]]

The latest Nightly update (2012-12-21) has broken the reftest-analyzer image comparison display on a retina MacBookPro display.

STR, using a Retina-display Mac:
- Load the reftest-analyzer link from the URL field.
- Click various test names in succession, to view the captured images.

Note how the display generally updates only the top-left quarter of the view, instead of the entire area.

Disabling hidpi support (by setting gfx.hidpi.enabled=0) will work around the problem.

Bisecting with a local build brings me to changeset 022244d0ca81 (bug 791675) as the first revision that exhibits the bug.

Comment 1

[Robert Longson [:longsonr]]

Jonathan, Daniel, do you know what's going on. I don't have a Mac so it's a bit difficult for me to tell.

Comment 2

[Jonathan Kew [:jfkthame]]

My gut feeling is that there's some connection with invalidation. I.e., when reftest-analyzer changes what's displayed in its main <svg> element, it invalidates an incorrect area (e.g. fails to do a device-pix to css-pix scaling somewhere).

When I load the example URL above and click on a test to load snapshot, it displays fine; but then as I click subsequent tests, only the left-hand side of the image gets updated (from other examples, I believe it's actually just the top-left quarter that is refreshed, but that particular example is all blank at the bottom so it's hard to tell).

Hmm - another symptom: if I load that URL and click the first testcase, it displays fine across the full width of the page. But as soon as I start to scroll the view (my screen isn't tall enough for the whole of it), the right-hand half vanishes. And the "magnifier" at the top-left of the reftest analyzer screen is only active for coordinates in that top-left quarter (regardless of whether the rest of the image is showing or not) - the coordinates just quit updating as soon as I move past the half-way point across or down the image. It's as though the <svg> element is being treated, for most purposes, as being 800x1000 *device* pixels in size instead of 800x1000 *css* pixels.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> Note how the display generally updates only the top-left quarter of the
> view, instead of the entire area.

I can reproduce something like this on Linux (no HiDPI) with these STR:
 1) Open URL. Click the first test-failure. (No need for comparison mode.)
 2) Zoom in (Ctrl+plus, even just one "notch")
 3) Scroll.

The right edges of the blue lines in the first test-snapshot aren't scrolled.  If I zoom in further, more and more of the snapshot isn't scrolled.

Comment 4

[Jonathan Kew [:jfkthame]]

I'm not sure how well I understand this code, but I think this code (from nsSVGImageFrame.cpp) is broken:

485   gfxMatrix scaling;
486   if (applyScaling) {
487     scaling.Scale(scaleFactors.width, scaleFactors.height);
488   } 
489   GeneratePath(&tmpCtx, scaling);
490   gfxRect extent = tmpCtx.GetUserPathExtent();
491   if (applyScaling) {
492     extent.Scale(1 / scaleFactors.width, 1 / scaleFactors.height);
493   }

The trouble is that although it's applying scaling to the context in GeneratePath(), the rect returned by GetUserPathExtent() will still be in *user* space - e.g., in the reftest-analyzer case, it's always (0, 0, 800, 1000), regardless of the scaling. But then it is inverse-scaled, such that it ends up too small if the scaling factor is > 1.0 (i.e., on a hidpi display, as I originally observed, or if you zoom in, as per comment 3).

So the problem will go away if we simply remove the "unscaling" code in lines 491-493. However, I have a couple of concerns about this: (a) I'm not clear what the implications are for the original issue in bug 791675; and (b) if I do this, although reftest-analyzer seems to behave fine, I get a bunch of errors spewed to the terminal:
Dec 21 20:12:14 rMBP.local firefox[63941] <Error>: CGContextSaveGState: invalid context 0x0
Dec 21 20:12:14 rMBP.local firefox[63941] <Error>: CGContextScaleCTM: invalid context 0x0
Dec 21 20:12:14 rMBP.local firefox[63941] <Error>: CGContextTranslateCTM: invalid context 0x0
...etc.

Another "fix" is to save and restore the matrix of tmpCtx around the GeneratePath call, so that the actual path generation happens with the scale applied, and then the original matrix is restored before calling GetUserPathExtent. This results in getting appropriately-scaled extents, so that the final "unscale" call using extent.Scale results in the desired values. And it seems to make reasonable sense, as far as I understand things. But it also generates the CoreGraphics errors mentioned above.

Or another way to look at this might be to claim that GeneratePath(ctx, mat) is broken - it should apply the given matrix while generating the path, but restore it before returning, so that it does not make a persistent change to the context's transform. (If the user actually wants that, they could just apply the matrix explicitly in the first place.) I don't know what other users it has, though, or what the original design intended - this code is all foreign to me.

Whatever the correct solution is here, I assume the same thing will be needed in nsSVGPathGeometryFrame.cpp, where the same code pattern occurs.

Comment 5

[Robert Longson [:longsonr]]

cairo has a limited range in which it works. The scaling is an attempt to make more content work by scaling the content in a way that's more likely to work with cairo and then scaling the result in an inverse direction.

Comment 6

[Robert Longson [:longsonr]]

Perhaps we just need to set an identity transform before getting the extents in the image case e.g. add a line in the middle just to the image case

GeneratePath(&tmpCtx, scaling);
tmpCtx.SetMatrix(gfxMatrix());
gfxRect extent = tmpCtx.GetUserPathExtent();

Does that make things work for you?

Comment 7

[Robert Longson [:longsonr]]

Attachment: patch?

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 694993 [details] [diff] [review]
patch?

This "works" in the same sense as the solutions suggested in comment 4 - i.e., the reftest-analyzer display behaves correctly - but it also suffers from the problem that it generates "invalid context 0x0" errors from CGContext transform functions. Something's not happy internally in the gfxContext...

Giving this feedback- because I think we need to avoid spamming the os x console with errors, even if the actual rendering doesn't seem to be affected, AFAICT.

Comment 9

[Robert Longson [:longsonr]]

No idea what to do about the context, I don't have a Mac and this works on Windows.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: alternative patch

Here's an approach that seems to work without generating CGContext errors - instead of trying to save/restore the context's matrix, which leads to those problems, use gfxContext::Save/Restore around the GeneratePath call.

(Though I'm still a bit curious -why- attempting to save/restore - or even just reset - the matrix produces those errors. But I haven't tried to track it down.)

Are you confident we -don't- need to do something similar in the nsSVGPathGeometryFrame case? I haven't attempted to create a testcase that exercises it, but the code pattern looks so similar that I assumed it probably suffered from the same problem.

Comment 11

[Daniel Holbert [:dholbert]]

Each of the attached patches (on its own) fixes the symptoms I saw in Comment 3 (on linux), FWIW.

Comment 12

[Daniel Holbert [:dholbert]]

I think jfkthame's patch probably makes more sense (and it's encouraging that it doesn't produce that odd-looking "invalid context" runtime output)

I wonder if we should stick this Save/Restore code _inside of_ GeneratePath(), so that other callers will benefit from it, too...?  (e.g. the analogous nsSVGPathGeometryFrame::ReflowSVG code)  It'd actually be trivial to do this by just declaring a gfxContextAutoSaveRestore at the start of GeneratePath.

Comment 13

[Daniel Holbert [:dholbert]]

Actually, never mind about moving the logic to GeneratePath -- it looks like there are other callers (some of nsSVGPathGeometryFrame::Render()'s code-paths, at least) that do their own Save() / Restore() calls, with a somewhat-delayed Restore() call.  So that suggestion would probably break those -- so, scratch that.

Also: incidentally, we already do something along the lines of the attached patches (call IdentityMatrix() between GeneratePath and GetUserPathExtent), in nsSVGPathGeometryFrame::GetBBoxContribution():
> 383   nsRefPtr<gfxContext> tmpCtx =
> 384     new gfxContext(gfxPlatform::GetPlatform()->ScreenReferenceSurface());
> 385 
> 386   GeneratePath(tmpCtx, aToBBoxUserspace);
> 387   tmpCtx->IdentityMatrix();
[...] /* comments */
> 401   gfxRect pathExtents = tmpCtx->GetUserPathExtent();
https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGPathGeometryFrame.cpp#38

That lends supports to the assertion that the attached patches are sane. Hooray!

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 694993 [details] [diff] [review]
patch?

Canceling review, because
(a) I think I prefer the Save/Restore route, per beginning of comment 12, if only because the console-spam is a bit unsettling (though I'm open to being convinced otherwise)
(b) we should probably investigate whether nsSVGPathGeometryFrame's analogous logic needs fixing before landing a final patch here
(c) we should add regression tests

Comment 15

[Robert Longson [:longsonr]]

We already have tests which cover nsSVGPathGeometryFrame.

Comment 16

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa2400559e0

The bounds test fails without this patch applied, the bbox test is just for completeness.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fa2400559e0

Comment 18

[Robert Longson [:longsonr]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a54a353eab8a

See bug 791675 for approval of roll-up patch which includes this.