379430 - Print preview hangs X

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: 2007-02-14-01 and later trunk builds

STEPS TO REPRODUCE:
0) Make sure you have a way to interact with your machine once X hangs
1) Start browser
2) Load data:text/html,<div>aaa<div style="height: 2000px"></div>bbb</div>
3) File > Print Preview

EXPECTED RESULTS: Print preview

ACTUAL RESULTS: X starts using 100% CPU and does not respond to Ctrl+Alt+Backspace.  Killing the browser quickly (within 30 seconds of the hang's start) means that X will probably come back to life within 10 minutes or so.  If it takes you longer to kill the browser, X will be in lala-land for longer.

NOTES:

This is not a problem with 2007-02-13-01 builds.

I've tried profiling this using jprof and sysprof, including using -sync.  All I can tell you is that X itself is hogging the CPU, and that from the mozilla end of things "_end" under the GTK update stuff called from the event loop is where the time looks like it lives.  Even with -sync.

Regression range in bonsai:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-13+01&maxdate=2007-02-14+01&cvsroot=%2Fcvsroot

My current guess is that this is a regression from bug 369834, based on that range.

Oh, for extra fun this might depend on your X version.  I have the one that comes with FC4: "X.Org version: 6.8.2"

Comment 1

[Boris Zbarsky [:bzbarsky]]

Of course it could be that this is really a cairo/thebes problem and the checkin for bug 369834 just exposed it...

In any case, for the time being print preview is going on the lengthening list of things I just shouldn't try to do.  :(

Comment 2

[Andrew Smith]

I'm not sure if I'm reproducing this or not. Doing a print preview on any page X takes a lot of CPU, then it stops, then if I scroll the preview it starts taking 30-90% of my 1.2GHz CPU.

Also X starts using a lot more memory. Going from 1.3% up to over 50%. The memory is freed once I stop scrolling.

X Window System Version 1.3.0

Could it be that we're seing the same thing except for some reason your system doesn't handle the load as well? Like if you have much less than my 1.3GB than I do it could be that the swapping is what's killing your system.

Comment 3

[Andrew Smith]

Erm.. I think I may have confused X's CPU and memory usage. Cause I can't reproduce what I posted a few minutes ago :(

Comment 4

[Arthur]

Under Linux with current nightlies I only get "An unknown error occurred while printing." when trying the Print Preview functionality.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's bug 394407.

Comment 7

[Stuart Parmenter]

eli this looks like you?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Back to Thebes.  The checkin for bug 369834 just made print preview work again; it had been busted between the cairo landing and that bug being fixed.  And given the information below it's not clear to me that this is a print preview issue at all.

What's going on here is that print preview uses a "72 dpi" device, which means that there are 80 app units per dev pixel as far as layout is concerned.  Then for on-screen display we scale by the ratio of the screen DPI and print DPI (100.0/72.0 in my case).

Then we go to paint print preview and enter nsSimplePageSequenceFrame::PaintPageSequence.  At this point the transform matrix on our mCairo is:
  {xx = 1, yx = 0, xy = 0, yy = 1, x0 = 0, y0 = 0}

We scale so that it becomes:
  {xx = 1.3888888359069824, yx = 0, xy = 0, yy = 1.3888888359069824,
   x0 = 0, y0 = 0}

Then we translate by the offset of the page from the page sequence, which is 360 app units (x and y).  After this translation the matrix is:

  {xx = 1.3888888359069824, yx = 0, xy = 0, yy = 1.3888888359069824, 
   x0 = 6.2499997615814209, y0 = 6.2499997615814209}

Which makes sense: at 80 app units per "device pixel", we're translating by 4.5 "device pixels", then scaling that up by 100.0/72.0.

After this, of course, all our operations are non-pixel aligned and don't get snapped to pixel boundaries because of the scale.  It looks like it doesn't even much matter what the operations are.  I commented out the painting of the actual print preview content, the painting of the page shadow, and all but the following part of the painting of the headers and footers:

    aRenderingContext.PushState();
    aRenderingContext.SetClipRect(aRect, nsClipCombine_kReplace);
    aRenderingContext.PopState();

and I _still_ get the multi-second hang.  Simple removing that SetClipRect call at that point makes things fast.

That said,if I remove the SetClipRect calls but allow the print preview content to paint, I still get the hang.  Presumably we clip somewhere in painting that.

roc suggested just removing the "bail out if we have a scale" code in gfxContext::UserToDevicePixelSnapped and with that change things work nice and fast.

Note that print preview allows the user to scale it, which means there _will_ be scaling involved no matter what.

Comment 9

[Stuart Parmenter]

vlad, can you take a look at this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

By the way, I assume that full zoom would have similar issues in terms of having a non-identity scale in the transform, right?  Is that also going to cause performance issues like this?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No. Full zoom works by changing the appunits-per-devpixel ratio.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I can't reproduce this -- print preview is responsive on all content that I've tried it on.  I don't see a way to zoom print preview, either.

Comment 13

[Boris Zbarsky [:bzbarsky]]

The zoom controls are disabled in the UI on Unix last I checked because the Gtk print backend used to not deal.  Not sure whether that's still an issue.

Note that your ability to reproduce will depend on you exact screen DPI.  For example, if you're at 144 DPI you may not have any sub-pixel anything going on here.

In any case, there's nothing special about content here.  I got major slowness print previewing about:blank, just due to the headers/footers that we put on the pages.  I'm happy to test any fixes you come up with, if that's what you're worried about.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Was at 96 DPI, forcing it to, say, 130 doesn't cause any problems.  I'm not worried about testing a fix, I'm trying to figure out how to debug it to be able to write a fix for testing :)  It certainly sounds like it has to do with non-pixel-aligned clip rects, and I can try to bang out a patch for that (maybe have a flag on gfxContext that tells it to always do pixel snapping for rectangles or something), but I'm not sure if it's the right fix.

I'd also like to understand why I can't reproduce the problems :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Any DPI that's not a multiple of 72 should be giving you non-pixel-aligned rects, I would think.

I'm also happy to provide whatever info you want (e.g. apply patches and attach the output, walk through stuff in gdb, whatever).

And I'd like to understand why you can't reproduce this too.  ;)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't we just snap the cliprect to pixels when possible? Boris could test that and if it fixes the problem for him, no-one else needs to reproduce this.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Wait a sec.  How about we make nsIRenderingContext::SetClipRect always round to pixels?  That way we punt on the problem until we get rid of nsIRenderingContext...  I think a normal rounding should be fine (that is, not an outwards or inwards round), and should give us correct output.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: potential fix?

bz, can you give this a shot?

Comment 19

[Boris Zbarsky [:bzbarsky]]

That patch solves the problem over here.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 287802 [details] [diff] [review]
potential fix?

Sounds good to me!

(Patch has a comment change in nsThebesImage that's not part of this, ignore that hunk)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 287802 [details] [diff] [review]
potential fix?

Fine ... but why does UserToDevicePixelSnapped bail out when a scale is present, anyway?

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 287802 [details] [diff] [review]
potential fix?

It bails out because the idea was to only snap based on non-integer translation; if you're using rectangles for things other than clip, I think you don't want to snap if there is a scale

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> if you're using rectangles for things other than clip, I think you don't want
> to snap if there is a scale

Why?

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #23)
> > if you're using rectangles for things other than clip, I think you don't want
> > to snap if there is a scale
> 
> Why?

Because I don't think you want those to be pixel-aligned, if you're drawing a scaled rectangle, especially scaled down.

Then again, maybe I'm wrong here -- we already have a separate flag for pixel snapping, so maybe everything that's snapping wants to always snap, even in the presence of a scale.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems at least scaling up, you'd want to keep on snapping. I see your point about scaling down ... stuff could just disappear. Hmm. I guess that's a potential problem with this patch too ... very small clipped elements could vanish. On the other hand, blending elements together into a single pixel isn't all that helpful either. Hmm.

One approach might be a gfxContext state flag that controls whether we want snapping to check the scale or not. Then clients can choose which effect they want.

Another approach would be to just speed up clipping by having non-pixel-aligned clips do a push_group/pop_group so drawing in the interim doesn't have to go through the mask path. This would require more state in gfxContext or nsIRenderingContext to track when a Restore needs to pop one or more clipping groups.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 287802 [details] [diff] [review]
potential fix?

Requesting approval here -- there may be a followup patch/bug to this if we decide to do something different, but this improves things at the moment.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in.  roc, let's chat on IRC and figure out if we want to go further here.