Last Comment Bug 704575 - Union dirty rects instead of painting all of them
: Union dirty rects instead of painting all of them
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal (vote)
: ---
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-22 12:15 PST by Patrick Walton (:pcwalton)
Modified: 2012-01-09 10:07 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Union dirty rectangles (3.86 KB, patch)
2011-11-30 07:14 PST, Chris Lord [:cwiiis]
bugmail: review-
Details | Diff | Splinter Review
Union dirty rectangles (address review comments) (7.47 KB, patch)
2011-11-30 08:02 PST, Chris Lord [:cwiiis]
bugmail: review+
Details | Diff | Splinter Review

Description Patrick Walton (:pcwalton) 2011-11-22 12:15:45 PST
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] 2011-11-30 07:14:43 PST
Created attachment 577948 [details] [diff] [review]
Union dirty rectangles

Union the dirty rectangles and squash multiple draw events, instead of handling all of them.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-30 07:42:26 PST
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] 2011-11-30 08:01:55 PST
(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] 2011-11-30 08:02:39 PST
Created attachment 577964 [details] [diff] [review]
Union dirty rectangles (address review comments)
Comment 5 Chris Lord [:cwiiis] 2011-11-30 08:18:07 PST
http://hg.mozilla.org/projects/birch/rev/c9164cc832de
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-03 03:27:40 PST
https://hg.mozilla.org/mozilla-central/rev/1bd1048056a6

Note You need to log in before you can comment on or make changes to this bug.