Union dirty rects instead of painting all of them

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pcwalton, Assigned: cwiiis)

Tracking

unspecified
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
We should be painting only one dirty rectangle per frame. Otherwise we get too many allocations and this can cause crashes.
Priority: -- → P1
(Assignee)

Comment 1

6 years ago
Created attachment 577948 [details] [diff] [review]
Union dirty rectangles

Union the dirty rectangles and squash multiple draw events, instead of handling all of them.
Attachment #577948 - Flags: review?(kgupta)
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.
Attachment #577948 - Flags: review?(kgupta) → review-
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
Created attachment 577964 [details] [diff] [review]
Union dirty rectangles (address review comments)
Assignee: pwalton → chrislord.net
Attachment #577948 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #577964 - Flags: review?(kgupta)
Attachment #577964 - Flags: review?(kgupta) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/projects/birch/rev/c9164cc832de
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd1048056a6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/1bd1048056a6
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.