Support content processes pushing layer transactions directly to omtc in parent process

RESOLVED FIXED in mozilla17

Status

()

Core
Widget
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(12 attachments, 7 obsolete attachments)

19.03 KB, patch
Details | Diff | Splinter Review
1.25 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
2.42 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
11.62 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
7.60 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
8.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.13 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
28.76 KB, patch
cjones
: review+
Details | Diff | Splinter Review
222.73 KB, patch
Details | Diff | Splinter Review
45.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.90 KB, patch
BenWa
: review+
cjones
: superreview+
Details | Diff | Splinter Review
32.47 KB, patch
BenWa
: review+
cjones
: review+
Details | Diff | Splinter Review
In b2g, we'll have a set of content processes that want to push their updates directly to the off-main-thread compositor in the top-most parent (server) process.

Basically, we need to hand the widget code an "opens" or "bridge" channel to the compositor's thread context.
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Depends on: 761962
Assignee: nobody → jones.chris.g
Depends on: 763234
Depends on: 763238
Should we extend CompositorFrameParent to support this? We don't want to duplicate that code.
There's no such animal.  My WIP is http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/6e11a9641634/745148-directcompositor if you want to get a sense of what things look like.
Created attachment 636647 [details] [diff] [review]
first version
Created attachment 640912 [details] [diff] [review]
Core of the logic being added here

The real patch is of course much larger than this, but it's all boilerplate to get here.

I suspect adding a RefLayer type will be contentious, hence feedback request.  Alternate suggestions welcome.
Attachment #636647 - Attachment is obsolete: true
Attachment #640912 - Flags: feedback?(roc)
Please ignore the |sCurrent| stuff in CompositorParent, it's a temporary hack.
Comment on attachment 640912 [details] [diff] [review]
Core of the logic being added here

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

This seems reasonable assuming we can later extend RefLayer to refer to layer subtrees in the same layer manager, perhaps by adding a way to attach referent IDs to individual layers.
Attachment #640912 - Flags: feedback?(roc) → feedback+
Created attachment 641193 [details] [diff] [review]
Rebased
Created attachment 641666 [details] [diff] [review]
part 0: Log the PID with MOZ_IPC_MESSAGE_LOG logging

Trivial helper.
Attachment #641666 - Flags: review?(bent.mozilla)
Created attachment 641668 [details] [diff] [review]
part 1: SyncLaunch() content processes when we're using direct-to-omtc drawing

This really stinks, but blame win32 and its lack of fork().

The motivating problem is, when we're using PCompositor to send messages directly from the content main thread to the compositor thread in the parent process, we need to PCompositor::Open() *right* after launching the content process.  Before any PBrowsers are created.

But to open the new channel, we have to use ShareHandle() on windows to send the child end of the pipe, and this requires having the HANDLE for the new process.  And getting that HANDLE requires disk IO.  Sigh.

This is bad, but I don't see a way around it, unfortunately.  On the bright side though, we only do this when we're using off-main-thread compositing, which means we can hide any latency here with an async animation.
Attachment #641668 - Flags: review?(bent.mozilla)
I had one more patch for you, but looking back over it I'm not sure it's needed.  Will clean up and verify asap.
Created attachment 641754 [details] [diff] [review]
part 1: Allow sending messages on an Opening channel, since they'll be queued if we're really actually still awaiting connect

Oh yeah, I remember what the problem was now.  Sigh.

So here's the story.  Chapter 1.  With content processes and OMTC, the main process does PCompositor::Open() to the content process, sending it a compositor Channel.  Previously, when opening these kinds of Channels, we blocked until we received the other side's pid.  That turns out to be unnecessary for "Opens" channels and child-process channels --- we already have the PID.

Who cares, you're probably asking yourself.  Well, we PCompositor::Open() asynchronously from the parent main thread.  The main thread goes back to its business, let's say painting using the sync PLayers protocol.  Meanwhile, we initialize the CompositorParent on the compositor thread, blocking in AsyncChannel::Open() on the other-side pid to be returned by the content-process main thread.  And when the content process starts up, the first thing it does is initialize the crash reporter, through sync SendPCrashReporterConstructor.  Hm.

 parent main thread --blocked on--> compositor thread
 compositor thread --blocked on--> content process
 content process --blocked on--> parent main thread

Whups.  The reason compositor thread blocks on content process is just to find out its pid basically, even though it already has it.  So we break the cycle there.

This patch doesn't wait for OnConnected, since we can send/recv IPC just fine before.
Attachment #641668 - Attachment is obsolete: true
Attachment #641668 - Flags: review?(bent.mozilla)
Attachment #641754 - Flags: review?(bent.mozilla)
Created attachment 641756 [details] [diff] [review]
2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and hand them off to the *Channel.

Chapter 2.  After that fix, I noticed the content process would hang on startup sometimes.  The content main thread was blocked on the sync PCrashReporter ctor again.

After some poking, I found that the ctor message was arriving at the parent process and being dispatch to the Channel listener.  We were just never sending a reply, so the content process stayed hung.

Then this hypothesis came to me while eating pizza, and I added a MOZ_NOT_REACHED() to GeckoChildProcessHost::OnMessageReceived(), and it blew up.  So what was happening is that the sync ctor message was being dropped on the floor.

This patch queues up those too-early messages and then hands them off for delivery when we switch channel listeners.

This bug has been latent forever.  I think the combination of SyncLaunch() (waiting for OnConnected) and the sync PCrashReporter() ctor asap after Channel::Open exarcerbated the problem.  But, even without those this bug could still pop up.
Attachment #641756 - Flags: review?(bent.mozilla)
Created attachment 641757 [details] [diff] [review]
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing

Same patch as before.
Attachment #641757 - Flags: review?(bent.mozilla)
I'm going to reuse some machinery Nico built in bug 598868 for finding the right CompositorParent to trigger recomposition for on async updates.
Depends on: 598868
Created attachment 642046 [details] [diff] [review]
part 4: Remove duplicated code in Basic*ContainerLayer
Attachment #642046 - Flags: review?(roc)
Created attachment 642047 [details] [diff] [review]
part 5: Pass the layer tree to ShadowLayersUpdate()
Attachment #642047 - Flags: review?(ajuma)
Created attachment 642049 [details] [diff] [review]
part 6: Allow layer trees to be given IDs so that the referent can be used in another context

This won't compile everywhere but I'll mop up those trivial compilation fixes in parallel.
Attachment #642049 - Flags: superreview?(roc)
Attachment #642049 - Flags: review?
Created attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition
Attachment #642050 - Flags: superreview?(roc)
Attachment #642050 - Flags: review?(bgirard)
Created attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

Not really a good way to split this up more, but it's mostly boilerplate.  The CompositorParent hunks are most interesting.  Please let me know if you don't comfortable reviewing the IPC bits.
Attachment #642053 - Flags: review?(bgirard)
Attachment #642053 - Flags: review?(ajuma)
Created attachment 642054 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor
Attachment #642054 - Flags: review?(roc)
Created attachment 642059 [details] [diff] [review]
Rollup, based on m-c rev 1dbe06319739

Includes dependencies.
Attachment #641193 - Attachment is obsolete: true

Updated

5 years ago
Attachment #642047 - Flags: review?(ajuma) → review+
Comment on attachment 642046 [details] [diff] [review]
part 4: Remove duplicated code in Basic*ContainerLayer

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

Nice.
Attachment #642046 - Flags: review?(roc) → review+
Attachment #641666 - Flags: review?(bent.mozilla) → review+
Attachment #641754 - Flags: review?(bent.mozilla) → review+
Comment on attachment 641756 [details] [diff] [review]
2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and hand them off to the *Channel.

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

::: ipc/glue/AsyncChannel.cpp
@@ +715,5 @@
> +
> +    // Dispatch whatever messages the previous listener had queued up.
> +    while (!pending.empty()) {
> +        OnMessageReceived(pending.front());
> +        pending.pop();

Should you pop before calling OnMessageReceived in case it reenters somehow? Something like:

  Message* msg = pending.front();
  pending.pop();
  OnMessageReceived(msg);

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +692,4 @@
>  void
>  GeckoChildProcessHost::OnChannelError()
>  {
>    // XXXbent Notify that the child process is gone?

Should probably file a followup since this can happen too, right?

::: ipc/glue/GeckoChildProcessHost.h
@@ +106,5 @@
>                                    base::ProcessArchitecture arch);
> +
> +  // There's a short window in between we launch the subprocess and we
> +  // hand the channel off that messages can be received by this, while
> +  // it's still the channel listener.  If so, the messages would

Nit: This comment is all sorts of grammatical nightmare. Must have been late ;) Let's clean it up a little.
Attachment #641756 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] from comment #23)
> Comment on attachment 641756 [details] [diff] [review]
> 2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and
> hand them off to the *Channel.
> 
> Review of attachment 641756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/AsyncChannel.cpp
> @@ +715,5 @@
> > +
> > +    // Dispatch whatever messages the previous listener had queued up.
> > +    while (!pending.empty()) {
> > +        OnMessageReceived(pending.front());
> > +        pending.pop();
> 
> Should you pop before calling OnMessageReceived in case it reenters somehow?

There's no possibility of reentry, but also no reason to use a nonstandard idiom.  Will change.

> >  {
> >    // XXXbent Notify that the child process is gone?
> 
> Should probably file a followup since this can happen too, right?
> 

Sure.  Our wild youth is catching up with us ;).

> ::: ipc/glue/GeckoChildProcessHost.h
> @@ +106,5 @@
> >                                    base::ProcessArchitecture arch);
> > +
> > +  // There's a short window in between we launch the subprocess and we
> > +  // hand the channel off that messages can be received by this, while
> > +  // it's still the channel listener.  If so, the messages would
> 
> Nit: This comment is all sorts of grammatical nightmare. Must have been late
> ;) Let's clean it up a little.

It was both late and a nightmare.
Comment on attachment 641757 [details] [diff] [review]
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing

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

::: dom/ipc/ContentParent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#if defined(ANDROID) || defined(XP_UNIX)
> +# include <sys/time.h>
> +# include <sys/resource.h>

Aha! You're the one who does this crazy indent thing. Ordinarily I'd balk, but you did alphabetize everything... Tough choice.
Attachment #641757 - Flags: review?(bent.mozilla) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> (In reply to ben turner [:bent] from comment #23)
> > Comment on attachment 641756 [details] [diff] [review]
> > 2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and
> > hand them off to the *Channel.
> > 
> > Review of attachment 641756 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: ipc/glue/AsyncChannel.cpp
> > @@ +715,5 @@
> > > +
> > > +    // Dispatch whatever messages the previous listener had queued up.
> > > +    while (!pending.empty()) {
> > > +        OnMessageReceived(pending.front());
> > > +        pending.pop();
> > 
> > Should you pop before calling OnMessageReceived in case it reenters somehow?
> 
> There's no possibility of reentry, but also no reason to use a nonstandard
> idiom.  Will change.
> 

Oh, now I remember what this was doing.  It's queue<Message> (not Message*), so the impl in the patch avoids a copy when calling OnMessageReceived.  So I didn't change this.
Comment on attachment 642049 [details] [diff] [review]
part 6: Allow layer trees to be given IDs so that the referent can be used in another context

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

This could also be considered an r+ if that helps.
Attachment #642049 - Flags: superreview?(roc) → superreview+
Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

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

::: gfx/layers/Layers.h
@@ +1431,5 @@
> +  virtual void InsertAfter(Layer* aChild, Layer* aAfter)
> +  { MOZ_NOT_REACHED("no"); }
> +
> +  virtual void RemoveChild(Layer* aChild)
> +  { MOZ_NOT_REACHED("no"); }

It would be cleaner to not expose RefLayer's ContainerLayer-ness. Private inheritance might work but we don't often use that. This will be much easier to address once layers are split into content-side and compositor-side objects, so let's not worry about it for now.

@@ +1442,5 @@
> +   * Set the ID of the layer's referent.
> +   */
> +  void SetReferentId(int64_t aId)
> +  {
> +    mId = aId;

Assert that aId is >= 0?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
>  static Thread* sCompositorThread = nsnull;
>  
> +/**
> + */
> +static Layer* GetIndirectShadowTree(int64_t aId);

Remove non-comment
Attachment #642050 - Flags: superreview?(roc) → superreview+
Comment on attachment 642054 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor

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

I don't understand why we need mIndirectLayerManager. It seems a bit vestigial. Why can't we use some other means to ensure RenderFrameParent creates a RefLayer?

::: layout/ipc/RenderFrameParent.cpp
@@ +552,5 @@
> +
> +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> +    if (!layer) {
> +      // Probably a temporary layer manager that doesn't know how to
> +      // use ref layers.

Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 642049 [details] [diff] [review]
> part 6: Allow layer trees to be given IDs so that the referent can be used
> in another context
> 
> Review of attachment 642049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This could also be considered an r+ if that helps.

Sure!  Thanks.
Attachment #642049 - Flags: superreview+
Attachment #642049 - Flags: review?
Attachment #642049 - Flags: review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 642054 [details] [diff] [review]
> part 9: Hook up the pieces and enable direct compositor
> 
> Review of attachment 642054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why we need mIndirectLayerManager. It seems a bit
> vestigial. Why can't we use some other means to ensure RenderFrameParent
> creates a RefLayer?
> 

We don't absolutely need it.  I'll reorganize things so that RenderFrameParent doesn't use that layer manager anymore.

> ::: layout/ipc/RenderFrameParent.cpp
> @@ +552,5 @@
> > +
> > +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > +    if (!layer) {
> > +      // Probably a temporary layer manager that doesn't know how to
> > +      // use ref layers.
> 
> Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?

We do.
Pushed 0-2 since those are more generic things we might step on

https://hg.mozilla.org/integration/mozilla-inbound/rev/d84d1d760483
https://hg.mozilla.org/integration/mozilla-inbound/rev/453481dbb828
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f19e4f90e7

Please leave this bug open.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > Comment on attachment 642054 [details] [diff] [review]
> > part 9: Hook up the pieces and enable direct compositor
> > 
> > Review of attachment 642054 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't understand why we need mIndirectLayerManager. It seems a bit
> > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > creates a RefLayer?
> > 
> 
> We don't absolutely need it.  I'll reorganize things so that
> RenderFrameParent doesn't use that layer manager anymore.
> 

Hmm ... I started to change this but it's actually somewhat nontrivial.  How strongly do you feel about changing this for this bug?
https://hg.mozilla.org/mozilla-central/rev/d84d1d760483
https://hg.mozilla.org/mozilla-central/rev/453481dbb828
https://hg.mozilla.org/mozilla-central/rev/d3f19e4f90e7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
See comment 32.  Thanks for merging!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > > Comment on attachment 642054 [details] [diff] [review]
> > > part 9: Hook up the pieces and enable direct compositor
> > > 
> > > Review of attachment 642054 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I don't understand why we need mIndirectLayerManager. It seems a bit
> > > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > > creates a RefLayer?
> > > 
> > 
> > We don't absolutely need it.  I'll reorganize things so that
> > RenderFrameParent doesn't use that layer manager anymore.
> > 
> 
> Hmm ... I started to change this but it's actually somewhat nontrivial.  How
> strongly do you feel about changing this for this bug?

Never mind again, I have to do pretty much the same thing for bug 750977, so might as well knock it out here.
Created attachment 642346 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor, v2
Attachment #642054 - Attachment is obsolete: true
Attachment #642054 - Flags: review?(roc)
Attachment #642346 - Flags: review?(roc)
With latest inbound and gaia, I'm seeing a ColorLayer being set as the root of the layer tree a content process.  That breaks a lot of things.
This doesn't happen on a desktop-b2g build.  Very disturbing.
We're hitting

#1  0x406d52c2 in PresShell::Paint (this=0x2187370, aViewToPaint=0x216e5f0, aWidgetToPaint=0x1f1d168, aDirtyRegion=..., aIntDirtyRegion=..., aWillSendDidPaint=true) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:5293

  nsRefPtr<ColorLayer> root = layerManager->CreateColorLayer();
  if (root) {
    nsPresContext* pc = GetPresContext();
    nsIntRect bounds =
      pc->GetVisibleArea().ToOutsidePixels(pc->AppUnitsPerDevPixel());
    bgcolor = NS_ComposeColors(bgcolor, mCanvasBackgroundColor);
    root->SetColor(bgcolor);
    root->SetVisibleRegion(bounds);
    layerManager->SetRoot(root);

I was under the impression that the root layer must be a container layer, but that's not mentioned anywhere in Layers.h.  OK.
Folded this patch

   void TemporarilyCompensateForContentScrollOffset(Layer* aContainer,
                                                    Layer* aShadowContent)
   {
     ContainerLayer* c = aShadowContent->AsContainerLayer();
+    if (!c) {
+      return;
+    }

into part 8.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> I was under the impression that the root layer must be a container layer,
> but that's not mentioned anywhere in Layers.h.  OK.

Right, sometimes we paint before we've constructed a root frame.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > ::: layout/ipc/RenderFrameParent.cpp
> > @@ +552,5 @@
> > > +
> > > +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > > +    if (!layer) {
> > > +      // Probably a temporary layer manager that doesn't know how to
> > > +      // use ref layers.
> > 
> > Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
> 
> We do.

Under what conditions?
At least for -moz-element.
Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

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

r- since I'd like to see the clarifications below before it goes in even if they are minor.

::: gfx/layers/Layers.h
@@ +1421,5 @@
> + * Composition:
> + *   look up subtree for GetReferentId()
> + *   ConnectReferentLayer(subtree)
> + *   compose
> + *   ClearReferentLayer()

It's not obvious to me why you want to connect/clear the referent layer for every composite. I think it's worth documenting that here.

@@ +1442,5 @@
> +   * Set the ID of the layer's referent.
> +   */
> +  void SetReferentId(int64_t aId)
> +  {
> +    mId = aId;

Instead of asserting that mId is >= 0 shouldn't we just use uint?

@@ +1462,5 @@
> +  /**
> +   * DRAWING PHASE ONLY
> +   * |aLayer| is the same as the argument to ConnectReferentLayer().
> +   */
> +  void ClearReferentLayer(Layer* aLayer)

Maybe we should rename this to disconnect/detach to better reflect that we're not clearing the referent layer but rather just detaching.

@@ +1491,5 @@
> +
> +  virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> +
> +  Layer* mTempReferent;
> +  int64_t mId;

Should we document that '0' is a special value? If for nothing else it's useful for asserting that mId has been set without introducing a debug only flag.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +294,5 @@
> +/**
> + * DRAWING PHASE ONLY
> + *
> + * For reach RefLayer in |aRoot|, look up its referent and connect it
> + * to the layer tree, if found.  On exiting scope, clears all resolved

clear=disconnect?

@@ +313,5 @@
> +
> +private:
> +  enum Op { Resolve, Clear };
> +  template<Op OP>
> +  void WalkTheTree(Layer* aLayer, Layer* aParent)

Why is this a template instead of making Op a parameter to WalkTheTree? As I understand it this will create two instance of this function.

@@ +332,5 @@
> +  }
> +
> +  Layer* mRoot;
> +
> +  AutoResolveRefLayers(const AutoResolveRefLayers&);

MOZ_DELETE

::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ +265,5 @@
>            specific.get_CanvasLayerAttributes().filter());
>          break;
>  
> +      case Specific::TRefLayerAttributes:
> +        MOZ_LAYERS_LOG(("[ParentSide]   canvas layer"));

s/canvas/reference
Attachment #642050 - Flags: review?(bgirard) → review-

Comment 45

5 years ago
Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

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

r+ with followups filed for the three "FIXME/bug XXXXX" comments.
Attachment #642053 - Flags: review?(ajuma) → review+
(In reply to Benoit Girard (:BenWa) from comment #44)

> ::: gfx/layers/Layers.h
> @@ +1421,5 @@
> > + * Composition:
> > + *   look up subtree for GetReferentId()
> > + *   ConnectReferentLayer(subtree)
> > + *   compose
> > + *   ClearReferentLayer()
> 
> It's not obvious to me why you want to connect/clear the referent layer for
> every composite. I think it's worth documenting that here.
> 

 * Clients will usually want to Connect/Clear() on each transaction to
 * avoid difficulties managing memory across multiple layer subtrees.

> @@ +1442,5 @@
> > +   * Set the ID of the layer's referent.
> > +   */
> > +  void SetReferentId(int64_t aId)
> > +  {
> > +    mId = aId;
> 
> Instead of asserting that mId is >= 0 shouldn't we just use uint?
> 

It was supposed to be uint64_t, this is a typo.  But we want to assert that it's != 0, i.e. a valid ID.  Having a RefLayer with the "null ID" is meaningless.

> @@ +1462,5 @@
> > +  /**
> > +   * DRAWING PHASE ONLY
> > +   * |aLayer| is the same as the argument to ConnectReferentLayer().
> > +   */
> > +  void ClearReferentLayer(Layer* aLayer)
> 
> Maybe we should rename this to disconnect/detach to better reflect that
> we're not clearing the referent layer but rather just detaching.
> 

Disconnect() is quite overloaded in gfx/layers.  Switched to Detach*() isn't.  

> @@ +1491,5 @@
> > +
> > +  virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> > +
> > +  Layer* mTempReferent;
> > +  int64_t mId;
> 
> Should we document that '0' is a special value? If for nothing else it's
> useful for asserting that mId has been set without introducing a debug only
> flag.
> 

  // 0 is a special value that means "no ID".

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +294,5 @@
> > +/**
> > + * DRAWING PHASE ONLY
> > + *
> > + * For reach RefLayer in |aRoot|, look up its referent and connect it
> > + * to the layer tree, if found.  On exiting scope, clears all resolved
> 
> clear=disconnect?
> 

s/clear/detach/

> @@ +313,5 @@
> > +
> > +private:
> > +  enum Op { Resolve, Clear };
> > +  template<Op OP>
> > +  void WalkTheTree(Layer* aLayer, Layer* aParent)
> 
> Why is this a template instead of making Op a parameter to WalkTheTree? As I
> understand it this will create two instance of this function.
> 

That's what we want.  Compilers are highly optimized to boil away constant parameters of templates.

> @@ +332,5 @@
> > +  }
> > +
> > +  Layer* mRoot;
> > +
> > +  AutoResolveRefLayers(const AutoResolveRefLayers&);
> 
> MOZ_DELETE
> 

Fixed.

> ::: gfx/layers/ipc/ShadowLayersParent.cpp
> @@ +265,5 @@
> >            specific.get_CanvasLayerAttributes().filter());
> >          break;
> >  
> > +      case Specific::TRefLayerAttributes:
> > +        MOZ_LAYERS_LOG(("[ParentSide]   canvas layer"));
> 
> s/canvas/reference

Fixed.
Created attachment 642662 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition, v2

Addressed comments, carrying over sr=roc.
Attachment #642050 - Attachment is obsolete: true
Attachment #642662 - Flags: superreview+
Attachment #642662 - Flags: review?(bgirard)
Blocks: 774386
Blocks: 774388
(In reply to Ali Juma [:ajuma] from comment #45)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
> 
> Review of attachment 642053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with followups filed for the three "FIXME/bug XXXXX" comments.

Thanks for the catch!  This slipped off my to-do list.
Created attachment 642666 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v2

Carrying over r=ajuma.
Attachment #642053 - Attachment is obsolete: true
Attachment #642053 - Flags: review?(bgirard)
Attachment #642666 - Flags: review?(bgirard)
Attachment #642666 - Flags: review+
Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

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

Posting my review draft I wrote before the patch was updated.

::: gfx/layers/ipc/CompositorChild.cpp
@@ +44,5 @@
> +{
> +  // There's only one compositor per child process.
> +  MOZ_ASSERT(!sCompositor);
> +
> +  nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));

Not a fan of variable shorthand.

@@ +57,5 @@
> +    NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> +    return nsnull;
> +  }
> +  // We release this ref in ActorDestroy().
> +  return sCompositor = cc.forget().get();

Why not make sCompositor a RefPtr?

@@ +78,5 @@
> +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  MOZ_ASSERT(sCompositor == this);
> +  sCompositor = NULL;
> +  MessageLoop::current()->PostTask(

Comment to explain why we're posting this as a task would be nice. Is this accurate/
// Release doesn't need to happen synchronously, postpone it.

::: gfx/layers/ipc/CompositorChild.h
@@ +43,4 @@
>  private:
>    nsRefPtr<LayerManager> mLayerManager;
>  
> +  // When we're in a child process, this is the process-global

What about when we're not in a child process? Maybe we can add a getter and assert if this is only valid in the child process?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +// FIXME/bug XXXXXX: There can only be one.  No wait, can be *more*
> +// than one.  Fix this.

If you plan on landing this as-is you should clarify the comment.
Attachment #642053 - Flags: review+

Updated

5 years ago
Attachment #642662 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #50)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
> 
> ::: gfx/layers/ipc/CompositorChild.cpp
> @@ +44,5 @@
> > +{
> > +  // There's only one compositor per child process.
> > +  MOZ_ASSERT(!sCompositor);
> > +
> > +  nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));
> 
> Not a fan of variable shorthand.
> 

s/cc/child/

> @@ +57,5 @@
> > +    NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> > +    return nsnull;
> > +  }
> > +  // We release this ref in ActorDestroy().
> > +  return sCompositor = cc.forget().get();
> 
> Why not make sCompositor a RefPtr?
> 

It creates static ctors which slow down startup.  We need jlebar's StaticRefPtr to fix this properly.

> @@ +78,5 @@
> > +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  MOZ_ASSERT(sCompositor == this);
> > +  sCompositor = NULL;
> > +  MessageLoop::current()->PostTask(
> 
> Comment to explain why we're posting this as a task would be nice. Is this
> accurate/
> // Release doesn't need to happen synchronously, postpone it.
> 

  // We don't want to release the ref to sCompositor here, during
  // cleanup, because that will cause it to be deleted while it's
  // still being used.  So defer the deletion to after it's not in
  // use.

> ::: gfx/layers/ipc/CompositorChild.h
> @@ +43,4 @@
> >  private:
> >    nsRefPtr<LayerManager> mLayerManager;
> >  
> > +  // When we're in a child process, this is the process-global
> 
> What about when we're not in a child process? Maybe we can add a getter and
> assert if this is only valid in the child process?
> 

I don't feel very strongly about this, but went ahead and moved the getter into the .cpp and added an assert.

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> >  namespace mozilla {
> >  namespace layers {
> >  
> > +// FIXME/bug XXXXXX: There can only be one.  No wait, can be *more*
> > +// than one.  Fix this.
> 
> If you plan on landing this as-is you should clarify the comment.

Yeah, it was early in the morning

// FIXME/bug 774386: we're assuming that there's only one
// CompositorParent, but that's not always true.  This assumption only
// affects CrossProcessCompositorParent below.
Attachment #642346 - Flags: review?(roc) → review+
Created attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3
Attachment #642666 - Attachment is obsolete: true
Attachment #642666 - Flags: review?(bgirard)
Attachment #642682 - Flags: review?(bgirard)
Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

Carrying over r=ajuma.
Attachment #642682 - Flags: review+
Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +736,5 @@
> +  static uint64_t ids;
> +  return ++ids;
> +}
> +
> +/** */

Missing a comment here. Maybe how this class differs from PCompositorParent.

@@ +814,2 @@
>  {
> +  LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);

Quite a few variable acronym in this page. I have no idea what the 'c' stands for here, let's use something more obvious.
Attachment #642682 - Flags: review?(bgirard) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Comment on attachment 642050 [details] [diff] [review]
> part 7: Create a RefLayer type to temporarily contain a foreign layer
> subtree during composition
> 
> @@ +1442,5 @@
> > +   * Set the ID of the layer's referent.
> > +   */
> > +  void SetReferentId(int64_t aId)
> > +  {
> > +    mId = aId;
> 
> Assert that aId is >= 0?
> 

Done, --> uint64_t and asserted != 0.

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> >  static Thread* sCompositorThread = nsnull;
> >  
> > +/**
> > + */
> > +static Layer* GetIndirectShadowTree(int64_t aId);
> 
> Remove non-comment

I meant to document this ... added

/**
 * Lookup the indirect shadow tree for |aId| and return it if it
 * exists.  Otherwise null is returned.  This must only be called on
 * the compositor thread.
 */
(In reply to Benoit Girard (:BenWa) from comment #54)
> Comment on attachment 642682 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor, v3
> 
> Review of attachment 642682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +736,5 @@
> > +  static uint64_t ids;
> > +  return ++ids;
> > +}
> > +
> > +/** */
> 
> Missing a comment here. Maybe how this class differs from PCompositorParent.
> 

/**
 * This class handles layer updates pushed directly from child
 * processes to the compositor thread.  It's associated with a
 * CompositorParent on the compositor thread.  While it uses the
 * PCompositor protocol to manage these updates, it doesn't actually
 * drive compositing itself.  For that it hands off work to the
 * CompositorParent it's associated with.
 */

> @@ +814,2 @@
> >  {
> > +  LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);
> 
> Quite a few variable acronym in this page. I have no idea what the 'c'
> stands for here, let's use something more obvious.

In this case here, "cit" is an STL idiom that's shorthand for "Const ITerator".
Kinda cute: every single try build failed on https://tbpl.mozilla.org/?tree=Try&rev=83b060905e12, but all of my local {mac,android,b2g,linux} builds succeed.  (Even with clobbers.) :|
And the rest

https://hg.mozilla.org/integration/mozilla-inbound/rev/71300529a740
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed87a3a43a6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/13059586b02b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df6cd42330d
https://hg.mozilla.org/integration/mozilla-inbound/rev/100fd0a81f9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeafc64692a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5808967288
https://hg.mozilla.org/mozilla-central/rev/71300529a740
https://hg.mozilla.org/mozilla-central/rev/ed87a3a43a6c
https://hg.mozilla.org/mozilla-central/rev/13059586b02b
https://hg.mozilla.org/mozilla-central/rev/1df6cd42330d
https://hg.mozilla.org/mozilla-central/rev/100fd0a81f9e
https://hg.mozilla.org/mozilla-central/rev/aeafc64692a5
https://hg.mozilla.org/mozilla-central/rev/4a5808967288
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla16 → mozilla17
Depends on: 776217

Updated

4 years ago
Depends on: 930838
You need to log in before you can comment on or make changes to this bug.