1148149 - Support Android Presentation API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Randall Barker [:rbarker]]

When screen casting to a chromecast from android, the Presentation API allows an application to render its own view instead of just mirroring the screen. By doing a second composite render to a SurfaceView, Fennec can mirror a page without having browser and OS chrome being visible in the screen cast.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch

Comment 2

[Randall Barker [:rbarker]]

Comment on attachment 8584109 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch

Who else should I ask to review this? I assume some one from gfx.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8584109 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch

Wes has played around a bit with the Presnetation API. Let's get another set of eyes on it.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8584109 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch

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

Looks pretty good to me! Let's see what jgilbert has to say.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +779,5 @@
>  }
>  
> +#ifdef MOZ_WIDGET_ANDROID
> +void
> +LayerManagerComposite::RenderToPresentationSurface()

We may want to make this a little more generic so it doesn't need to be wrapped in MOZ_WIDGET_ANDROID

@@ +873,5 @@
> +
> +  mCompositor->EndFrame();
> +  mCompositor->SetFBAcquireFence(mRoot);
> +
> +  compositor->SetProjMatrix(originalProjMatrix);

I like RAII wrappers for stuff like this so you can early-return without blowing stuff up. So you'd have AutoProjMatrix at the top or something like that.

Comment 5

[Randall Barker [:rbarker]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Comment on attachment 8584109 [details] [diff] [review]
> 0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch
> 
> Review of attachment 8584109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good to me! Let's see what jgilbert has to say.
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +779,5 @@
> >  }
> >  
> > +#ifdef MOZ_WIDGET_ANDROID
> > +void
> > +LayerManagerComposite::RenderToPresentationSurface()
> 
> We may want to make this a little more generic so it doesn't need to be
> wrapped in MOZ_WIDGET_ANDROID
> 

Well it currently does require the AndroidBridge. I would rather not make it more generic until we are ready to hook up the WebRTC tab sharing to the compositor since it will require some refactoring at that time any way.

Comment 6

[Randall Barker [:rbarker]]

> @@ +873,5 @@
> > +
> > +  mCompositor->EndFrame();
> > +  mCompositor->SetFBAcquireFence(mRoot);
> > +
> > +  compositor->SetProjMatrix(originalProjMatrix);
> 
> I like RAII wrappers for stuff like this so you can early-return without
> blowing stuff up. So you'd have AutoProjMatrix at the top or something like
> that.

Okay, but where should I put them? Right now I have them defined right above the function.

Comment 7

[Randall Barker [:rbarker]]

Attachment: 0002-RAII-Wrappers-15032716-2d2b676.patch

Comment 8

[Randall Barker [:rbarker]]

Comment on attachment 8584861 [details] [diff] [review]
0002-RAII-Wrappers-15032716-2d2b676.patch

Where should the scoped classes live?

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8584109 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +461,5 @@
>  
>  bool
>  GLContextEGL::SwapBuffers()
>  {
>      if (mSurface) {

It looks like this could use a:
MOZ_ASSERT_IF(!mSurface, !mSurfaceOverride)

@@ +462,5 @@
>  bool
>  GLContextEGL::SwapBuffers()
>  {
>      if (mSurface) {
> +        EGLSurface surface = mSurfaceOverride && mSurfaceOverride != EGL_NO_SURFACE ? mSurfaceOverride : mSurface;

`mSurfaceOverride && mSurfaceOverride != EGL_NO_SURFACE` is redundant.

@@ +802,5 @@
> +        MOZ_CRASH("Failed to create EGLConfig!\n");
> +        return nullptr;
> +    }
> +
> +    MOZ_ASSERT(aWindow != nullptr);

MOZ_ASSERT(aWindow) is fine.

@@ +808,5 @@
> +    EGLSurface surface = sEGLLibrary.fCreateWindowSurface(EGL_DISPLAY(), config, aWindow, 0);
> +
> +    if (surface == EGL_NO_SURFACE) {
> +        MOZ_CRASH("Failed to create EGLSurface!\n");
> +        return nullptr;

Don't bother returning after a MOZ_CRASH.

@@ +817,5 @@
> +
> +void
> +GLContextProviderEGL::DestroyEGLSurface(EGLSurface surface)
> +{
> +    mozilla::gl::DestroySurface(surface);

This looks like it should just call eglDestroySurface, but it also calls MakeCurrent unconditionally. These should not be packaged together in a non-obvious manner. R- for this.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8584109 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch

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

The .java stuff looks fine to me.

::: mobile/android/base/GeckoAppShell.java
@@ +291,5 @@
>  
>      public static native SurfaceBits getSurfaceBits(Surface surface);
>  
> +    public static native void addPresentationSurface(Surface surface);
> +    public static native void removePresentationSurface(Surface surface);

With the autogenerated jni code, I've been slowly trying to kill off GeckoAppShell and just reflect this directly from MediaPlayerManager for instance. Up to you if you want to move it though.

::: mobile/android/base/MediaPlayerManager.java
@@ +245,5 @@
> +        presentation = null;
> +    }
> +
> +    private void updatePresentation() {
> +        if (mediaRouter != null) {

I think this might look nicer with early returns, but I'm fine either way nowadays (Swift has forced me to accept lots of indentation).

@@ +270,5 @@
> +            } else if (presentation != null) {
> +                presentation.dismiss();
> +                presentation = null;
> +            }
> +        } else {

No need for this.

@@ +278,5 @@
> +    private static class SurfaceListener implements SurfaceHolder.Callback {
> +        @Override
> +        public void surfaceChanged(SurfaceHolder holder, int format, int width,
> +                                                int height) {
> +            GeckoAppShell.addPresentationSurface(holder.getSurface());

I bet this is right, but it looks wrong :) Can you add a note about why addPresentation is ok to do rather something like than changePresentationSurface?

Comment 11

[Randall Barker [:rbarker]]

(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 8584109 [details] [diff] [review]
> 0001-Bug-1148149-Support-Android-Presentation-API-15032615-63b6a78.patch
> 
> Review of attachment 8584109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The .java stuff looks fine to me.
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +291,5 @@
> >  
> >      public static native SurfaceBits getSurfaceBits(Surface surface);
> >  
> > +    public static native void addPresentationSurface(Surface surface);
> > +    public static native void removePresentationSurface(Surface surface);
> 
> With the autogenerated jni code, I've been slowly trying to kill off
> GeckoAppShell and just reflect this directly from MediaPlayerManager for
> instance. Up to you if you want to move it though.
> 

I tried to put them in MediaPlayerManger but got:

Error: Class android.support.v7.media.MediaRouter could not be found.
make[5]: *** [jni-stubs.inc] Error 1

Obviously I did something wrong but couldn't figure out what was missing.
 
> 
> @@ +278,5 @@
> > +    private static class SurfaceListener implements SurfaceHolder.Callback {
> > +        @Override
> > +        public void surfaceChanged(SurfaceHolder holder, int format, int width,
> > +                                                int height) {
> > +            GeckoAppShell.addPresentationSurface(holder.getSurface());
> 
> I bet this is right, but it looks wrong :) Can you add a note about why
> addPresentation is ok to do rather something like than
> changePresentationSurface?

It doesn't hurt to call it here but it doesn't need to be here either. I'll remove it an leave the function empty.

Comment 12

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15033110-793fd63.patch

Comment 13

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15033112-93bbb54.patch

Comment 14

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15033112-0cd925d.patch

Comment 15

[Randall Barker [:rbarker]]

Comment on attachment 8586292 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15033112-0cd925d.patch

I believe I have addressed all the review concerns except adding the MOZ_ASSERT_IF(!mSurface, !mSurfaceOverride) to the swap buffer since it seems it could potentially change the behavior of the call. If you feel it should be in there maybe a separate patch would be better?

Comment 16

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=957985cc5294

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8586292 [details] [diff] [review]
0001-Bug-1148149-Support-Android-Presentation-API-15033112-0cd925d.patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +776,5 @@
>    RecordFrame();
>  }
>  
> +#ifdef MOZ_WIDGET_ANDROID
> +namespace {

Why is this in an anonymous namespace.

@@ +793,5 @@
> +    mCompositor->SetProjMatrix(mOriginalProjMatrix);
> +  }
> +private:
> +  CompositorOGL* mCompositor;
> +  const Matrix4x4 mOriginalProjMatrix;

If mOriginalProjMatrix is const, so too should we have `CompositorOGL* const mCompositor`

@@ +835,5 @@
> +public:
> +  ScopedContextSurfaceOverride(GLContextEGL* aContext, void* aSurface) :
> +    mContext(aContext)
> +  {
> +    mContext->SetEGLSurfaceOverride(aSurface);

Assert that the current EGLSurface override is null.

@@ +900,5 @@
> +
> +  float scale = 1.0;
> +
> +  if ((pageWidth > actualWidth) || (pageHeight > actualHeight)) {
> +     const float scaleWidth = (float)actualWidth / (float)pageWidth;

3-space indents?

@@ +922,5 @@
> +  Matrix viewMatrix;
> +  viewMatrix.PreTranslate(-1.0, 1.0);
> +  viewMatrix.PreScale((2.0f * scale) / (float)actualWidth, (2.0f * scale) / (float)actualHeight);
> +  viewMatrix.PreScale(1.0f, -1.0f);
> +  viewMatrix.PreTranslate((int)((float)offset.x / scale), offset.y);

Describe what you're doing here.

@@ +925,5 @@
> +  viewMatrix.PreScale(1.0f, -1.0f);
> +  viewMatrix.PreTranslate((int)((float)offset.x / scale), offset.y);
> +
> +  Matrix4x4 projMatrix = Matrix4x4::From2D(viewMatrix);
> +  projMatrix._33 = 0.0f;

Why?

@@ +931,5 @@
> +  ScopedCompositorProjMatrix overrideProjMatrix(compositor, projMatrix);
> +
> +  ScopedScissorRect screen(egl, 0, 0, actualWidth, actualHeight);
> +  egl->fClearColor(0.0, 0.0, 0.0, 0.0);
> +  egl->fClear(LOCAL_GL_COLOR_BUFFER_BIT);

If the result is opaque, you might want to deliberately disable blending. Also, framebuffer invalidation might be more accurate than clearing.

Comment 18

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15040718-108afb5.patch

Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+

Comment 19

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15040815-afdb410.patch

Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+

Comment 20

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15040815-bd2db8c.patch

Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+

Comment 21

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15040815-acd8d03.patch

Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+

Comment 22

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e3309ff03f5

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9271d92ee0e2

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes affecting multiple robocop chunks.
https://hg.mozilla.org/integration/fx-team/rev/a1d6b3af4d41

https://treeherder.mozilla.org/logviewer.html#?job_id=2642906&repo=fx-team

Comment 25

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1148149-Support-Android-Presentation-API-15041022-2e5667d.patch

Comment 26

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ac3ae987a20

Comment 27

[Randall Barker [:rbarker]]

Carry forward :snorp and :jgilbert r+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5a1050092dd5

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: 
[Suggested wording]: Support Android Presentation API for screen casting
[Links (documentation, blog post, etc)]: