676250 - Make OpenGL painting bypass the Cocoa setNeedsDisplay/drawRect machinery

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

Attachment: wip

With OpenGL painting, we're doing some unnecessary work during invalidation: We store the invalid region on the NSWindow using -[NSView setNeedsDisplayInRect:]. That's not necessary since we'll be recompositing the whole window anyway, and Gecko knows which regions it needs to repaint inside the retained layers because each layer stores its own dirty region.

One case where avoiding the unnecessary region unioning is bug 554004 comment 2. 

Another reason why we don't really want to use setNeedsDisplay  for GL painting is given in bug 676248.

Instead of using setNeedsDisplayInRect and waiting for an invocation of drawRect, we can just use the Objective C equivalent of posting an nsRunnable for drawing. The wip patch should be mostly complete, except it doesn't post NS_WILL_PAINT events yet.

Comment 1

[Markus Stange [:mstange]]

Attachment: v1

This seems to work well. It's needed for bug 675410.

Comment 2

[Markus Stange [:mstange]]

(In reply to Markus Stange from comment #0)
> except it
> doesn't post NS_WILL_PAINT events yet.

I'm pretty sure that NS_WILL_PAINT events aren't necessary because all Gecko-originating invalidations come from the refresh driver anyway which has already flushed everything.

Comment 3

[Josh Aas]

Comment on attachment 675318 [details] [diff] [review]
v1

Review of attachment 675318 [details] [diff] [review]:
-----------------------------------------------------------------

Benoit Girard would be a better reviewer.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 675318 [details] [diff] [review]
v1

Review of attachment 675318 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I'm sure you've checked this but just to be safe: Make sure it is well tested for window resize (with and without personas see bug 797431), and losing/regaining window focus (bug 770056).

::: widget/cocoa/nsChildView.mm
@@ +2186,5 @@
> +    return;
> +
> +  if ([self isUsingOpenGL]) {
> +    // When using OpenGL we don't want to go through the usual Cocoa painting
> +    // machinery for performance reasons. We just use a timer instead.

s/timer/refresh driver.

Instead of 'performance reasons' we should elaborate. If I understand it correctly it prevents us from repainting the window surface. We should note that here.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

This patch is a few months old and has bit-rotted. I'll see if I can bring it up to date.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: v1.1

De-bitrotted.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 (r+'d by BenWa)

Addressed Benoit's review comments.

Comment 8

[Markus Stange [:mstange]]

Actually, sorry, I should have answered the comments because I don't think they're valid.

(In reply to Benoit Girard (:BenWa) from comment #4)
> ::: widget/cocoa/nsChildView.mm
> @@ +2186,5 @@
> > +    return;
> > +
> > +  if ([self isUsingOpenGL]) {
> > +    // When using OpenGL we don't want to go through the usual Cocoa painting
> > +    // machinery for performance reasons. We just use a timer instead.
> 
> s/timer/refresh driver.

The timer I'm referring to here is the [self performSelector:@selector(drawUsingOpenGLCallback) withObject:nil afterDelay:0] Cocoa timer. The refresh driver would be used either way. So I think we should keep "timer".

> Instead of 'performance reasons' we should elaborate. If I understand it
> correctly it prevents us from repainting the window surface. We should note
> that here.

Well, the real reason is more complicated than that and wouldn't fit in a comment. Specifically, it prevents us from accessing the normal window buffer surface unnecessarily, so we waste less time synchronizing the two surfaces. (These synchronizations show up in a profiler as CGSDeviceLock / _CGSLockWindow / _CGSSynchronizeWindowBackingStore.)
It also means that Cocoa doesn't have any potentially expensive invalid rect management for us.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I was originally going to land this patch with Benoit's changes, but I'll hold off until the above is resolved.

Comment 10

[Benoit Girard (:BenWa)]

Ohh I see now. You're right refresh driver isn't the right word. Timer isn't quite the right word either, we're just en-queue an event to run refresh outside of setNeedsDisplayInRect. The explanation you give is great and I don't see why doesn't fit in a comment. Perhaps something along those lines:

Draw the frame outside of setNeedsDisplayInRect to prevent us from needing to access the normal window buffer surface unnecessarily, so we waste less time synchronizing the two surfaces. (These synchronizations show up in a profiler as CGSDeviceLock / _CGSLockWindow / _CGSSynchronizeWindowBackingStore.)
It also means that Cocoa doesn't have any potentially expensive invalid rect management for us.

The comment is big but the code on it's own isn't clear without it.

Comment 11

[Markus Stange [:mstange]]

Good, let's use that comment. How about "delayed perform" instead of "timer"? I'm having a hard time coming up with a good word. But we should be consistent and replace "timer" in this comment, too:

+    // We usually don't get here for Gecko-initiated repaints. Those call
+    // drawUsingOpenGL from a timer and don't go through drawRect.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3 (r+'d by BenWa)

Alright, I used Benoit's comment, and replaced the "timer" comment in the other block with this:

// We usually don't get here for Gecko-initiated repaints. Instead, those  
// eventually call drawUsingOpenGL, and don't go through drawRect.         
// Paints that come through here are triggered by something that Cocoa     
// controls, for example by window resizing or window focus changes.       

If that comment is incorrect / needs updating, let me know.

I've pushed to inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/c95439870e05

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c95439870e05

Comment 14

[Markus Stange [:mstange]]

I approve of the comment. Thanks!