1271870 - Remove compositor widget access on MacOS

Status: RESOLVED WONTFIX

(Core :: Widget: Cocoa, defect)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

This is needed for compositor process.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Fix OSX Compile failure due to unified build.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Introduce OSXCompositorWidgetProxy.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Implements GetClientSize().

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Move PreRender and PostRender to OSXCompositorWidgetProxy.

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 5: Move other stuff to OSXCompositorWidgetProxy.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8751159 [details] [diff] [review]
Part 2: Introduce OSXCompositorWidgetProxy.

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

::: widget/cocoa/OSXCompositorWidgetProxy.h
@@ +15,5 @@
> +};
> +
> +namespace widget {
> +
> +class OSXCompositorWidgetProxy : public CompositorWidgetProxy

"MacCompositorWidgetProxy" would be more normal for Gecko, I think.

::: widget/cocoa/nsChildView.h
@@ +697,5 @@
>    // scrolling. It is reset to false once a new gesture starts (as indicated by
>    // a PANGESTURE_(MAY)START event).
>    bool mCurrentPanGestureBelongsToSwipe;
>  
> +  RefPtr<mozilla::widget::OSXCompositorWidgetProxy> mCompositorWidgetProxy;

I was trying to avoid this in the Windows patch - it leaves us open to the possibility of mCompositorWidgetProxy being different in nsBaseWidget versus nsChildView. Ideally it would exist in just one place, until we know what the full GPU process looks like.

To match Windows you could add an AsMacProxy() virtual method to CompositorWidgetProxy, then in nsChildView.mm have a helper like:

  MacCompositorWidgetProxy* GetWidgetProxy() {
    return mCompositorWidgetProxy->AsMacProxy();
  }

::: widget/cocoa/nsChildView.mm
@@ +2684,5 @@
>  }
>  
> +LayerManager*
> +nsChildView::GetLayerManager(layers::PLayerTransactionChild* aShadowManager,
> +                             layers::LayersBackend aBackendHint,

This overload can go away with the AsMacProxy change suggested.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8751160 [details] [diff] [review]
Part 3: Implements GetClientSize().

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

I'm going to defer to Markus for the rest since I don't know anything about OS X widgets :)

Markus, bug 1265975 and bug 1264545 might help for background.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8751159 [details] [diff] [review]
Part 2: Introduce OSXCompositorWidgetProxy.

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

::: widget/cocoa/OSXCompositorWidgetProxy.h
@@ +15,5 @@
> +};
> +
> +namespace widget {
> +
> +class OSXCompositorWidgetProxy : public CompositorWidgetProxy

nah, we're really inconsistent but have been using OSX more frequently than Mac recently. E.g. OSXVsyncSource, OSXNotificationCenter.

Comment 9

[Markus Stange [:mstange]]

So, if the goal is for OSXCompositorWidgetProxy to be used in the compositor process, then these patches won't get us closer to that goal. They access the NSView hierarchy of the widget, which will only exist in the parent process and will not be accessible from the compositor process.

On OS X, you can't just composite into a window from a different process. What you need to do instead is:
 - Have one GL context in the compositor process and one GL context in the parent process.
 - The compositor process needs to composite into an IOSurface and share that IOSurface to the parent process.
 - The parent process owns the widget, and its GL context is attached to that widget. It does a second "composite" step, drawing that IOSurface.

This second composite is what requires all the stuff that is currently in nsChildView.mm. Since this second composite happens on the widget's GL context, there's no value in moving the code that you're moving.

Comment 10

[Markus Stange [:mstange]]

To expand on this: I originally told David that we'll need to send a texture with the titlebar contents from the parent process to the compositor process. But I no longer think we should do this. Since we need to do a second composite in the parent process anyway, we can still use that composite to put the titlebar onto the screen. That should simplify things.
So, to repeat, I propose that things work like this: The compositor process composites the complete layer tree. Its output ends up in a framebuffer that is attached to an IOSurface which is shared to the parent process. Then the parent process draws the IOSurface and the titlebar (using nsChildView::DrawWindowOverlay) into the window's GL context.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Markus, thanks for your explanation. This is very helpful. I'll implement based on your opinion.

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Fix OSX Compile failure due to unified build. (carry r+: dvander)

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Introduce OSXCompositorWidgetProxy.

David, would you review this patch again. The big different from previous patch is
I added IsInProcess and AsMacProxy function.

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Move RectTextureImage to separate file.

Move this class out so we can easily reuse it in other class.

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Using MacIOSurface for RectTextureImage.

RectTextureImage uses shared memory which causes an extra copy to GPU. I replace it
with IOSurface to prevent this extra copy. Also we can use this IOSurface to share
buffer between compositor and widget.

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 5: Move mBasicCompositorImage to CompositorWidgetProxy.

Add ToSurfaceDescriptor and FromSurfaceDescriptor to RectTextureImage. So In OOP mode,
We can pass surface descriptor through IPC to share buffer.

Comment 17

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 6: Create OpenGL with offscreen mode so that we can forward surface to widget.

For OpenGL context and OOP compositor, we create it with offscreen mode. And then we
can share this offscreen buffer to widget by sending it surface descriptor. Note this
patch contained many TODO tags which are about IPC stuff. Those TODO tags would be
replaced by real IPC in follow-up patch.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8755680 [details] [diff] [review]
Part 4: Using MacIOSurface for RectTextureImage.

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

::: gfx/2d/MacIOSurface.h
@@ +109,5 @@
>    double GetContentsScaleFactor() { return mContentsScaleFactor; }
>    size_t GetDevicePixelWidth(size_t plane = 0);
>    size_t GetDevicePixelHeight(size_t plane = 0);
>    size_t GetBytesPerRow(size_t plane = 0);
> +  void Lock(bool aReadOnly = true);

I'd prefer this to take an enum, and have no default value. You can make that change in a follow-up patch.

::: widget/cocoa/RectTextureImage.h
@@ +21,3 @@
>  // Manages a texture which can resize dynamically, binds to the
>  // LOCAL_GL_TEXTURE_RECTANGLE_ARB texture target and is automatically backed
>  // by a power-of-two size GL texture. The latter two features are used for

You're removing the automatic power-of-two backing, so please change this comment.

@@ +72,1 @@
>    LayoutDeviceIntSize mBufferSize;

Please rename this to mSize now that the buffer is always sized to the used size.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8755677 [details] [diff] [review]
Part 2: Introduce OSXCompositorWidgetProxy.

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

::: widget/CompositorWidgetProxy.h
@@ +204,5 @@
>    virtual WinCompositorWidgetProxy* AsWindowsProxy() {
>      return nullptr;
>    }
>  
> +  virtual OSXCompositorWidgetProxy* AsMacProxy() {

nit: "AsOSXProxy" for consistency

Comment 20

[Markus Stange [:mstange]]

I think we need to talk about this approach in a larger group. I know it's implementing the plan I proposed, but after talking to Jeff he doesn't like the idea of this.

Comment 21

[Markus Stange [:mstange]]

I think landing part 4 is a good idea anyway. It may make BasicCompositor a little faster, and it won't run into arbitrary texture size restrictions in GLContext.

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Markus Stange [:mstange] [away until June 27] from comment #21)
> I think landing part 4 is a good idea anyway. It may make BasicCompositor a
> little faster, and it won't run into arbitrary texture size restrictions in
> GLContext.

Good idea, I'll create a bug for landing part 4.

Comment 23

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8755681 [details] [diff] [review]
Part 5: Move mBasicCompositorImage to CompositorWidgetProxy.

I'll request again once we are ready to do this.

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #22)
> (In reply to Markus Stange [:mstange] [away until June 27] from comment #21)
> > I think landing part 4 is a good idea anyway. It may make BasicCompositor a
> > little faster, and it won't run into arbitrary texture size restrictions in
> > GLContext.
> 
> Good idea, I'll create a bug for landing part 4.

Create bug 1281686 for this.

Comment 25

[Markus Stange [:mstange]]

Closing this bug. We currently have no plans to have a separate GPU process on macOS.