Closed Bug 1647877 Opened 2 years ago Closed 2 years ago

Remote GDI presentation path always updates the entire window, should use dirty region

Categories

(Core :: Graphics, defect)

Desktop
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- wontfix
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 --- wontfix
firefox80 --- fixed

People

(Reporter: mstange, Assigned: cmartin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

CompositorWidget::StartRemoteDrawingInRegion has a region parameter that lets it optimize the window update region. But the remote GDI drawing path does not make use of that region and always updates the entire window, which is rather wasteful.

:cmartin, can you comment to the bug?

Flags: needinfo?(cmartin)

I agree it seems like this could be done more efficiently. Bug 1604412 was already getting quite complex, and so I took a "make it work, then make it fast" approach. It seems like it's time to do some work towards the second part of that :)

I can take a crack at implementing this when I get some free cycles, but in case anyone else is inclined to take it on - Probably the solution will involve "remembering" the update region from StartRemoteDrawingInRegion() and then sending it to the remote_backbuffer::Provider via the shared memory region during the EndRemoteDrawing() call.

I'm more inclined to think of this as an "enhancement" rather than a regression, as this is IMO new and unprecedented behaviour. We knew we were going to have to accept some amount of GDI perf hit for this GPU sandbox, and so this seems like an improvement on that initial feature.

Flags: needinfo?(cmartin)

I agree with the "make it work, then make it fast" approach. But please do treat this with a high priority. We've seen cases where the memory bandwidth overhead from updating the whole window can affect browser performance in unexpected ways. See bug 1406414 for example: The throbber animation during page load can starve resources from the rest of the browser so that page load takes a very long time.

(In reply to Markus Stange [:mstange] from comment #3)

I agree with the "make it work, then make it fast" approach. But please do treat this with a high priority. We've seen cases where the memory bandwidth overhead from updating the whole window can affect browser performance in unexpected ways. See bug 1406414 for example: The throbber animation during page load can starve resources from the rest of the browser so that page load takes a very long time.

Fair enough - I will put it near the top of my todo list.

See Also: → 1638675
Severity: -- → S3
OS: Unspecified → Windows
Hardware: Unspecified → Desktop

Seems unlikely the fix would be simple enough we'd take it in 78.

For initial implementation of remote backbuffer, I took a simple approach of
just BitBlt-ing the entire backbuffer contents to the window.

This now only updates the bounding rectangle of the dirty region.

Assignee: nobody → cmartin

Changing bug to "leave-open" because Markus pointed out that there is already machinery in place to reduce a region to a maximum number of rectangles, which is much simpler to pass over IPC. I will follow this first changeset up with another that allows up to 8 update rectangles, as Markus suggested that seems like a good "sweet spot"

Keywords: leave-open
Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d861a4e70043
Remote Backbuffer: Only BitBlt dirty region r=mstange

Removed leave-open tag - Will file a new bug for the follow-up instead.

Keywords: leave-open
Blocks: 1654617
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.