Currently GLContextProviderEGL uses the nsIWidget to obtain the native surface for GLContext creation. When the GPU Process is enabled, it will not have access to the nsIWidget as it will reside in the parent process. Therefore it must be converted to use the CompositorWidget which is available in both the GPU process as well as the parent process.

Would you take a look at these patches and let me know if this is the correct approach? Thanks.

Will GLContextProviderEGL operate in the GPU process or UI process? Or both?

(In reply to David Anderson [:dvander] from comment #8) > Will GLContextProviderEGL operate in the GPU process or UI process? Or both? The context is created in what ever process and thread the compositor is used so it would be in the GPU process if it exists. Since there is no nsWindow in the GPU process this seemed like the best solution. I would also guess that as some point the function that takes an nsIWindow to create the context could be removed.

Drive-by comment: FYI we recently made GLContextProviderWGL GPU-process-safe in bug 1323799 and the patch seems a lot smaller/simpler than what you're doing here. In particular the necessary native data stuff was just passed in without having to create a wrapper. Not sure if that's useful to you or not.

Comment on attachment 8826756 [details] [diff] [review] 0002-Bug-1329362-part-2-Convert-GLContextProviderEGL-to-use-CompositorWidget-instead-of-nsIWidget-internally-17011313-dbf3579.patch Review of attachment 8826756 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/CompositorWidget.h @@ +297,5 @@ > } > > protected: > explicit CompositorWidget(const layers::CompositorOptions& aOptions); > + CompositorWidget(){} Umm we shouldn't create a CompositorWidget without CompositorOptions, otherwise those options will just initialized to 0 which is not correct.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > Drive-by comment: FYI we recently made GLContextProviderWGL GPU-process-safe > in bug 1323799 and the patch seems a lot smaller/simpler than what you're > doing here. In particular the necessary native data stuff was just passed in > without having to create a wrapper. Not sure if that's useful to you or not. I tried the same approach used in WGL first, however GLContextEGL::CreateGLContext is private and had to be called from a member function of GLContextProviderEGL because it is declared a friend of GLContextEGL. When I made the function a member function, things got complicated because then *every* version of GLContextProvider needed to have that function implemented due to the fact that we are using a macro in GLContextProvider.h to define at compile time which implementation to actually use. GLContextEGL::CreateGLContext could be made public and that would alleviate the need to add a member function but I assumed it had been made private so that no would call it by mistake. I would rather not have use the wrapper but the other solutions seemed to be more complicated in actual scope. I'm open to suggestions if you see a cleaner path.

I updated the original patches to address comment #13. Additionally I created an alternate patch that does not require a wrapper. I assume the ALTERNATE APPROACH patch is what you had in mind? If so, who would you recommend I ask review this? Thanks.

From a first glance, yes, the alternate approach looks better to me. I will take a closer look at the patch later today. Note that the updated original approach still has problems for the graphics team, because in the graphics branch we *do* use that CompositorOptions instance, and we need it to have real values. https://hg.mozilla.org/projects/graphics/file/585ed02acd43/gfx/gl/GLContextProviderEGL.cpp#l715

Comment on attachment 8828878 [details] [diff] [review] 0001-Bug-1329362-Update-GLContextProviderEGL-to-support-multiprocess-context-creation-17012008-5f0e802.patch Review of attachment 8828878 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this generally looks sane as a first step - making GLContextProviderEGL use a CompositorWidget instead of a nsIWidget. However this patch has a number of things going on making it hard to review, it would be really really really really helpful if you split them out into separate patches. For example: Part 1 - Removal of unused nsIWidget* arguments Part 2 - Addition of GetNativeData functions on CompositorWidget Part 3 - Changes to the various call sites that pass in nsIWidget* to instead pass in CompositorWidget* and pushing the RealWidget() calls in one level (without getting rid of them entirely) Part 4 - Android-specific changes Part 5 - GTK-specific changes Part 6 - windows-specific changes etc. I'm not saying to use exactly the split I described above, but try to keep the patches small. Reviewing difficulty is compounded by the fact that these files have some changes in the graphics branch and so we need to make sure each change can be properly merged over there. I can do a first pass of formal review and can flag dvander for review on anything I'm not sure about.

Comment on attachment 8829018 [details] [diff] [review] 0009-Bug-1329362-part-9-Update-GLContextProviderEGL-CreateForCompositorWidget-and-GLContextProviderEGL-CreateForWindow-to-use-GLContextEGLFactory-Create-for-GLContext-creation-17012016-6804e8b.patch Review of attachment 8829018 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for splitting up the patches like this! They're easily 100x easier to review in terms of total review time. ::: gfx/gl/GLContextProviderEGL.cpp @@ +139,5 @@ > } > > +class GLContextEGLFactory { > +public: > + static already_AddRefed<GLContext> Create(EGLNativeWindowType aWindow); This indent looks like three spaces. Should be four to be consistent with the file @@ +146,5 @@ > + ~GLContextEGLFactory(){} > +}; > + > +already_AddRefed<GLContext> > +GLContextEGLFactory::Create(EGLNativeWindowType aWindow) Add a comment somewhere on this class that indicates you had to create it to get access to private GLContextEGL members, without modifying the set of functions on GLContextProviderEGL because of the shared interface.

Comment on attachment 8829011 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to-CompositorWidget-and-InProcessCompositorWidget-17012016-2e62f4b.patch Review of attachment 8829011 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/CompositorWidget.h @@ +269,5 @@ > + return nullptr; > + } > + > + virtual void SetNativeData(uint32_t aDataType, uintptr_t aVal) > + {} I don't think you're using CompositorWidget::SetNativeData anywhere, so it can be removed (at least for now). If my understanding of the compositorwidget architecture is correct, you won't need this function at all. Instead, when you need to pass the surface from the Android widgetry in the UI process, the way to do it is to have an android-specific CompositorWidgetDelegate class. nsBaseWidget (in the UI process) will have a handle to that from [1]. That same class also implements CompositorWidgetChild, and can send the surface over IPC to CompositorWidgetParent. The object that implements CompositorWidgetParent will be the Android CompositorWidget, and so it just needs to hold on to the surface that comes in over IPC and then returns it from GetNativeData. However you mentioned on IRC that you're using Android binders and such instead, so maybe there's some reason you can't do this? I'm not sure. Anyway, :dvander should sign off on this either way, so I'll redirect the review to him. [1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/widget/nsBaseWidget.cpp#1312

Comment on attachment 8829011 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to-CompositorWidget-and-InProcessCompositorWidget-17012016-2e62f4b.patch Review of attachment 8829011 [details] [diff] [review]: ----------------------------------------------------------------- Why/where does the compositor need to access this data?

That's in part 6 and part 9. The compositor needs the surface to create a GL context. I just realized that CompositorWidgetWGL does the equivalent thing by casting to the windows-specific WinCompositorWidget and getting it from that [1]. That avoids putting the GetNativeData function on the CompositorWidget interface, and that's probably what we should be doing here as well. [1] https://hg.mozilla.org/projects/graphics/file/a5a6f59731d6/gfx/gl/GLContextProviderWGL.cpp#l497

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36) > That's in part 6 and part 9. The compositor needs the surface to create a GL > context. > > I just realized that CompositorWidgetWGL does the equivalent thing by > casting to the windows-specific WinCompositorWidget and getting it from that > [1]. That avoids putting the GetNativeData function on the CompositorWidget > interface, and that's probably what we should be doing here as well. > > [1] > https://hg.mozilla.org/projects/graphics/file/a5a6f59731d6/gfx/gl/ > GLContextProviderWGL.cpp#l497 But the Window's provider only has to support windows. Are you saying I should ifdef this for each platorm in EGL? That seems worse to me.

Comment on attachment 8829011 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to-CompositorWidget-and-InProcessCompositorWidget-17012016-2e62f4b.patch Review of attachment 8829011 [details] [diff] [review]: ----------------------------------------------------------------- Okay, that's what I was wondering. Yeah I'd rather we do that instead, since this API is an easy way to expose things that might not work.

(In reply to Randall Barker [:rbarker] from comment #37) > But the Window's provider only has to support windows. Are you saying I > should ifdef this for each platorm in EGL? That seems worse to me. For now you only need to care about Android, right? So you can just do #define GET_NATIVE_WINDOW(aWidget) ((EGLNativeWindowType)aWidget->AsAndroid()->GetJavaSurface() and leave the other GET_NATIVE_WINDOW defines as aWidget->RealWidget()->GetNativeData(...). We can deal with the other platforms one at a time.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39) > (In reply to Randall Barker [:rbarker] from comment #37) > > But the Window's provider only has to support windows. Are you saying I > > should ifdef this for each platorm in EGL? That seems worse to me. > > For now you only need to care about Android, right? So you can just do > > #define GET_NATIVE_WINDOW(aWidget) > ((EGLNativeWindowType)aWidget->AsAndroid()->GetJavaSurface() > > and leave the other GET_NATIVE_WINDOW defines as > aWidget->RealWidget()->GetNativeData(...). We can deal with the other > platforms one at a time. No, I would have to create a new macro because the GET_NATIVE_WINDOW is also used on the nsIWidget. But I can, it just seems to be more complicated for what benefit?

The benefit is less pollution of the CompositorWidget interface. In the end it's dvander's call but I agree with him that having a function like GetNativeData on CompositorWidget where it behaves very differently from the same function on nsIWidget seems like a footgun. People might start using it expecting it to work normally, or might even start calling it accidentally from places they're not supposed to.

I'd like to see the nsIWidget::GetNativeData API go away someday, since it exposes a bunch of untyped, platform-specific data that gets misused a lot. There's little overlap between platforms too. Even if it's a little more complicated to expose Android-specific methods, it makes the code more robust, which makes it clearer for what new platforms have to do to support CompositorWidgets.

Address review comments.

This should probably be re-reviewed since I had to re-write it based on the changes in patch #2.

Comment on attachment 8829661 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to-AndroidCompositorWidget-r-dvander-17012314-7a3a546.patch Review of attachment 8829661 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/AndroidCompositorWidget.h @@ +38,5 @@ > ScreenMargin& aFixedLayerMargins); > + > + > + void* GetNativeData(uint32_t aDataType); > + void SetNativeData(uint32_t aDataType, uintptr_t aVal); I think the idea is to make the function "GetEGLSurface" instead of "GetNativeData", and to drop SetNativeData entirely.

Comment on attachment 8829662 [details] [diff] [review] 0009-Bug-1329362-part-9-Update-GLContextProviderEGL-CreateForCompositorWidget-and-GLContextProviderEGL-CreateForWindow-to-use-GLContextEGLFactory-Create-for-GLContext-creation-r-kats-17012314-84afe0e.patch Review of attachment 8829662 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderEGL.cpp @@ +11,3 @@ > #elif defined(MOZ_WIDGET_ANDROID) > + #define GET_NATIVE_WINDOW_FROM_REAL_WIDGET(aWidget) ((EGLNativeWindowType)aWidget->GetNativeData(NS_JAVA_SURFACE)) > + #define GET_NATIVE_WINDOW_FROM_COMPOSITOR_WIDGET(aWidget) ((EGLNativeWindowType)aWidget->AsAndroid()->GetNativeData(NS_JAVA_SURFACE)) And then this would be ..->AsAndroid()->GetEGLSurface()

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45) > Comment on attachment 8829661 [details] [diff] [review] > 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to- > AndroidCompositorWidget-r-dvander-17012314-7a3a546.patch > > Review of attachment 8829661 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/android/AndroidCompositorWidget.h > @@ +38,5 @@ > > ScreenMargin& aFixedLayerMargins); > > + > > + > > + void* GetNativeData(uint32_t aDataType); > > + void SetNativeData(uint32_t aDataType, uintptr_t aVal); > > I think the idea is to make the function "GetEGLSurface" instead of > "GetNativeData", and to drop SetNativeData entirely. Sorry, I missed the part where you wanted the function name to change. I will still need to set the surface externally in Bug 1331109 since the surface has to come over binder and I need some way to get it into the AndroidCompositorWidget. But I can remove it and add it back in Bug 1331109. It just made more sense to me to add the Get/Set in the same patch.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45) > Comment on attachment 8829661 [details] [diff] [review] > 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to- > AndroidCompositorWidget-r-dvander-17012314-7a3a546.patch > > Review of attachment 8829661 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/android/AndroidCompositorWidget.h > @@ +38,5 @@ > > ScreenMargin& aFixedLayerMargins); > > + > > + > > + void* GetNativeData(uint32_t aDataType); > > + void SetNativeData(uint32_t aDataType, uintptr_t aVal); > > I think the idea is to make the function "GetEGLSurface" instead of > "GetNativeData", and to drop SetNativeData entirely. Sorry, I forgot that SetNativeData *is* currently used: https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/gfx/layers/composite/LayerManagerComposite.cpp#1059

Comment on attachment 8829661 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to-AndroidCompositorWidget-r-dvander-17012314-7a3a546.patch Review of attachment 8829661 [details] [diff] [review]: ----------------------------------------------------------------- Right - ideally this should only expose the data needed, as the proper type.

(In reply to David Anderson [:dvander] from comment #49) > Comment on attachment 8829661 [details] [diff] [review] > 0002-Bug-1329362-part-2-Add-GetNativeData-and-SetNativeData-to- > AndroidCompositorWidget-r-dvander-17012314-7a3a546.patch > > Review of attachment 8829661 [details] [diff] [review]: > ----------------------------------------------------------------- > > Right - ideally this should only expose the data needed, as the proper type. Okay, but the proper type is a void*: gfx/gl/GLTypes.h:typedef void* EGLSurface;

(In reply to Randall Barker [:rbarker] from comment #48) > Sorry, I forgot that SetNativeData *is* currently used: > > https://dxr.mozilla.org/mozilla-central/rev/ > 3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/gfx/layers/composite/ > LayerManagerComposite.cpp#1059 Oh, so then you need functions like these on AndroidCompositorWidget: ANativeWindow* GetPresentationWindow(); EGLSurface GetEGLSurface(); void SetEGLSurface(EGLSurface aSurface); and the code in LayerManagerComposite::RenderToPresentationSurface should be calling these functions via mCompositor->GetWidget->AsAndroid().

Address review comments.

Address review comments.

Comment on attachment 8829702 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-accessor-functions-for-EGLSurface-and-ANativeWindow-to-AndroidCompositorWidget-r-dvander-17012317-d8901b4.patch I may have messed up the types. Looking at it again.

Fixed issue with EGLNativeWindowType.

Updated after fix to patch #2.

Comment on attachment 8829709 [details] [diff] [review] 0009-Bug-1329362-part-9-Update-GLContextProviderEGL-CreateForCompositorWidget-and-GLContextProviderEGL-CreateForWindow-to-use-GLContextEGLFactory-Create-for-GLContext-creation-r-kats-17012318-d2d173d.patch Review of attachment 8829709 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderEGL.cpp @@ +151,5 @@ > +public: > + static already_AddRefed<GLContext> Create(EGLNativeWindowType aWindow); > +private: > + GLContextEGLFactory(){} > + ~GLContextEGLFactory(){} Now the indenting here looks like 5 spaces instead of 4. Also please add a space between () and {}

Comment on attachment 8829707 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-accessor-functions-for-EGLNativeWindowType-ANativeWindow-and-EGLSurface-to-AndroidCompositorWidget-r-dvander-17012318-9ae649d.patch Review of attachment 8829707 [details] [diff] [review]: ----------------------------------------------------------------- Presumably part 8 also needs updating as a result of these changes, so that LayerManagerComposite uses the new functions. I'm still not convinced you need all the setter functions, but I don't know the details of how the binder stuff works so maybe you do.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #58) > Comment on attachment 8829707 [details] [diff] [review] > 0002-Bug-1329362-part-2-Add-accessor-functions-for-EGLNativeWindowType- > ANativeWindow-and-EGLSurface-to-AndroidCompositorWidget-r-dvander-17012318- > 9ae649d.patch > > Review of attachment 8829707 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm still not convinced you > need all the setter functions, but I don't know the details of how the > binder stuff works so maybe you do. I will just add them in the patch where I'm using them.

Comment on attachment 8829937 [details] [diff] [review] 0008-Bug-1329362-part-8-Convert-LayerManagerComposite-RenderToPresentationSurface-to-use-CompositorWidget-instead-of-nsIWidget-r-kats-17012409-510586d.patch Review of attachment 8829937 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the ifdef restored ::: gfx/layers/composite/LayerManagerComposite.cpp @@ +1037,5 @@ > void > LayerManagerComposite::RenderToPresentationSurface() > { > + widget::CompositorWidget* const widget = mCompositor->GetWidget(); > + ANativeWindow* window = widget->AsAndroid()->GetPresentationANativeWindow(); I think you need to keep the #ifdef MOZ_WIDGET_ANDROID, otherwise this won't compile on other platforms (because they don't have AndroidCompositorWidget.h). And you can make the local |widget| variable an AndroidCompositorWidget so you just need the ->AsAndroid() call once instead of three times.

Comment on attachment 8829938 [details] [diff] [review] 0009-Bug-1329362-part-9-Update-GLContextProviderEGL-CreateForCompositorWidget-and-GLContextProviderEGL-CreateForWindow-to-use-GLContextEGLFactory-Create-for-GLContext-creation-r-kats-17012409-cd00cf8.patch Review of attachment 8829938 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Probably good to do a try push before landing to make sure the patchset builds everywhere.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #69) > Comment on attachment 8829937 [details] [diff] [review] > 0008-Bug-1329362-part-8-Convert-LayerManagerComposite- > RenderToPresentationSurface-to-use-CompositorWidget-instead-of-nsIWidget-r- > kats-17012409-510586d.patch > > Review of attachment 8829937 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with the ifdef restored > > I think you need to keep the #ifdef MOZ_WIDGET_ANDROID, otherwise this won't > compile on other platforms (because they don't have > AndroidCompositorWidget.h). And you can make the local |widget| variable an > AndroidCompositorWidget so you just need the ->AsAndroid() call once instead > of three times. > It's redundant, the whole function is in an #if defined(MOZ_WIDGET_ANDROID) block: https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/gfx/layers/composite/LayerManagerComposite.cpp#967 I think it is left over from when FxOS was trying to use the function as well.

Hah, ok that's fine then.

try from last night: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6007c702c3bdd29fea071bcf4b83386c7f86395

try from today: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47202aa5aac71b61870f48bf7f4033534e45267e Looks like the usual random try failures.

Comment on attachment 8829929 [details] [diff] [review] 0002-Bug-1329362-part-2-Add-accessor-functions-for-EGLNativeWindowType-ANativeWindow-and-EGLSurface-to-AndroidCompositorWidget-r-dvander-17012409-015e722.patch Review of attachment 8829929 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Pushed by rbarker@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cff8143e007 part 1, Remove unused GLContextEGL::CreateSurfaceForWindow and GLContextEGL::DestroySurface functions r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/4132cf335efd part 2, Add accessor functions for EGLNativeWindowType, ANativeWindow, and EGLSurface to AndroidCompositorWidget r=dvander,kats https://hg.mozilla.org/integration/mozilla-inbound/rev/8b768ff7e836 part 3, Convert GLContext::RenewSurface to use CompositorWidget in place of nsIWidget r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/76b7bb41377a part 4, Remove unused Windows class AutoDestroyHWND from GLContextProviderEGL r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/84217cc8f8e8 part 5, Remove unused Android LOG macro r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/5c27508f4643 part 6, Rename Android specific macro GET_JAVA_SURFACE to GET_NATIVE_WINDOW in GLContextProviderEGL r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c4a087327d part 7, Remove unused nsIWidget parameter from CreateConfig functions in GLContextProviderEGL r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/6998e73596c6 part 8, Convert LayerManagerComposite::RenderToPresentationSurface to use CompositorWidget instead of nsIWidget r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/46031962847c part 9, Update GLContextProviderEGL::CreateForCompositorWidget and GLContextProviderEGL::CreateForWindow to use GLContextEGLFactory::Create for GLContext creation r=kats

https://hg.mozilla.org/mozilla-central/rev/6cff8143e007 https://hg.mozilla.org/mozilla-central/rev/4132cf335efd https://hg.mozilla.org/mozilla-central/rev/8b768ff7e836 https://hg.mozilla.org/mozilla-central/rev/76b7bb41377a https://hg.mozilla.org/mozilla-central/rev/84217cc8f8e8 https://hg.mozilla.org/mozilla-central/rev/5c27508f4643 https://hg.mozilla.org/mozilla-central/rev/f5c4a087327d https://hg.mozilla.org/mozilla-central/rev/6998e73596c6 https://hg.mozilla.org/mozilla-central/rev/46031962847c