Closed Bug 1186000 Opened 4 years ago Closed 4 years ago

Support screen mirroring to HDMI display on gonk

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(3 files, 19 obsolete files)

11.09 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
11.23 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
8.97 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → sotaro.ikeda.g
Blocks: 1144103
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8636656 - Attachment is patch: true
Attachment #8636656 - Attachment mime type: text/x-patch → text/plain
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8636656 - Attachment is obsolete: true
Blocks: 1116089
Attached patch wip patch (obsolete) — Splinter Review
Confirmed rendering to HDMI display on nexus-5-l. But rendering result had some problems. Some layers sizes were not scaled correctly.
Attachment #8636730 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Fix drawing problem.
Attachment #8637307 - Attachment is obsolete: true
Depends on: 1186800
Depends on: 1186968
Attached patch wip patch (obsolete) — Splinter Review
Add screen rotation handling.
Attachment #8637543 - Attachment is obsolete: true
(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.
Depends on: 1187048
Depends on: 1187345
Attached patch wip patch (obsolete) — Splinter Review
Fix ROTATION_180 problem and cleanup.
Attachment #8638015 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Update nits.
Attachment #8638668 - Attachment is obsolete: true
Depends on: 1187503
Attached patch wip patch (obsolete) — Splinter Review
rebased.
Attachment #8638677 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Rebased.
Attachment #8639430 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
update thread handling.
Attachment #8639434 - Attachment is obsolete: true
Attachment #8639843 - Attachment is obsolete: true
Attachment #8639848 - Attachment description: patch - Add mirroring handling to nsScreenGonk → patch part 1 - Add mirroring handling to nsScreenGonk
Attachment #8639849 - Attachment description: patch - Add mirroring handling to LayerManagerComposite → patch part 2 - Add mirroring handling to LayerManagerComposite
Rebased.
Attachment #8639848 - Attachment is obsolete: true
Depends on: 1188381
Add pref
Attachment #8639862 - Attachment is obsolete: true
Add pref.
Attachment #8639849 - Attachment is obsolete: true
Attachment #8639850 - Attachment is obsolete: true
Attachment #8639888 - Flags: review?(mwu)
Remove unnecessary change.
Attachment #8639885 - Attachment is obsolete: true
Update comments.
Attachment #8639979 - Attachment is obsolete: true
Attachment #8639983 - Flags: review?(mwu)
Attachment #8639887 - Flags: review?(nical.bugzilla)
Attachment #8639887 - Flags: review?(matt.woodrow)
Attachment #8639887 - Flags: review?(nical.bugzilla)
Attachment #8639887 - Flags: review?(matt.woodrow)
I am going to update how to set viewport.
(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.
Attachment #8639887 - Flags: review?(matt.woodrow)
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?
(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.
(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?
Apply the comment.
Attachment #8639887 - Attachment is obsolete: true
Attachment #8639887 - Flags: review?(matt.woodrow)
Attachment #8643839 - Flags: review?(matt.woodrow)
(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.
Attachment #8643839 - Flags: review?(matt.woodrow) → review+
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/
Attachment #8639983 - Flags: review?(mwu) → review+
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.
Attachment #8639888 - Flags: review?(mwu) → review+
Apply the comment.
Attachment #8639983 - Attachment is obsolete: true
Attachment #8645058 - Flags: review+
No longer depends on: 1186800
Duplicate of this bug: 1186800
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
Update a comment.
Attachment #8639888 - Attachment is obsolete: true
Attachment #8645065 - Flags: review+
Depends on: 1192352
(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?
Flags: needinfo?(kuoe0)
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!
Flags: needinfo?(kuoe0)
> > 
> > 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
https://hg.mozilla.org/mozilla-central/rev/203271f48e46
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1192949
Duplicate of this bug: 1186800
Blocks: 1194249
No longer blocks: 1194249
Depends on: 1194249
Blocks: 1225402
No longer blocks: 1225402
You need to log in before you can comment on or make changes to this bug.