704575 - Union dirty rects instead of painting all of them

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

We should be painting only one dirty rectangle per frame. Otherwise we get too many allocations and this can cause crashes.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Union dirty rectangles

Union the dirty rectangles and squash multiple draw events, instead of handling all of them.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 577948 [details] [diff] [review]
Union dirty rectangles

>+
>+            // Combine the next draw event's rect with the last one in the queue
>+            const nsIntRect& nextRect = nextEvent->Rect();
>+            const nsIntRect& lastRect = mLastDrawEvent->Rect();
>+            mLastDrawEvent->Init(AndroidGeckoEvent::DRAW, lastRect.Union(nextRect));

(As mentioned on IRC) I think we should try to detect cases where unioning two small rects results in an abnormally large rect - for example if there is a 1x1 rect in the top-left corner and a 1x1 rect in the bottom-right corner. We can heuristically detect this by checking the area of the unioned rect to see if it is greater than 10x the sum of the original rects, or something of that sort. Logging this would give us an idea of how often this happens and whether or not we should handle it.

[0];
>         mEventQueue.RemoveElementAt(0);
>         if (ae->Type() == AndroidGeckoEvent::DRAW) {
>-            mNumDraws--;
>+            if (--mNumDraws == 0)
>+                mLastDrawEvent = nsnull;

Shouldn't this hunk also be added to RemoveNextEvent()? In fact, RemoveNextEvent() should be rewritten to just call GetNextEvent() and discard the return value, since it's duplicate code. r-

Also, do we care about coalescing DRAW events that are separated by events other than move/motion events? i.e. if we get DRAW ACCELERATION DRAW I think we should still be able to coalesce them. But if this patch is "good enough" in that it handles the most common cases then that's fine.

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #2)
> Comment on attachment 577948 [details] [diff] [review] [diff] [details] [review]
> Union dirty rectangles
> 
> >+
> >+            // Combine the next draw event's rect with the last one in the queue
> >+            const nsIntRect& nextRect = nextEvent->Rect();
> >+            const nsIntRect& lastRect = mLastDrawEvent->Rect();
> >+            mLastDrawEvent->Init(AndroidGeckoEvent::DRAW, lastRect.Union(nextRect));
> 
> (As mentioned on IRC) I think we should try to detect cases where unioning
> two small rects results in an abnormally large rect - for example if there
> is a 1x1 rect in the top-left corner and a 1x1 rect in the bottom-right
> corner. We can heuristically detect this by checking the area of the unioned
> rect to see if it is greater than 10x the sum of the original rects, or
> something of that sort. Logging this would give us an idea of how often this
> happens and whether or not we should handle it.

Good idea, I've added a warning when the area of the new dirty rectangle exceeds the combined area of the previous rectangles by more than 8 times (as discussed).

> [0];
> >         mEventQueue.RemoveElementAt(0);
> >         if (ae->Type() == AndroidGeckoEvent::DRAW) {
> >-            mNumDraws--;
> >+            if (--mNumDraws == 0)
> >+                mLastDrawEvent = nsnull;
> 
> Shouldn't this hunk also be added to RemoveNextEvent()? In fact,
> RemoveNextEvent() should be rewritten to just call GetNextEvent() and
> discard the return value, since it's duplicate code. r-

I've removed RemoveNextEvent and renamed GetNextEvent, PopNextEvent.

> Also, do we care about coalescing DRAW events that are separated by events
> other than move/motion events? i.e. if we get DRAW ACCELERATION DRAW I think
> we should still be able to coalesce them. But if this patch is "good enough"
> in that it handles the most common cases then that's fine.

This should already happen - it merges all draw events into the last in the queue, no matter what's before it, then it coalesces consecutive motion events.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Union dirty rectangles (address review comments)

Comment 5

[Chris Lord [:cwiiis]]

http://hg.mozilla.org/projects/birch/rev/c9164cc832de

Comment 6

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd1048056a6

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/1bd1048056a6