446376 - window.scrollBy(10,10) redraw whole image instead small regions.

Status: RESOLVED WORKSFORME

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Oleg Romashin (:romaxa)]

Uncomment debug info in nsThebesImage:
http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesImage.cpp#426

open URL,
load next url 'javascript:window.scrollBy(10,10);'

Result:
sending expose event [0x8442cc8] 0x8078b28 0xe0006a (rects follow):
        664 0 10 330
        0 330 674 10
nsThebesImage::Draw src [1530.000000 922.000000 674.000000 340.000000] dest [0.000000 0.000000 674.000000 340.000000] trans: [0.000000 0.000000] dec: [0.000000 0.000000]

Instead repainting of two rectangles we are repainting whole image  vert.rect-x-horiz.rect

PS: only horiz or only vert scrolling works fine:
sending expose event [0x8442cc8] 0x8078b28 0xe0006a (rects follow):
        0 321 674 19
nsThebesImage::Draw src [1530.000000 1262.000000 674.000000 19.000000] dest [0.000000 321.000000 674.000000 19.000000] trans: [0.000000 0.000000] dec: [0.000000 0.000000]

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix

Probably we have to call list.Paint for each region subrect....

Small optimization can be added by using aDirtyRegion.GetBounds() if GetBounds ~ sum of all subrects.

Comment 2

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

It would be better to change the third parameter of Paint to const nsRegion&.

Comment 3

[Oleg Romashin (:romaxa)]

> It would be better to change the third parameter of Paint to const nsRegion&.
> 
What about all other functions:

nsThebesImage::Draw
nsLayoutUtils::DrawImage
nsImageFrame::PaintImage
nsDisplayImage::Paint
nsDisplayList::Paint
nsDisplayWrapList::Paint
nsDisplayClip::Paint
nsDisplayList::Paint

Change third parameter for them also?

Comment 4

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

(In reply to comment #3)
> nsThebesImage::Draw
> nsLayoutUtils::DrawImage

No

> nsImageFrame::PaintImage
> nsDisplayImage::Paint
> nsDisplayList::Paint
> nsDisplayWrapList::Paint
> nsDisplayClip::Paint
> nsDisplayList::Paint
> 
> Change third parameter for them also?

Yes.

Comment 5

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

Er, no -- we call paint on the entire image dimensions, but we're clipped to the dirty region already.  Only a portion of the image is actually painted.  However, the problem is actually here:

sending expose event [0x8442cc8] 0x8078b28 0xe0006a (rects follow):
        664 0 10 330
        0 330 674 10

we're calling expose on two fairly disjoint rectangles, and I presume we're getting that same region back.  Or, worse, that they're being combined in the union of the two rectangles.

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix r2

Changed third argument to nsRegion for all functions.

Comment 7

[Oleg Romashin (:romaxa)]

(In reply to comment #5)
> Er, no -- we call paint on the entire image dimensions, but we're clipped to
> the dirty region already.  Only a portion of the image is actually painted. 

Initially problem was noticed on Xserver side while panning/scrolling with stylus on N800... Xserver was redrawing whole image on each diagonal scroll.

> However, the problem is actually here:
> 
> sending expose event [0x8442cc8] 0x8078b28 0xe0006a (rects follow):
>         664 0 10 330
>         0 330 674 10
> 
> we're calling expose on two fairly disjoint rectangles, and I presume we're
> getting that same region back.  Or, worse, that they're being combined in the
> union of the two rectangles.
They're being combined here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#988
> 

Comment 8

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

Comment on attachment 330614 [details] [diff] [review]
Proposed fix r2

I don't think we should do this, but should instead fix the actual problem.

(In reply to comment #7)
> (In reply to comment #5)
> > Er, no -- we call paint on the entire image dimensions, but we're clipped to
> > the dirty region already.  Only a portion of the image is actually painted. 
> 
> Initially problem was noticed on Xserver side while panning/scrolling with
> stylus on N800... Xserver was redrawing whole image on each diagonal scroll.

The X server should be fixed to take into account any clip region, then.

> They're being combined here:
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#988

That shouldn't matter -- the clip should be being set in the expose event.  If it's not, that's the problem that should be fixed.

Comment 9

[Oleg Romashin (:romaxa)]

> That shouldn't matter -- the clip should be being set in the expose event.  If
> it's not, that's the problem that should be fixed.

Vlad can you explain how does it works?
For example I have some specific display Item which supposed to be draw in some specific way (scaling, decoding and something else...).
I'm receiving paint event with some aDirtyRect, how can get ClipRegion and draw/decode/handle only needed part?

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 330614 [details] [diff] [review]
Proposed fix r2


Also instead this mass change we can use OptimizeVisibility function to remember and use nsRegion.

Comment 11

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

Any low-level painting code that needs the actual area that'll be covered can always pull out the current clip region/rects from the nsIRenderingContext (or better, the GfxContext).

For the expose code, I was referring to this: http://hg.mozilla.org/mozilla-central/index.cgi/file/88bbab534518/widget/src/gtk2/nsWindow.cpp#l1641

but it looks like that already does the right thing with multiple rectangles, and the display should already be clipped to the view region.

So I'm not sure what the problem is -- drawing the entire image (by specifying the entire rectangle) should only result in the parts that aren't being clipped out being computed.  If something more is being computed, then that's a bug in cairo or in pixman (which the X server also uses).

Comment 12

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

Or, I should say, we might have a non-rectangular clip (or perhaps non-integer-aligned clip) at that point and so mask clipping is happening, which will end up with the full area rendered.

Comment 13

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

Comment on attachment 330614 [details] [diff] [review]
Proposed fix r2

minusing per Vlad

Comment 14

[Oleg Romashin (:romaxa)]

Seems clipping works fine on X86 scratchbox.

Func:void nsImageFrame::PaintImage(nsIRenderingContext&, nsPoint, const nsRect&, imgIContainer*)::1183
Func:_cairo_xlib_surface_set_picture_clip_rects::761
Func:_cairo_xlib_surface_set_gc_clip_rects::790
nsThebesImage::Draw src [1084.000000 522.000000 674.000000 340.000000] dest [0.000000 0.000000 674.000000 340.000000] trans: [0.000000 0.000000] dec: [0.000000 0.000000]
Func:void nsImageFrame::PaintImage(nsIRenderingContext&, nsPoint, const nsRect&, imgIContainer*)::1185
Func:_cairo_xlib_surface_set_clip_region::1985, n_boxes:[0]:[664,0,10,330]
Func:_cairo_xlib_surface_set_clip_region::1985, n_boxes:[1]:[0,330,674,10]
Func:_cairo_xlib_surface_ensure_gc::816
Func:_cairo_xlib_surface_set_gc_clip_rects::784 surface->num_clip_rects:2


#0  _cairo_xlib_surface_ensure_gc (surface=0x84380d8) at gfx/cairo/cairo/src/cairo-xlib-surface.c:816
#1  0xb4471988 in _cairo_xlib_surface_composite (op=CAIRO_OPERATOR_OVER, src_pattern=0x81734c0, mask_pattern=0x0, abstract_dst=0x84380d8, src_x=0, src_y=0, mask_x=0, mask_y=0, dst_x=0, dst_y=0, 
    width=674, height=340) at gfx/cairo/cairo/src/cairo-xlib-surface.c:1575
#2  0xb4458f7a in _cairo_surface_composite (op=CAIRO_OPERATOR_OVER, src=0x81734c0, mask=0x0, dst=0x84380d8, src_x=0, src_y=0, mask_x=0, mask_y=0, dst_x=0, dst_y=0, width=674, height=340)
    at gfx/cairo/cairo/src/cairo-surface.c:1227
#3  0xb445b455 in _composite_trap_region (clip=0x81ee15c, src=0x81734c0, op=CAIRO_OPERATOR_OVER, dst=0x84380d8, trap_region=0xbfe492d4, extents=0xbfe492b4)
    at gfx/cairo/cairo/src/cairo-surface-fallback.c:449
#4  0xb445b6d8 in _clip_and_composite_trapezoids (src=0x81734c0, op=CAIRO_OPERATOR_OVER, dst=0x84380d8, traps=0xbfe49344, clip=0x81ee15c, antialias=CAIRO_ANTIALIAS_NONE)
    at gfx/cairo/cairo/src/cairo-surface-fallback.c:644
#5  0xb445badc in _cairo_surface_fallback_paint (surface=0x84380d8, op=CAIRO_OPERATOR_OVER, source=0x81734c0)
    at gfx/cairo/cairo/src/cairo-surface-fallback.c:709
#6  0xb4459446 in _cairo_surface_paint (surface=0x84380d8, op=CAIRO_OPERATOR_OVER, source=0x1)


I need to investigate more what is going on... on server and cairo on arm.

Comment 15

[Oleg Romashin (:romaxa)]

I have made 2 builds with and without patch:
https://garage.maemo.org/svn/browser/mozilla/trunk/microb-engine/microb-engine/debian/patches/perf_tweaks/bug446376_easy.diff

And diagonal scrolling performance very visible if content zoomed.
 

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: TiledBackground testcase

The same problem with fixed tiled background.
Zoom attached testcase with full zoom.

Try to scroll it up-down, left-right, and diagonal or circle movements...

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: BackTrace for fbFetchTransformed_Nearest_Normal pixman call

Even if we are clipping we still compositing full size boundsRect.

Comment 18

[Oleg Romashin (:romaxa)]

Also here we are clipping whole dirtyRect....
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2181

Vlad is it supposed to be so? or we should clip only region subrects?

Comment 19

[Oleg Romashin (:romaxa)]

Also I think just clipping not enough... because we are doing some additional operations in some cases by using temporary surfaces, which does not know anything about clipping .

Comment 20

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

(In reply to comment #19)
> Also I think just clipping not enough... because we are doing some additional
> operations in some cases by using temporary surfaces, which does not know
> anything about clipping .

That's a good point actually -- if we have a temporary, even one created by PushGroup(), the clip is reset to the bounds.  So that might have a lot to do with it, and we should make sure to restore the clip to that surface whenever we do that.  That could be exciting, but should help a lot here.

Comment 21

[Oleg Romashin (:romaxa)]

What is the right way to do it? 
Should we move dirtyRegion to DrawTile/Image functions and use it there?

Comment 22

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

> That's a good point actually -- if we have a temporary, even one created by
> PushGroup(), the clip is reset to the bounds.  So that might have a lot to do
> with it, and we should make sure to restore the clip to that surface whenever
> we do that.  That could be exciting, but should help a lot here.

Hm, I'm lying here -- PushGroup() and friends do propagate the clip to the new temporary.  It might not get propagated to any places where a temporary wasn't created via PushGroup though, but there aren't many of those.  The nsCSSRendering clip-to-drityrect bit really shouldn't be needed; should be safe to get rid of it entirely (all drawing should be clipped to the dirty rect/region anyway).

In the original patch, the real change happened in nsImageFrame::PaintImage.. I think we need to figure out two things:

1) Why the destination doesn't have the correct clip already set (Gecko bug); or, if it does:

2) Why pixman/cairo isn't taking advantage of the much smaller clip region when compositing happens (cairo bug).

If the second, we can get that fixed inside cairo.  If the first, we should figure out who's not correctly setting the clip in Gecko.

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Is this issue still active?

Comment 24

[Oleg Romashin (:romaxa)]

No this is already fixed with new layers graphics