Support Android Presentation API

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
10 days ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

Trunk
Firefox 40
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, relnote-firefox 40+)

Details

Attachments

(1 attachment, 9 obsolete attachments)

38.71 KB, patch
Details | Diff | Splinter Review
Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Assignee: nobody → rbarker
Assignee

Updated

4 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
Assignee

Comment 2

4 years ago
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.
Attachment #8584109 - Flags: review?(snorp)
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.
Attachment #8584109 - Flags: feedback?(wjohnston)
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.
Attachment #8584109 - Flags: review?(snorp)
Attachment #8584109 - Flags: review?(jgilbert)
Attachment #8584109 - Flags: review+
Assignee

Comment 5

4 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Assignee

Comment 6

4 years ago
> @@ +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.
Assignee

Comment 8

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

Where should the scoped classes live?
Attachment #8584861 - Flags: feedback?(snorp)
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.
Attachment #8584109 - Flags: review?(jgilbert) → review-
Attachment #8584861 - Flags: feedback?(snorp) → feedback+
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?
Attachment #8584109 - Flags: feedback?(wjohnston) → feedback+
Assignee

Comment 11

4 years ago
(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.
Assignee

Comment 12

4 years ago
Attachment #8584109 - Attachment is obsolete: true
Attachment #8584861 - Attachment is obsolete: true
Assignee

Comment 15

4 years ago
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?
Attachment #8586292 - Flags: review?(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.
Attachment #8586292 - Flags: review?(jgilbert) → review+
Assignee

Comment 18

4 years ago
Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+
Attachment #8586292 - Attachment is obsolete: true
Assignee

Comment 19

4 years ago
Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+
Attachment #8589413 - Attachment is obsolete: true
Assignee

Comment 20

4 years ago
Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+
Attachment #8589932 - Attachment is obsolete: true
Assignee

Comment 21

4 years ago
Address review comments and rebase to tip of inbound.
Carry forward :snorp and :jgilbert r+
Attachment #8589937 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 27

4 years ago
Carry forward :snorp and :jgilbert r+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5a1050092dd5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Depends on: 1155678
Release Note Request (optional, but appreciated)
[Why is this notable]: 
[Suggested wording]: Support Android Presentation API for screen casting
[Links (documentation, blog post, etc)]:
You need to log in before you can comment on or make changes to this bug.