711168 - Implement the compositor protocol for OMTC

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

The Compositor protocol will host PLayers.

Comment 1

[Ali Juma [:ajuma]]

Attachment: Implemention of OMTC for OS X

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 583248 [details] [diff] [review]
Implemention of OMTC for OS X

All the changes are behind a pref.

Comment 3

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

Very exciting :).  Congratulations, guys!

I'm going to start reviewing this patch today, but just to warn you, it may take me a bit of time.

Comment 4

[Ali Juma [:ajuma]]

One thing that will need fixing is CompositorChild::Destroy. We need to make sure that ShadowLayersChild owned by the CompositorChild gets destroyed, and that this happens in the right order wrt the destruction of the BasicLayerManager.

Comment 5

[Ali Juma [:ajuma]]

Attachment: Implementation of OMTC for OS X, v2

Patch updated to fix up the destruction issues from Comment 4.

Comment 6

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

Comment on attachment 583816 [details] [diff] [review]
Implementation of OMTC for OS X, v2

With the current drawing path, IIUC, we're going to pay an extra copy
on the upload in ShadowBufferOGL::Upload.  If we can lock a texture to
system memory (the dirty region) in the "compositor child", draw to
that, and then just swap it out during the PLayers:Update, I think we
would be better off.  Maybe we can use IOSurfaces for that?  Something
to optimize in a followup bug.

These patches bake in the assumption that the compositor parent uses a
GL layer manager.  I think support for Basic layer managers could have
been added here pretty naturally, but I'm fine with that being added
either in v2 or a followup bug.

>diff --git a/gfx/layers/ipc/Compositor.cpp b/gfx/layers/ipc/Compositor.cpp
>diff --git a/gfx/layers/ipc/Compositor.h b/gfx/layers/ipc/Compositor.h

Since this is part of the mac-cross-thread-specific compositor setup,
I think they should be private to widget/src/cocoa.

>diff --git a/gfx/layers/ipc/CompositorChild.cpp b/gfx/layers/ipc/CompositorChild.cpp

>+CompositorChild*
>+CompositorChild::CreateCompositor(LayerManager *aLayerManager,
>+                                  CompositorParent *aCompositorParent)
>+{
>+  Thread* compositorThread = new Thread("CompositorThread");
>+  if (compositorThread->Start()) {
>+    MessageLoop *parentMessageLoop = MessageLoop::current();
>+    MessageLoop *childMessageLoop = compositorThread->message_loop();
>+    CompositorChild *compositorChild = new CompositorChild(compositorThread, aLayerManager);
>+    mozilla::ipc::AsyncChannel *parentChannel =
>+      aCompositorParent->GetIPCChannel();
>+    mozilla::ipc::AsyncChannel *childChannel =
>+      compositorChild->GetIPCChannel();
>+    mozilla::ipc::AsyncChannel::Side childSide =
>+      mozilla::ipc::AsyncChannel::Child;
>+

Gak my eyes!  |using namespace mozilla::ipc;| plz ;).  (Wherever it's
moved.)

>+    compositorChild->Open(parentChannel, childMessageLoop, childSide);
>+    compositorChild->SendInit();
>+

I think this setup code belongs in widget, probably nsBaseWidget,
since the mac-cross-thread initialization will depend on it.

>diff --git a/gfx/layers/ipc/CompositorChild.h b/gfx/layers/ipc/CompositorChild.h

>+#ifndef mozilla_layers_CompositorChild_h
>+#define mozilla_layers_CompositorChild_h
>+
>+#include "mozilla/layers/PCompositorChild.h"
>+
>+namespace base {
>+  class Thread;
>+}
>+
>+using base::Thread;
>+

Nit: don't import types into the global namespace in headers.

But generally, I think CompositorChild should not be entangled with
the threading scaffolding we're setting it up on for mac-cross-thread
compositing.  Let's talk about refactoring that.  I'll try to include
a suggestion in this review comment when I finish going through
everything.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+void
>+CompositorParent::Destroy()

You shouldn't need this code.  The "child side" should initiate
shutdown in the cross-thread case, and there aren't any race
conditions we need to worry about, unlike in the cross-process case
where the parent side can sometimes initiate shutdown.  I would assert
here that the ManagedPLayersParent().Length() == 0.

>+bool
>+CompositorParent::RecvInit()
>+{
>+  CancelableTask *composeTask = NewRunnableMethod(this, &CompositorParent::Composite);
>+  MessageLoop::current()->PostTask(FROM_HERE, composeTask);

We can't start compositing until we have a PLayersParent, right?
What's this used for?

>+void
>+CompositorParent::Composite()
>+{
>+  if (mStopped) {
>+    return;
>+  }
>+
>+  if (!mLayerManager) {
>+    CancelableTask *composeTask = NewRunnableMethod(this, &CompositorParent::Composite);
>+    MessageLoop::current()->PostDelayedTask(FROM_HERE, composeTask, 10);
>+    return;

This looks pretty iffy to me.  Can't we just request a Composite()
when the layer manager is created, or remember that we need to
Composite()?

>+  }
>+
>+  mLayerManager->EndEmptyTransaction();
>+}
>+
>+PLayersParent*
>+CompositorParent::AllocPLayers(const LayersBackend &backend, const WidgetDescriptor &widget)
>+{
>+  if (widget.type() != WidgetDescriptor::TViewWidget) {
>+    NS_ERROR("Invalid widget descriptor\n");
>+    return NULL;
>+  }
>+
>+  if (backend == LayerManager::LAYERS_OPENGL) {
>+    nsIWidget *nsWidget = (nsIWidget*)widget.get_ViewWidget().widgetPtr();
>+    nsRefPtr<LayerManagerOGL> layerManager = new LayerManagerOGL(nsWidget);
>+
>+    if (!layerManager->Initialize()) {
>+      NS_ERROR("Failed to init OGL Layers");
>+      return NULL;
>+    }
>+
>+    ShadowLayerManager* slm = layerManager->AsShadowManager();
>+    if (!slm) {
>+      return NULL;
>+    }
>+
>+    void *glContext = layerManager->gl()->GetNativeData(mozilla::gl::GLContext::NativeGLContext);
>+    NativeContext nativeContext = NativeContext((uintptr_t)glContext);
>+    SendNativeContextCreated(nativeContext);
>+
>+    mLayerManager = layerManager;
>+
>+    return new ShadowLayersParent(slm, this);
>+  } else {
>+    NS_ERROR("Unsupported backend selected for Async Compositor");
>+    return NULL;
>+  }
>+}
>+
>+bool
>+CompositorParent::DeallocPLayers(PLayersParent* actor)
>+{
>+  delete actor;
>+  return true;
>+}
>+
>+} // namespace layers
>+} // namespace mozilla
>+

>diff --git a/gfx/layers/ipc/CompositorParent.h b/gfx/layers/ipc/CompositorParent.h

>+#ifndef mozilla_layers_CompositorParent_h
>+#define mozilla_layers_CompositorParent_h
>+
>+#include "mozilla/layers/PCompositorParent.h"
>+#include "mozilla/layers/PLayersParent.h"
>+#include "ShadowLayersHost.h"
>+
>+class LayerManager;
>+
>+namespace mozilla {
>+namespace layers {
>+
>+class CompositorParent : public PCompositorParent,
>+                         public ShadowLayersHost
>+{
>+  NS_INLINE_DECL_REFCOUNTING(CompositorParent)
>+public:
>+  CompositorParent();
>+  virtual ~CompositorParent();
>+
>+  bool RecvInit();
>+  bool RecvStop();
>+
>+  void RequestComposition();
>+

Nit: call this |ScheduleComposition()|, since it's a command not a
request :).  But see below, I think this can be made private to
CompositorParent.

>+  virtual mozilla::layout::RenderFrameParent* GetRenderFrameParent() { return NULL; }
>+  virtual CompositorParent* GetCompositorParent() { return this; }
>+
>+  void Destroy();
>+
>+protected:
>+  virtual PLayersParent* AllocPLayers(const LayersBackend &backend, const WidgetDescriptor &widget);
>+
>+  virtual bool DeallocPLayers(PLayersParent* aLayers);
>+
>+private:
>+  void Composite();
>+
>+  nsRefPtr<LayerManager> mLayerManager;
>+  bool mStopped;
>+
>+  DISALLOW_EVIL_CONSTRUCTORS(CompositorParent);
>+};
>+
>+} // layers
>+} // mozilla
>+
>+#endif // mozilla_layers_CompositorParent_h

>diff --git a/gfx/layers/ipc/PCompositor.ipdl b/gfx/layers/ipc/PCompositor.ipdl

>+include protocol PLayers;
>+
>+using mozilla::LayersBackend;
>+using mozilla::null_t;
>+
>+namespace mozilla {
>+namespace layers {
>+
>+// The Compositor needs to know about the widget to initialize the LayerManager.
>+struct ViewWidget {
>+  uintptr_t widgetPtr;
>+};
>+
>+// Gives platform specific information about the widget to initialize the LayerManager.
>+union WidgetDescriptor {
>+  ViewWidget;
>+  null_t;
>+};
>+
>+// Cocoa: The widget needs to have access to the native context for resizing
>+struct NativeContext {
>+  uintptr_t nativeContext;
>+};
>+

I would rather we not have any code in here that depends on running in
the same process.  If some of these items have cross-process
descriptors we can use, by all means let's use those.  But ISTR that
not being the case.  So for the out-of-band setup we need here for mac
cross-thread, let's make a mac-cross-thread specific C++ interface.  I
think that'll be simpler in the long run.

>+/**
>+ * The PCompositor protocol is used to manage communication between
>+ * the main thread and the compositor thread context. It's primary
>+ * purpose is to manage the PLayers sub protocol.
>+ */
>+rpc protocol PCompositor

This should be 'sync'.  You don't need 'rpc' semantics, and in general
should avoid 'rpc' if at all possible ;).

>+{
>+  // Compositor can manage many Layer Manager (PLayers)
>+  manages PLayers;
>+

Is that true?  Is it because of multiple firefox windows that might be
open on mac, so there's one layer manager per window?  But in that
case, is there one PCompositor per window, with one layer manager per
compositor?

If the PLayers here can be a "singleton", that would help, because it
will simplify assumptions about the implementation here, and we'll
probably add IPDL support for explicit singletons soon-ish, e.g.

  manages single PLayers;

which will simplify various parts of the generated C++ interface.

[Update to self: CompositorParent::Destroy() seems to suggest PLayers
is a singleton.]

>+parent:  
>+
>+  sync Init();
>+  sync Stop();
>+

I don't know what these mean from looking at them.  Please add
comments.

>+child:
>+
>+  // We create the LayerManager async thus we need to notify
>+  // content that we have a native context to attach to the widget.
>+  async NativeContextCreated(NativeContext aNativeContext);
>+

I think this belongs with the "back-door", mac-cross-thread setup in
C++.

>diff --git a/gfx/layers/ipc/ShadowLayersHost.h b/gfx/layers/ipc/ShadowLayersHost.h

>+class ShadowLayersHost
>+{

"Host" is a bit confusing to me here.  What about "ShadowLayersManager"?

>+NS_INLINE_DECL_REFCOUNTING(ShadowLayersHost)
>+

What's this used for?

>+public:
>+  virtual mozilla::layout::RenderFrameParent* GetRenderFrameParent() = 0;
>+  virtual CompositorParent* GetCompositorParent() = 0;

Instead of this mini-RTTI, how about making a 

  virtual void ShadowLayersUpdated() = 0;

and then just calling that, modifying RenderFrameParent to fit this
interface?

>diff --git a/gfx/layers/ipc/ShadowLayersParent.cpp b/gfx/layers/ipc/ShadowLayersParent.cpp

>-ShadowLayersParent::ShadowLayersParent(ShadowLayerManager* aManager)
>+ShadowLayersParent::ShadowLayersParent(ShadowLayerManager* aManager, ShadowLayersHost* aHost)
>   : mDestroyed(false)
> {
>   MOZ_COUNT_CTOR(ShadowLayersParent);
>   mLayerManager = aManager;
>+  mHost = aHost;

These should be set up by initializers along with |mDestroyed|, plz to
be fixing.  (Sorry, probably my fault with mLayerManager.)

> void
>@@ -214,16 +216,21 @@ ShadowLayersParent::RecvUpdate(const Inf
> 
>       const CommonLayerAttributes& common = attrs.common();
>       layer->SetVisibleRegion(common.visibleRegion());
>       layer->SetContentFlags(common.contentFlags());
>       layer->SetOpacity(common.opacity());
>       layer->SetClipRect(common.useClipRect() ? &common.clipRect() : NULL);
>       layer->SetTransform(common.transform());
>       layer->SetTileSourceRect(common.useTileSourceRect() ? &common.tileSourceRect() : NULL);
>+      if (mHost->GetCompositorParent()) {
>+        layer->AsShadowLayer()->SetShadowTransform(common.transform());
>+        layer->AsShadowLayer()->SetShadowVisibleRegion(common.visibleRegion());
>+        layer->AsShadowLayer()->SetShadowClipRect(layer->GetClipRect());
>+      }

We're going to want to refactor the code that processes shadow
attributes in RenderFrameParent, but for now how about doing this in
CompositorParent::ShadowLayersUpdated()?  If that looks gross, let's
chat about an alternate stopgap.

>+      if (mHost->GetCompositorParent()) {
>+        mLayerManager->SetRoot(mRoot);
>+      }

(Similarly here.)

>-  Frame()->ShadowLayersUpdated();
>+  if (Frame()) {
>+    Frame()->ShadowLayersUpdated();
>+  }
> 
>+  if (mHost->GetCompositorParent()) {
>+    mHost->GetCompositorParent()->RequestComposition();
>+  }

I propose calling mHost->ShadowLayersUpdated() here, and then in
CompositorParent you can schedule the composition.  That would also
keep the composition-scheduling internal to CompositorParent, so it
could be removed from the public API.

> RenderFrameParent*
> ShadowLayersParent::Frame()
> {
>-  return static_cast<RenderFrameParent*>(Manager());
>+  if (mDestroyed) {
>+    return NULL;
>+  } else {
>+    return mHost->GetRenderFrameParent();
>+  }
> }
> 

Frame() should be unnecessary, I think this entire method can be removed.

>diff --git a/widget/src/cocoa/nsChildView.h b/widget/src/cocoa/nsChildView.h
>diff --git a/widget/src/cocoa/nsChildView.mm b/widget/src/cocoa/nsChildView.mm

I'm not really qualified to review this code.

>diff --git a/widget/src/xpwidgets/nsBaseWidget.cpp b/widget/src/xpwidgets/nsBaseWidget.cpp

>   if (mLayerManager) {
>     mLayerManager->Destroy();
>+    mLayerManager = NULL;

Nit: local style is |nsnull| I think.

>@@ -819,26 +826,63 @@ LayerManager* nsBaseWidget::GetLayerMana
>                                             LayerManagerPersistence aPersistence,
>                                             bool* aAllowRetaining)
> {

>     if (mUseAcceleratedRendering) {

Per above, not a fan of baking GL layers into omtc here, but not a
blocker for this landing either.

>+      // Try to use an async compositor first, if possible
>+      bool useCompositor =
>+        Preferences::GetBool("layers.offmainthreadcomposition.enabled", false);
>+      if (useCompositor) {
>+        LayerManager* lm = CreateBasicLayerManager();
>+        mCompositorParent = new CompositorParent();
>+        mCompositor = CompositorChild::CreateCompositor(lm, mCompositorParent);
>+
>+        if (mCompositor) {
>+          // e10s uses the parameter to pass in the shadow manager from the TabChild
>+          // so we don't expect to see it there since this doesn't support e10s.
>+          NS_ASSERTION(aShadowManager == NULL, "Async Compositor not supported with e10s");
>+          WidgetDescriptor desc = ViewWidget((uintptr_t)dynamic_cast<nsIWidget*>(this));
>+          PLayersChild* shadowManager = mCompositor->SendPLayersConstructor(
>+                                          LayerManager::LAYERS_OPENGL,
>+                                          desc);
>+

Per above, I think that shoe-horning platform/cross-thread-specific
setup into PCompositor is pretty gross.  I think what we want here is
a generic bit of "create compositor thread, initialize layer manager"
that's done by nsBaseWidget but overrideable by platform-specific
backends.  So I'd propose moving the
CompositorChild::CreateCompositor() code out of CompositorChild and
into nsBaseWidget and have nsBaseWidget own the Thread.  Maybe use a
virtual CreateCompositor() method on nsBaseWidget, that the cocoa
backend calls, then after that method finishes the cocoa widget
backend does cocoa-specific setup?  Does that make sense?  Would that
fit with what you have here?

I'm quite surprised and happy with how small this patch ended up
being.  I'd like to see a v2 and then I think this is ready to land.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Comment on attachment 583816 [details] [diff] [review]
> Implementation of OMTC for OS X, v2
> 
> With the current drawing path, IIUC, we're going to pay an extra copy
> on the upload in ShadowBufferOGL::Upload.  If we can lock a texture to
> system memory (the dirty region) in the "compositor child", draw to
> that, and then just swap it out during the PLayers:Update, I think we
> would be better off.  Maybe we can use IOSurfaces for that?  Something
> to optimize in a followup bug.

I have a few ideas in mind. I was planning on taking this problem on at the same time as we make PLayers update async. Let's do this in a follow up bug.

> These patches bake in the assumption that the compositor parent uses a
> GL layer manager.  I think support for Basic layer managers could have
> been added here pretty naturally, but I'm fine with that being added
> either in v2 or a followup bug.

The problem with the Basic layer manager is we still need to make sure its thread safe. Since we're concerned about android I don't think we should focus on Basis. Let's get OGL working on android first and fall back to the java layer manager for now then add support for basic when OGL is working.

> 
> I would rather we not have any code in here that depends on running in
> the same process.  If some of these items have cross-process
> descriptors we can use, by all means let's use those.  But ISTR that
> not being the case. 

I don't think there is anything for the context/widget so we need to do this nasty hack :(. I think we should just have a strong convention for denoting what uses a 'same-process-hack' so that it's clear the reader.

> >+{
> >+  // Compositor can manage many Layer Manager (PLayers)
> >+  manages PLayers;
> >+
> 
> Is that true?  Is it because of multiple firefox windows that might be
> open on mac, so there's one layer manager per window?  But in that
> case, is there one PCompositor per window, with one layer manager per
> compositor?

No longer true. We originally planned to have only one compositor thread but we decided to have one compositor per layer manager. This shouldn't happen on mobile and will help desktop in supporting vsync.

> 
> If the PLayers here can be a "singleton", that would help, because it
> will simplify assumptions about the implementation here, and we'll
> probably add IPDL support for explicit singletons soon-ish, e.g.
> 
>   manages single PLayers;
> 
> which will simplify various parts of the generated C++ interface.
> 
> [Update to self: CompositorParent::Destroy() seems to suggest PLayers
> is a singleton.]

It can, are there any changes we can make now or should we file a follow up bug on IPDL changes?

> >+public:
> >+  virtual mozilla::layout::RenderFrameParent* GetRenderFrameParent() = 0;
> >+  virtual CompositorParent* GetCompositorParent() = 0;
> 
> Instead of this mini-RTTI, how about making a 
> 
>   virtual void ShadowLayersUpdated() = 0;
> 
> and then just calling that, modifying RenderFrameParent to fit this
> interface?

That's a great idea.

> >+      // Try to use an async compositor first, if possible
> >+      bool useCompositor =
> >+        Preferences::GetBool("layers.offmainthreadcomposition.enabled", false);
> >+      if (useCompositor) {
> >+        LayerManager* lm = CreateBasicLayerManager();
> >+        mCompositorParent = new CompositorParent();
> >+        mCompositor = CompositorChild::CreateCompositor(lm, mCompositorParent);
> >+
> >+        if (mCompositor) {
> >+          // e10s uses the parameter to pass in the shadow manager from the TabChild
> >+          // so we don't expect to see it there since this doesn't support e10s.
> >+          NS_ASSERTION(aShadowManager == NULL, "Async Compositor not supported with e10s");
> >+          WidgetDescriptor desc = ViewWidget((uintptr_t)dynamic_cast<nsIWidget*>(this));
> >+          PLayersChild* shadowManager = mCompositor->SendPLayersConstructor(
> >+                                          LayerManager::LAYERS_OPENGL,
> >+                                          desc);
> >+
> 
> Per above, I think that shoe-horning platform/cross-thread-specific
> setup into PCompositor is pretty gross.  I think what we want here is
> a generic bit of "create compositor thread, initialize layer manager"
> that's done by nsBaseWidget but overrideable by platform-specific
> backends.  So I'd propose moving the
> CompositorChild::CreateCompositor() code out of CompositorChild and
> into nsBaseWidget and have nsBaseWidget own the Thread.  Maybe use a
> virtual CreateCompositor() method on nsBaseWidget, that the cocoa
> backend calls, then after that method finishes the cocoa widget
> backend does cocoa-specific setup?  Does that make sense?  Would that
> fit with what you have here?

It is nasty, I'll refactor this in v2. I'll give your suggestion a shot.

> I'm quite surprised and happy with how small this patch ended up
> being.  I'd like to see a v2 and then I think this is ready to land.

Thanks, it was really easy with how well ShadowLayers was designed. If only it wasn't for the cross-thread hacks :(. 

All the comments I haven't responded to will be fixed in v2.

Comment 8

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

(In reply to Benoit Girard (:BenWa) from comment #7)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> > Comment on attachment 583816 [details] [diff] [review]
> > Implementation of OMTC for OS X, v2
> >
> > These patches bake in the assumption that the compositor parent uses a
> > GL layer manager.  I think support for Basic layer managers could have
> > been added here pretty naturally, but I'm fine with that being added
> > either in v2 or a followup bug.
> 
> The problem with the Basic layer manager is we still need to make sure its
> thread safe. Since we're concerned about android I don't think we should
> focus on Basis. Let's get OGL working on android first and fall back to the
> java layer manager for now then add support for basic when OGL is working.
> 

Do we have full GL layers support for all the GPUs we'll see on android devices?

Either way, basic layers support is fine for a followup. 

> > 
> > I would rather we not have any code in here that depends on running in
> > the same process.  If some of these items have cross-process
> > descriptors we can use, by all means let's use those.  But ISTR that
> > not being the case. 
> 
> I don't think there is anything for the context/widget so we need to do this
> nasty hack :(. I think we should just have a strong convention for denoting
> what uses a 'same-process-hack' so that it's clear the reader.
> 

I think this will be clearest if done in widget/src/cocoa, out of band, as I suggested below.  Let's see how that turns out.

> > 
> > If the PLayers here can be a "singleton", that would help, because it
> > will simplify assumptions about the implementation here, and we'll
> > probably add IPDL support for explicit singletons soon-ish, e.g.
> > 
> >   manages single PLayers;
> > 
> > which will simplify various parts of the generated C++ interface.
> > 
> > [Update to self: CompositorParent::Destroy() seems to suggest PLayers
> > is a singleton.]
> 
> It can, are there any changes we can make now or should we file a follow up
> bug on IPDL changes?
> 

For the purposes of this bug, just update the comment there.

Comment 9

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> >+/**
> >+ * The PCompositor protocol is used to manage communication between
> >+ * the main thread and the compositor thread context. It's primary
> >+ * purpose is to manage the PLayers sub protocol.
> >+ */
> >+rpc protocol PCompositor
> 
> This should be 'sync'.  You don't need 'rpc' semantics, and in general
> should avoid 'rpc' if at all possible ;).

Changing this to sync gives a compile-time error (even when none of the message declarations use rpc), since then PCompositorParent is of type |IProtocolManager<SyncChannle::SyncListener>| but mManager in PLayersParent is of type |IProtocolManager<RPCChannel::RPCListener>*|.

However, if we declare PCompositor as sync but also give it a manager declared as rpc, then PCompositorParent ends up with type |IProtocolManager<RPCChannel::RPCListener>|, as required by PLayersParent.

So it seems that the top-level protocol in a 'manages' hierarchy needs to be declared rpc.

Comment 10

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

Oh ok, I know what the issue is here.  It's an IPDL-compiler bug.  And tbh, I'm not sure of the best way to fix it :/.  Let's not block on that though: make this 'rpc' with a comment that it's a workaround, and file a followup on the compiler bug.

Comment 11

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> Oh ok, I know what the issue is here.  It's an IPDL-compiler bug.  And tbh,
> I'm not sure of the best way to fix it :/.  Let's not block on that though:
> make this 'rpc' with a comment that it's a workaround, and file a followup
> on the compiler bug.

Filed Bug 716631.

Comment 12

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

Thanks!

Comment 13

[Ali Juma [:ajuma]]

Attachment: Implementation of OMTC for OS X, v3

Patch updated to address review comments.

Some details:

gfx/layers/ipc/Compositor.{h, cpp} no longer contain any mac-specific code, but instead contain a workaround for the inability to include IPDL-generated headers within widget/src/cocoa. 

The mac-specific code is now in nsChildView::CreateCompositor in widget/src/cocoa/nsChildView.mm, which overrides nsBaseWidget::CreateCompositor.

In the PCompositor protocol, there is no longer an Init message; instead, the PLayers constructor is now sync. The lack of any async declarations causes us to run into Bug 717027, so an async FixMeDoNotCall message has been added as a workaround.

Comment 14

[Ali Juma [:ajuma]]

One remaining issue is that LayerManagerOGL::Initialize() (which happens on the compositor thread) uses ScopedGfxFeatureReporter, which ultimately ends up using nsObserverService. This means we fail assertions that nsObserverService is only used on the main thread. I'm looking into a workaround for this.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

This really sucks; ScopedGfxFeatureReporter only uses the nsObserverService to destroy some strings before the end of XPCOM shutdown in order to avoid that they'd get reported as leaked.

Comment 16

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

Is it thread safe to begin with?

Comment 17

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> Is it thread safe to begin with?

I believe so -- the main thread is blocked during LayerManagerOGL::Initialize(), and the ScopedGfxFeatureReporter is created and destroyed during this call.

We're also getting another set of assertions related to the use of Preferences::AddBoolVarCache in LayerManagerOGL::Initialize; this is being used to make the FPS counter pref changes take effect without a restart. This code is not thread safe, and the obvious solution is to simply get rid of this code and require a restart to change the FPS counter pref.

Comment 18

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

(In reply to Ali Juma [:ajuma] from comment #17)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > Is it thread safe to begin with?
> 
> I believe so -- the main thread is blocked during
> LayerManagerOGL::Initialize(), and the ScopedGfxFeatureReporter is created
> and destroyed during this call.
> 

Relying on the compositor being initialized synchronously for thread safety makes me nervous.  That might not always be the case.

But AnnotateCrashReport() is indeed thread safe (in chrome processes).

> We're also getting another set of assertions related to the use of
> Preferences::AddBoolVarCache in LayerManagerOGL::Initialize; this is being
> used to make the FPS counter pref changes take effect without a restart.
> This code is not thread safe, and the obvious solution is to simply get rid
> of this code and require a restart to change the FPS counter pref.

Sigh :/.  We could also listen to those pref changes elsewhere and notify the compositor with a message.

Comment 19

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

Comment on attachment 587713 [details] [diff] [review]
Implementation of OMTC for OS X, v3

>diff --git a/gfx/layers/ipc/Compositor.h b/gfx/layers/ipc/Compositor.h

>+// Needed when we cannot directly include CompositorParent.h since it includes
>+// an IPDL-generated header (e.g. IPDL-generated headers cannot be included in
>+// widget/src/cocoa).

Why not?  Is there a bug on file for that?

Given that, let's call this file CompositorCocoaWidgetHelper or
something like that to avoid confusing this with real
implementation code.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+// Go down shadow layer tree, setting properties to match their non-shadow
>+// counterparts.
>+static void
>+SetShadowProperties(Layer* aLayer)
>+{
>+  ShadowLayer* shadow = aLayer->AsShadowLayer();
>+  shadow->SetShadowTransform(aLayer->GetTransform());
>+  shadow->SetShadowVisibleRegion(aLayer->GetVisibleRegion());
>+  shadow->SetShadowClipRect(aLayer->GetClipRect());
>+
>+  for (Layer* child = aLayer->GetFirstChild();
>+      child; child = child->GetNextSibling()) {
>+    SetShadowProperties(child);
>+  }

For complicated layer trees, this will be quite suboptimal
because we'll be essentially copying the layer tree on each
update.  We should make ShadowLayersParent::RecvUpdate default to
updating shadow properties for changed layers (it has that
information), and then put the burden on the code in
RenderFrameParent to only recompute transforms when needed.

But let's do that in a followup.  If you could file that and add
a "FIXME" here, that'd be great.

>diff --git a/gfx/layers/ipc/CompositorParent.h b/gfx/layers/ipc/CompositorParent.h

>+  bool RecvStop();
>+

 virtual bool RecvStop() MOZ_OVERRIDE;

>+  // Workaround for Bug 717027.
>+  bool RecvFixMeDoNotCall();
>+

  virtual bool RecvFixMeDoNotCall() MOZ_OVERRIDE;

(Yeah yeah I know.  But good habit to get into ;).

>+  virtual void ShadowLayersUpdated();

  virtual void ShadowLayersUpdated() MOZ_OVERRIDE;

>+  nsIWidget* mWidget;

Mmmm I see we need mWidget to initialize the layer manager, but
it's not used otherwise.  Holding onto a naked pointer here makes
me nervous, but I don't have any better ideas for the
initialization sequence right now.  Let's at least have
|CompositorParent::AllocPLayers| null this out after initializing
the layer manager.

Another issue for a followup: these methods in LayerManagerOGL
touch its held widget pointer

  mWidget->GetClientBounds(rect);
  mWidget->DrawOver(this, rect);
  mWidget->GetBounds(rect);

GetClientBounds and GetBounds are not at all threadsafe, so we
need to fix this situation stat.  I don't know much about
DrawOver but I suspect it will be pretty badly broken too.  It
looks like we need a "friend" class of widget that can park
itself on the compositor thread.  We should pull roc into that
discussion.

That reminds me ... how are we going to handle window resizes on
mac with omtc?

>diff --git a/gfx/layers/ipc/PCompositor.ipdl b/gfx/layers/ipc/PCompositor.ipdl

>+// This should really be 'sync', but we're using 'rpc' as a workaround
>+// for Bug 716631.
>+  // Workaround for Bug 717027.

Sorry about this.

>diff --git a/layout/ipc/RenderFrameParent.h b/layout/ipc/RenderFrameParent.h

>+  virtual void ShadowLayersUpdated();

  virtual void ShadowLayersUpdated() MOZ_OVERRIDE;


(rubber-stamp on the cocoa widgetry.)


>diff --git a/widget/src/xpwidgets/nsBaseWidget.cpp b/widget/src/xpwidgets/nsBaseWidget.cpp

>+    PLayersChild* shadowManager = mCompositorChild->SendPLayersConstructor(                                                LayerManager::LAYERS_OPENGL);

Whoa what happened here?

>+         * If we'd get a none-basic layer manager they'd crash. This is ok though

"non-basic"

Alright, this isn't shippable yet but let's get it landed.  Looking good!

r=me with fixes above and bugs filed.

Comment 20

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)

> But AnnotateCrashReport() is indeed thread safe (in chrome processes).

ScopedGfxFeatureReporter uses AppendAppNotesToCrashReport(), which isn't thread safe (since it access the global notesField object without any locking). I'll file a follow-up for this.

Comment 21

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> Comment on attachment 587713 [details] [diff] [review]
> Implementation of OMTC for OS X, v3
> 
> >diff --git a/gfx/layers/ipc/Compositor.h b/gfx/layers/ipc/Compositor.h
> 
> >+// Needed when we cannot directly include CompositorParent.h since it includes
> >+// an IPDL-generated header (e.g. IPDL-generated headers cannot be included in
> >+// widget/src/cocoa).
> 
> Why not?  Is there a bug on file for that?

I tried again and it worked fine -- not sure why it wasn't working before. So Compositor.h and Compositor.cpp are no longer needed.


> We should make ShadowLayersParent::RecvUpdate default to
> updating shadow properties for changed layers (it has that
> information), and then put the burden on the code in
> RenderFrameParent to only recompute transforms when needed.
> 
> But let's do that in a followup.  If you could file that and add
> a "FIXME" here, that'd be great.

Filed Bug 717688.

 
> Another issue for a followup: these methods in LayerManagerOGL
> touch its held widget pointer
> 
>   mWidget->GetClientBounds(rect);
>   mWidget->DrawOver(this, rect);
>   mWidget->GetBounds(rect);

Filed Bug 717925.


> That reminds me ... how are we going to handle window resizes on
> mac with omtc?

Currently (without OMTC), the resize event involves notifying the GLContext about the size change. So if we require acquiring a lock to access the GLContext, we can ensure that resizing blocks on composition. Filed Bug 717938 for this.


> >+  // Workaround for Bug 717027.

Now that Bug 717027 is fixed, the workaround is (of course) no longer needed.


(In reply to Ali Juma [:ajuma] from comment #20)
> ScopedGfxFeatureReporter uses AppendAppNotesToCrashReport(), which isn't
> thread safe (since it access the global notesField object without any
> locking). I'll file a follow-up for this.

Filed Bug 717951.

Also, filed Bug 717958 for making LayerManagerOGL's access to FPS counter preference thread-safe.

Comment 22

[Ali Juma [:ajuma]]

Attachment: Implementation of OMTC for OS X, v4

Patch updated to address review comments. Carrying forward r=cjones.

Comment 23

[Ali Juma [:ajuma]]

(In reply to Ali Juma [:ajuma] from comment #21)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> > Comment on attachment 587713 [details] [diff] [review]
> > Implementation of OMTC for OS X, v3
> > 
> > >diff --git a/gfx/layers/ipc/Compositor.h b/gfx/layers/ipc/Compositor.h
> > 
> > >+// Needed when we cannot directly include CompositorParent.h since it includes
> > >+// an IPDL-generated header (e.g. IPDL-generated headers cannot be included in
> > >+// widget/src/cocoa).
> > 
> > Why not?  Is there a bug on file for that?
> 
> I tried again and it worked fine -- not sure why it wasn't working before.
> So Compositor.h and Compositor.cpp are no longer needed.

I spoke too soon -- when including IPDL-generated headers in widget/cocoa/nsChildView.mm, we get errors when building nsChildView.o for 32-bit OS X. The problem is that when an IPDL-generated header is included before nsObjCExceptions.h, we include exception_defines.h before nsObjCExceptions.h, which in turn causes the problem described in Bug 568513, Comment 7. And we can't include nsObjCExceptions.h before an IPDL-generated header, since this causes the "You_must_include_basictypes.h_before_prtypes.h!" error.

Even though the 64-bit compiler isn't complaining, we're running into the same problem there too (in that try and catch are being redefined before nsObjCExceptions.h is included, which isn't what we want).

I'll file a follow-up bug for this, and revert to the Compositor.{h, cpp} workaround (renaming these files to CompositorCocoaWidgetHelper.{h, cpp} as suggested in Comment 19).

Comment 24

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

Sounds good to me.

Comment 25

[Ali Juma [:ajuma]]

Attachment: Implementation of OMTC for OS X, v5

Patch updated to address the issue from Comment 23 (which is now Bug 719036). Carrying forward r=cjones.

Comment 26

[Ali Juma [:ajuma]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd7228ca8a7

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5bd7228ca8a7