Create a top-level protocol between the paint thread and compositor

RESOLVED WONTFIX

Status

()

Core
Graphics: Layers
RESOLVED WONTFIX
11 months ago
11 months ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

We need a new top-level protocol to connect the paint thread to the compositor thread. We also need LayerTransactionParent to be able to find TextureClients created by this protocol, similar to how LTP needs to find async compositables via the static ImageBridgeParent map.
Created attachment 8878809 [details] [diff] [review]
part 1, move PaintThread shutdown

Since the IPC channel will be bound to the paint thread, we need shutdown to be on the paint thread as well. (At least, this part of it, but probably anything we add should be deleted on that thread anyway.)
Attachment #8878809 - Flags: review?(mchang)
Created attachment 8878810 [details] [diff] [review]
part 2, PPaintBridge

This adds a skeletal PPaintBridge protocol between the OMTP thread and the compositor thread.

Unlike past gfx protocols, the endpoint here originates from the content process. Hopefully that is okay because I like it better than how past protocols work. It makes endpoint ownership local to the code that uses it, and makes it trivial to bind these parent protocols together.

For example ImageBridgeParents are kept in an std::map so LayerTransactionParent can find the associated one for a process ID. That sort of hackery won't be needed here.
Attachment #8878810 - Flags: review?(wmccloskey)
Comment on attachment 8878809 [details] [diff] [review]
part 1, move PaintThread shutdown

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

::: gfx/layers/PaintThread.cpp
@@ +52,4 @@
>  /* static */ void
>  PaintThread::Shutdown()
>  {
> +  UniquePtr<PaintThread> pt(sSingleton.forget());

nit: Assert main thread.

@@ +57,4 @@
>      return;
>    }
>  
> +  RefPtr<nsIThread> thread = pt->GetThread();

Am I reading this right? We take complete ownership of the singleton and move it to DestroyPaintThread. On the main thread, we call thresd->Shutdown() which spins the event loop. So once DestroyPaintThread finishes, all references to the PaintThread* will die, but we'll still have a reference to the actual underlying nsIThread. The main thread Shutdown() will then finish. So we'll have a pointer to the underlying thread which will be kept alive, but the PaintThread* can die and be cleaned up before the nsIThread has been cleaned up?
(In reply to Mason Chang [:mchang] from comment #3)
> Comment on attachment 8878809 [details] [diff] [review]
> part 1, move PaintThread shutdown
> 
> Review of attachment 8878809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +57,4 @@
> >      return;
> >    }
> >  
> > +  RefPtr<nsIThread> thread = pt->GetThread();
> 
> Am I reading this right? We take complete ownership of the singleton and
> move it to DestroyPaintThread. On the main thread, we call
> thresd->Shutdown() which spins the event loop. So once DestroyPaintThread
> finishes, all references to the PaintThread* will die, but we'll still have
> a reference to the actual underlying nsIThread. The main thread Shutdown()
> will then finish. So we'll have a pointer to the underlying thread which
> will be kept alive, but the PaintThread* can die and be cleaned up before
> the nsIThread has been cleaned up?

Yup, that's exactly right. It's a little twisted, maybe the nsIThread should be a separate static pointer after all.
Created attachment 8879473 [details] [diff] [review]
part 1, move PaintThread shutdown v2

It looks like we might not actually need a PPaintBridge protocol, but I'd still like to do this shutdown refactoring. This version decouples the nsIThread so the shutdown task doesn't look as weird.
Attachment #8878809 - Attachment is obsolete: true
Attachment #8878809 - Flags: review?(mchang)
Attachment #8879473 - Flags: review?(mchang)
Attachment #8878810 - Flags: review?(wmccloskey)
Comment on attachment 8879473 [details] [diff] [review]
part 1, move PaintThread shutdown v2

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

::: gfx/layers/PaintThread.cpp
@@ +47,5 @@
>  }
>  
> +void
> +DestroyPaintThread(UniquePtr<PaintThread>&& pt)
> +{

nit: Assert which thread

@@ +55,3 @@
>  /* static */ void
>  PaintThread::Shutdown()
>  {

nit: ASsert which thread this executes on.
Attachment #8879473 - Flags: review?(mchang) → review+
This is getting subsumed by 1377060. I'll move part 1 to that bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.