1186000 - Support screen mirroring to HDMI display on gonk

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Sotaro Ikeda [:sotaro]]

On android, if a device supports HDMI display, when HDMI display is connected to the device, the device outputs the main displays content to the HDMI display. The same thing should be done also on gonk.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Confirmed rendering to HDMI display on nexus-5-l. But rendering result had some problems. Some layers sizes were not scaled correctly.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix drawing problem.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Add screen rotation handling.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> Created attachment 8638015 [details] [diff] [review]
> wip patch
> 
> Add screen rotation handling.

ROTATION_180 do not work correctly.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix ROTATION_180 problem and cleanup.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Update nits.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

rebased.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Rebased.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

update thread handling.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add mirroring handling to nsScreenGonk

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add mirroring handling to LayerManagerComposite

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Fix HwcHAL's screen size handling

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add mirroring handling to nsScreenGonk

Rebased.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add mirroring handling to nsScreenGonk

Add pref

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add mirroring handling to LayerManagerComposite

Add pref.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Fix HwcHAL's screen size handling

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add mirroring handling to nsScreenGonk

Remove unnecessary change.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add mirroring handling to nsScreenGonk

Update comments.

Comment 21

[Sotaro Ikeda [:sotaro]]

I am going to update how to set viewport.

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> I am going to update how to set viewport.

I re-think about it. Just set viewport does not fix ClipRect coordinate correctly.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8639887 [details] [diff] [review]
patch part 2 - Add mirroring handling to LayerManagerComposite

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +891,5 @@
> +    // Only primary screen support mirroring
> +    return;
> +  }
> +
> +  nsWindow* mirrorScreenWidget = screen->GetMirroringWidget();

It's a shame that android doesn't hide their external screen stuff behind a window so that we could use the same code for both instead of #ifdefs.

@@ +983,5 @@
>    RootLayer()->RenderLayer(clipRect);
>  
>    mCompositor->EndFrame();
> +  nsRefPtr<Composer2D> composer2D;
> +  composer2D = mCompositor->GetWidget()->GetComposer2D();

Can you explain this bit more please.

Why do we try rendering with composer2D *after* we've already composited using Compositor? Why don't we check the return value of Composer2D::Render?

SetDispAcquireFence used to be called unconditionally, why is it only if we have a composer2d now?

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> Comment on attachment 8639887 [details] [diff] [review]
> patch part 2 - Add mirroring handling to LayerManagerComposite
> 
> Review of attachment 8639887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +891,5 @@
> > +    // Only primary screen support mirroring
> > +    return;
> > +  }
> > +
> > +  nsWindow* mirrorScreenWidget = screen->GetMirroringWidget();
> 
> It's a shame that android doesn't hide their external screen stuff behind a
> window so that we could use the same code for both instead of #ifdefs.
> 
> @@ +983,5 @@
> >    RootLayer()->RenderLayer(clipRect);
> >  
> >    mCompositor->EndFrame();
> > +  nsRefPtr<Composer2D> composer2D;
> > +  composer2D = mCompositor->GetWidget()->GetComposer2D();
> 
> Can you explain this bit more please.
> 
> Why do we try rendering with composer2D *after* we've already composited
> using Compositor? Why don't we check the return value of Composer2D::Render?

Composer2D(hardware composer) is implemented only on gonk. On gonk, Compositor render to adnroid::FramebufferSurface, it does not mean rendered to display. To render to display, hwc need to do composition. If all gfx layers are composited by Compositor, Composer2D works just like buffer forwarder.

The following is related diagram.
  https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/widget_HwcComposer2D_FirefoxOS_1_4.pdf

> SetDispAcquireFence used to be called unconditionally, why is it only if we
> have a composer2d now?

Sorry, I made a mistake:( I was confused with FramebufferSurface::setReleaseFenceFd() it is set by hwc. SetDispAcquireFence should be called after OpenGL buffer swap, It calls FramebufferSurface::onFrameAvailable(). I am going to update a patch.

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> Why don't we check the return value of Composer2D::Render?

Composer2D::Render() is a last calling function of RenderToPresentationSurface(), since SetDispAcquireFence() will be moved based on comment 24. There seems no necessity to check the return value of Composer2D::Render(). Is there a necessity to the check?

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add mirroring handling to LayerManagerComposite

Apply the comment.

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> > Why don't we check the return value of Composer2D::Render?
> 
> Composer2D::Render() is a last calling function of
> RenderToPresentationSurface(), since SetDispAcquireFence() will be moved
> based on comment 24. There seems no necessity to check the return value of
> Composer2D::Render(). Is there a necessity to the check?

Sorry, I got confused with TryRenderWithHwc(), which is used *before* we render our GL layers. This all makes sense now.

Comment 28

[Michael Wu [:mwu]]

Comment on attachment 8639983 [details] [diff] [review]
patch part 1 - Add mirroring handling to nsScreenGonk

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

The handling of mGLContext and mMirroringWidget is so tricky that I'm not sure that the smart pointers are helping us at all. Not sure if we can do any better with the current tools we have now though..

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +489,5 @@
> +    MOZ_ASSERT(IsPrimaryScreen());
> +
> +    if (mMirroringWidget) {
> +        nsCOMPtr<nsIWidget> widtet = mMirroringWidget.forget();
> +        NS_ReleaseOnMainThread(widtet);

nit: s/widtet/widget/

Comment 29

[Michael Wu [:mwu]]

Comment on attachment 8639888 [details] [diff] [review]
patch part 3 - Fix HwcHAL's screen size handling

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

::: widget/gonk/HwcComposer2D.cpp
@@ +747,5 @@
>          // No mHwc prepare, if already prepared in current draw cycle
>          mList->hwLayers[mList->numHwLayers - 1].handle = dispSurface->lastHandle;
>          mList->hwLayers[mList->numHwLayers - 1].acquireFenceFd = dispSurface->GetPrevDispAcquireFd();
>      } else {
> +        // Update screen rect

Doesn't seem like a useful comment.

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add mirroring handling to nsScreenGonk

Apply the comment.

Comment 32

[Tommy Kuo (away forever...)]

Hi Sotaro, I've finished the patch for Bug 1186800. But I've get the problem with the cropped result. Did you have the correct result with mirror display? I found the following link mentioned about the resolution bug.

https://code.google.com/p/android/issues/detail?id=40225

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Fix HwcHAL's screen size handling

Update a comment.

Comment 34

[Sotaro Ikeda [:sotaro]]

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

Comment 35

[Sotaro Ikeda [:sotaro]]

(In reply to Tommy Kuo [:KuoE0] from comment #32)
> Hi Sotaro, I've finished the patch for Bug 1186800.

There was not update in Bug 1186800. Can you upload the patch in Bug 1186800? I am not sure if it is similar to Bug 1186000 fix.

> But I've get the problem
> with the cropped result. Did you have the correct result with mirror
> display? I found the following link mentioned about the resolution bug.
> 
> https://code.google.com/p/android/issues/detail?id=40225

If you saw the problem, can you update the picture and STR?

Comment 36

[Tommy Kuo (away forever...)]

I uploaded my patch in Bug 1186800 already. I think it is as same as yours. Actually, the patch you provided is almost ready. And I'll upload the picture about the cropped result on an external display on Monday!

Comment 37

[Sotaro Ikeda [:sotaro]]

> > 
> > https://code.google.com/p/android/issues/detail?id=40225

This seems not a gecko's problem nor android's problem. hdmi display resolution is determined by platform's hal's implementation. In qcom's case, ExternalDisplay::configureHDMIDisplay() does it.

  http://androidxref.com/5.1.1_r6/xref/hardware/qcom/display/msm8974/libexternal/external.cpp#86

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/203271f48e46

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/203271f48e46