1082894 - Allow passing a region to invalidate when scheduling a composition

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For bug 1082892, I want to schedule a composition when a mouse moves, in order to draw the mouse cursor.  With BasicCompositor, we need to invalidate the region to be redrawn.  I'm handling mousemove events from a non-main, non-compositor thread so I'm using the CompositorParent::ScheduleRenderOnCompositorThread() API.  I don't want to deal with any more thread hopping to invalidate my region, so I've extended the ScheduleRenderOnCompositorThread() method to accept a region to invalidate when the render is scheduled (defaulting to empty region).

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Allow passing a region to invalidate when scheduling a composition

Hey Benoit, looks like you reviewed code in here recently.  Let me know if I should tag someone else for review.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8505172 [details] [diff] [review]
Allow passing a region to invalidate when scheduling a composition

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

::: gfx/layers/ipc/CompositorParent.h
@@ +152,5 @@
>  
>    void AsyncRender();
>  
>    // Can be called from any thread
> +  void ScheduleRenderOnCompositorThread(const nsIntRegion& aInvalidate = nsIntRegion());

I don't like this API because it's inherently racy. You have something here that's being changed out band. I checked your other patch and you seem to be fine because you invalidate the whole screen.

Imagine that you send an IPC message to the compositor to invalidate x=10,y=10,w=10,h=10 which is queued in the compositor thread, meanwhile the main thread moves cursor position to x=15,y=10,w=10,h=10. Then the compositor processes the schedule render composite while invalidating x=10,y=10,w=10,h=10 and so we end up invalidating only a subset of the cursor.

The obvious thing to do is the have the invalidate map 1:1 with the draw. If we're trying to do something quick and dirty here then I'd suggest just adding an API to invalidate everything which is how the calle of this code is using this anyways. If we want to invalidate subrect then we should deal with this problem.

Or maybe it's not racy and I missed something, let me know.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Benoit Girard (:BenWa) from comment #2)
> I don't like this API because it's inherently racy.

I understand the problem you're referring to, but in my mind the burden is on the caller to ensure that the invalidation requests synchronize properly with whatever state changes trigger the invalidation request.  (It so happens that the first user of this API doesn't need to synchronize and also invalidates everything anyway.)

> The obvious thing to do is the have the invalidate map 1:1 with the draw.

I don't have a problem with that, but I think the API would look pretty much the same.  I don't think CompositorParent knows the bounds of its underlying surface, but I don't recall.  What would you suggest?

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #3)
> I understand the problem you're referring to, but in my mind the burden is
> on the caller to ensure that the invalidation requests synchronize properly
> with whatever state changes trigger the invalidation request.  (It so
> happens that the first user of this API doesn't need to synchronize and also
> invalidates everything anyway.)

I would rather not have obvious footguns. We have better options here anyways. Had a quick chat with the jeff&bjacob about this and native cursors and they're not a fan either even for a quick prototype.

> 
> > The obvious thing to do is the have the invalidate map 1:1 with the draw.
> 
> I don't have a problem with that, but I think the API would look pretty much
> the same.  I don't think CompositorParent knows the bounds of its underlying
> surface, but I don't recall.  What would you suggest?

There's mEGLSurfaceSize which is OGL only. We could add something like this to basic but that's not great. We could either invalidate a ridiculous size but that's not great either (I'd be happy with this solution over the current patch FWIW because it's not racy).

I think it would be much better to just add a message to set a cursor position, set a cursor image using a surface and implement it in the compositor. It's not that much work and it avoids this racy API. The compositor API can let us implement this to be entirely platform neutral easily.

I imagine you're quickly going to run into a main thread software cursor being really bad anyways. It will be easily extensible there to move to an async (Compositor thread) cursor and better positioned to someday move to a hardware cursor.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Benoit Girard (:BenWa) from comment #4)
> I think it would be much better to just add a message to set a cursor
> position, set a cursor image using a surface and implement it in the
> compositor. It's not that much work and it avoids this racy API. The
> compositor API can let us implement this to be entirely platform neutral
> easily.
> 

That ties the setting of the cursor position to the main thread (the one that speaks PCompositor), which is an inherent responsiveness limitation.

Separately, there's nothing platform-specific about the patches I wrote, I just parked the code in gonk widgetry because there are no other consumers of the API yet.  A comment in the patch in bug 1082892 explains this.

> I imagine you're quickly going to run into a main thread software cursor
> being really bad anyways. It will be easily extensible there to move to an
> async (Compositor thread) cursor and better positioned to someday move to a
> hardware cursor.

I don't understand this comment, sorry.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't know what the next steps are here.  This patch itself is pretty minor, but it's required for bug 1082892 which I would like to land.  I had hoped we could deal with this patch on its own because it's very small, but since we've pulled the other bug into this discussion and we're not all clear on how things fit together, let me try explaining briefly

 - for FFOS devices like Raspberry Pi that usually use mice instead of touch screens, we want to draw a mouse pointer (otherwise the mouse is unusable).  This is also useful for phones that have styluses, like the Galaxy Notes.
 - gecko can't rely on anything else to draw the mouse/stylus pointer on FFOS.  It has to draw the pointer itself.
 - we don't want to force pointer updates to hop through the main thread, because the main thread can be busy for "awhile" and pointer responsiveness is crucial with mice
 - in the gonk backend, mousemove events are read on a dedicated input-event thread, and pointer updates (via recomposition) are directly scheduled from that thread
 - the pointer itself is drawn by gonk nsWindow on the compositor thread, when the scheduled composition occurs

It so happens that we're forced to use BasicCompositor on Raspberry Pi for now because the GL impl isn't ready for production.  This patch is required to allow the pointer updating code to force a recompose.  However, if we find it useful to support scheduling a re-render from non-compositor threads, and we track invalid regions on the compositor even when they're not always used, it also seems generally useful to me to allow atomically invalidating a region along with a scheduled recompose.  Hence this separate bug and patch.

I don't know what "main thread software cursor" refers to in this picture.  For one, the main thread isn't involved in anything.  I also don't know what "hardware cursor" is supposed to mean.  When the RPi's GL impl is ready, we'll switch to drawing the cursor with GL (or composer2d).  But we'd still use this same code to schedule the rerenders.  And on chips that support it, it would be even better to draw the cursor using overlay hardware.

If we can't get past this patch, let's just WONTFIX this bug asap so we don't continue wasting our time.  It's not absolutely essential to upstream this patch, but it would make it easier for everyone hacking on RPi.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I should add that IMO it's a style issue whether compositor or widget owns the cursor.  But since cursors are a widget-y thing and we already support drawing widget over/underlays, it makes more sense to me to stick the cursor hackery in widget.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Skim read this bug quickly, so maybe not useful, but we have ClientLayerManager::SendInvalidRegion which widget/windows/nsWindowGfx.cpp uses.

I can read through this properly tomorrow if it'd be helpful.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yep, except that we schedule a recomposite to update the pointer from a non-main, non-compositor thread.  To use SendInvalidRegion(), we'd have to hop back to the main thread which would put an inherent limit on pointer responsiveness.  (Or open another another IPC channel to the compositor, which doesn't really help anything.)

I don't think it's worth your time to do a close read of this bug.  I'm a bit grouchy because I don't want to touch gecko gfx anymore, but honestly it wouldn't be the end of the world if we didn't upstream this patch.  Just a small-ish burden on early-adopter FFOS-on-RPi devs.

Comment 10

[Benoit Girard (:BenWa)]

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #9)
> honestly it
> wouldn't be the end of the world if we didn't upstream this patch.  Just a
> small-ish burden on early-adopter FFOS-on-RPi devs.

I'd rather not. I'm not trying to impede any progress here, I just want to see if we can come up with a simple solution without taking on technical debt (or understand how invalidating a subset from another thread of the screen isn't racy).

I'm open to -any- alternative that doesn't introduce a racy API.

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #9)
> Yep, except that we schedule a recomposite to update the pointer from a
> non-main, non-compositor thread. 

Ahh ok, I saw widget so I figured we were calling it from the main thread. But it's still racy as far as I can see.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh well, I tried.  Let's continue to assume that BasicCompositor is never used for omtc, and not add this small extension to an already inherently racy and broken API.

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #9)
> Yep, except that we schedule a recomposite to update the pointer from a
> non-main, non-compositor thread.  To use SendInvalidRegion(), we'd have to
> hop back to the main thread which would put an inherent limit on pointer
> responsiveness.  (Or open another another IPC channel to the compositor,
> which doesn't really help anything.)

Right, sorry, that was in your first comment too.

Do you have use cases for invalidating regions that aren't the entire window with this API?

I feel that making this just take a bool/flag to indicate you want to override any existing invalid region and just recomposite the whole window would be pretty reasonable.

It's still possible that the compositor composites between you changing sCursor and the ScheduleRenderOnCompositorThread message being processed, but EndRemoteDrawing doesn't actually take the invalid region into account (it just needs to be non-empty for us to get there) so this doesn't matter.

Note that setting sCurson on your event thread and reading it from the compositor thread is potentially racy too, but that's contained within the gonk code so doesn't bother me much.

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #11)
> Oh well, I tried.  Let's continue to assume that BasicCompositor is never
> used for omtc, and not add this small extension to an already inherently
> racy and broken API.

We're actually using BasicCompositor on windows in some cases, we just shipped that with 32.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Do you have use cases for invalidating regions that aren't the entire window
> with this API?

If I wanted to actually draw the cursor efficiently with BasicCompositor, I would only invalidate its old destination and new destination.  However, since BasicCompositor is a workaround for me, I don't care enough to go to that trouble.

> Note that setting sCurson on your event thread and reading it from the
> compositor thread is potentially racy too, but that's contained within the
> gonk code so doesn't bother me much.
> 

Yep, as I mentioned above there's no real reason to synchronize that for now.  But if we wanted to, it would be relatively simple to do.

> (In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need
> me from comment #11)
> > Oh well, I tried.  Let's continue to assume that BasicCompositor is never
> > used for omtc, and not add this small extension to an already inherently
> > racy and broken API.
> 
> We're actually using BasicCompositor on windows in some cases, we just
> shipped that with 32.

Oh boy ... I've seen some pretty substantial invalidation glitchiness with BasicCompositor and omtc.  Hopefully it only bites content processes ...