1138287 - Refactor GonkDisplay/nsWindow/hwc to support multiple displays

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shelly Lin [:shelly]]

To support display types other than primary display, here are the possible TODOs:

- Create a new class "DisplayDevice" in widget/gonk/libdisplay, to store the connected display device info, such as width, height, xdpi, surface format, and its ANativeWindow...

- Changes in GonkDisplay/GonkDisplayJB
-- Create display types {DISPLAY_PRIMARY, DISPLAY_EXTERNAL, DISPLAY_VIRTUAL}
-- Create interfaces of "AddDisplay", "RemoveDisplay", "GetDevice"

- Changes in nsWindow
-- Should manage an array of DisplayDevice
-- Extend the current impl of 'sVirtualBounds'

Comment 1

[Shelly Lin [:shelly]]

Correct the part of "Changes in nsWindow":

- GonkDisplayJB should manage a list of DisplayDevice
- nsWindow should add a 'mDisplayType' indicating which display does this window bound to
- Extension of the current impl of 'sVirtualBounds' is still needed

Comment 2

[Shelly Lin [:shelly]]

Latest update:
This bug does not involve changes of nsWindow.

Comment 3

[Shelly Lin [:shelly]]

Attachment: [WIP gonk-libdisplay.diff

Comment 4

[Shelly Lin [:shelly]]

Attachment: [WIP] multiscreen-gonk.diff

I think we have reached a pretty solid state as to the overall design structure, please ignore my debug printings, I'll clean them up before requesting reviews, thanks a lot!

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 8584395 [details] [diff] [review]
[WIP] multiscreen-gonk.diff

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

I have to think more about the GonkDisplay changes, but I might as well post the comments I have now.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +211,5 @@
>                    bool isOffscreen,
>                    EGLConfig config,
>                    EGLSurface surface,
> +                  EGLContext context,
> +                  uint32_t aDisplayType)

I don't think we actually want to tell GLContextEGL about the display type. The path here is using the display type for three reasons:

1. To avoid calling hwc->Init on the secondary displays. The problem is HWC initialization doesn't actually belong here. IIRC the main reason to do initialization there is because we can't get mSurface otherwise. I suspect we should be obtaining mSurface at the time we need it though - when TryRender or Render is called. Render already passes a surface, so we need to also pass a surface to TryRender.
2. To pass the display type to HWC when calling GLContextEGL::SwapBuffers. This would only happen if HWC is disabled. We can support figuring out the display type using only the EGLSurface, but this can be considered at a later point.
3. To determine whether we should make treat this as offscreen in CreateForWindow. The external displays should not be treated as offscreen, because they're not.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +667,5 @@
>    }
>  
> +  int displaytype = 0;
> +  if (composer2D) {
> +    displaytype = (static_cast<nsWindow*>(mCompositor->GetWidget()))->GetDisplayType();

Rather than getting the display type from the window, just pass the window to hwc.

::: gfx/layers/opengl/CompositingRenderTargetOGL.cpp
@@ +58,4 @@
>      }
>  
>      mCompositor->PrepareViewport(mInitParams.mSize);
> +    mGL->fScissor(0, 0, mInitParams.mSize.width, mInitParams.mSize.height);

What's this for?

Comment 6

[Sotaro Ikeda [:sotaro]]

shelly, is there a overview design document about it?

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> shelly, is there a overview design document about it?

It seems to exit in Bug 1116089.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > shelly, is there a overview design document about it?
> 
> It seems to exit in Bug 1116089.

I checked Bug 1116089, but there seems not exit about a diagram/document that talks about how Widget and gfx is going to be changed.

shelly, is there a such document existed somethere?

Comment 9

[Sotaro Ikeda [:sotaro]]

The such document could help to understand if the change is correct from architecture point of view and could help to find out problems.

Comment 10

[Sotaro Ikeda [:sotaro]]

The reason why I asked the question in Comment 8 is that I want to understand overall architecture correctly. I could imagine the use case that 2nd display shows same contents as main display. But I could not imagine correctly 2nd display show different contents. 

One example is that during WebRTC video chat, show peer's video to 2nd display. But I could not find correct API to do it. At first I thought that it is provided by Presentation API (bug 1069230). But it seems not provide a capability what I expected. This api could not work for the above WebRTC use case. 

http://w3c.github.io/presentation-api/

Comment 11

[Marco Chen [:mchen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> One example is that during WebRTC video chat, show peer's video to 2nd
> display. But I could not find correct API to do it. At first I thought that
> it is provided by Presentation API (bug 1069230). But it seems not provide a
> capability what I expected. This api could not work for the above WebRTC use
> case. 
> 
> http://w3c.github.io/presentation-api/

Hi Sotaro,

For your example, I think Presentation API can help you.
But we don't talk it here in order to keep this bug focused on what it want to do.

Maybe you can mail me or open another user story bug and needinfo me.

Thanks.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Marco Chen [:mchen] from comment #11)
> (In reply to Sotaro Ikeda [:sotaro] from comment #10)
> > One example is that during WebRTC video chat, show peer's video to 2nd
> > display. But I could not find correct API to do it. At first I thought that
> > it is provided by Presentation API (bug 1069230). But it seems not provide a
> > capability what I expected. This api could not work for the above WebRTC use
> > case. 
> > 
> > http://w3c.github.io/presentation-api/
> 
> Hi Sotaro,
> 
> For your example, I think Presentation API can help you.
> But we don't talk it here in order to keep this bug focused on what it want
> to do.
> 
> Maybe you can mail me or open another user story bug and needinfo me.
> 
> Thanks.

Ok, I create another user story bug. I think if we do not have a correct understanding about overall architecture, we could fail to implement each component correctly.

Comment 13

[Sotaro Ikeda [:sotaro]]

> Hi Sotaro,
> 
> For your example, I think Presentation API can help you.
> But we don't talk it here in order to keep this bug focused on what it want
> to do.
> 
> Maybe you can mail me or open another user story bug and needinfo me.
> 
> Thanks.

Bug 1150503 was created.

Comment 14

[Sotaro Ikeda [:sotaro]]

In Bug 1144103, it is going to change GonkDisplay by a different approach.

Comment 15

[Shelly Lin [:shelly]]

(In reply to Michael Wu [:mwu] from comment #5)
> Comment on attachment 8584395 [details] [diff] [review]
> [WIP] multiscreen-gonk.diff
> 
> Review of attachment 8584395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have to think more about the GonkDisplay changes, but I might as well post
> the comments I have now.
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +211,5 @@
> >                    bool isOffscreen,
> >                    EGLConfig config,
> >                    EGLSurface surface,
> > +                  EGLContext context,
> > +                  uint32_t aDisplayType)
> 
> I don't think we actually want to tell GLContextEGL about the display type.
> The path here is using the display type for three reasons:
> 
The main reason we let GLContextEGL know about the display type, is because hwc needs to know which display it is rendering (in order to tell which display it should prepare and commit for), so both TryRender and Render have the information of display type. hwc->Render is called by GLContextEGL::SwapBuffers, thus the easiest way to do that is to tell GLContextEGL about the display type.

> 1. To avoid calling hwc->Init on the secondary displays. The problem is HWC
> initialization doesn't actually belong here. IIRC the main reason to do
> initialization there is because we can't get mSurface otherwise. I suspect
> we should be obtaining mSurface at the time we need it though - when
> TryRender or Render is called. Render already passes a surface, so we need
> to also pass a surface to TryRender.
Maybe we could let surface carry the info of display type, and pass a surface to TryRender. I think this would solve both the problems of initialization and render on which display with hwc.

> 2. To pass the display type to HWC when calling GLContextEGL::SwapBuffers.
> This would only happen if HWC is disabled. We can support figuring out the
> display type using only the EGLSurface, but this can be considered at a
> later point.
Display of connecting through Wifi Display uses EGLSurface only.

> 3. To determine whether we should make treat this as offscreen in
> CreateForWindow. The external displays should not be treated as offscreen,
> because they're not.
Got it.
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +667,5 @@
> >    }
> >  
> > +  int displaytype = 0;
> > +  if (composer2D) {
> > +    displaytype = (static_cast<nsWindow*>(mCompositor->GetWidget()))->GetDisplayType();
> 
> Rather than getting the display type from the window, just pass the window
> to hwc.
Sure, this is one of the TODO in the list of 'item-to-polish'.

> 
> ::: gfx/layers/opengl/CompositingRenderTargetOGL.cpp
> @@ +58,4 @@
> >      }
> >  
> >      mCompositor->PrepareViewport(mInitParams.mSize);
> > +    mGL->fScissor(0, 0, mInitParams.mSize.width, mInitParams.mSize.height);
> 
> What's this for?
ahh...it's probably a leftover in the early stage of using Wifi Display to connect to an external display, I'm not sure it is required in our latest change, let me figure it out.

Comment 16

[Shelly Lin [:shelly]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > > shelly, is there a overview design document about it?
> > 
> > It seems to exit in Bug 1116089.
> 
> I checked Bug 1116089, but there seems not exit about a diagram/document
> that talks about how Widget and gfx is going to be changed.
> 
> shelly, is there a such document existed somethere?

Hi Sotaro, we'll come up with a Wiki sooner as possible, thanks for your inputs.

Comment 17

[Sotaro Ikeda [:sotaro]]

I prefer GLContextEGL does not have a dependence to HwcComposer2D. Then created Bug 1152135.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> I prefer GLContextEGL does not have a dependence to HwcComposer2D. Then
> created Bug 1152135.

EGLSurface connection to GLContextEGL could be changed from outside by using GLContextEGL::SetEGLSurfaceOverride(). The function has a problem. Bug 1151936 is going to address it.

Comment 19

[Shelly Lin [:shelly]]

Attachment: Refactor in GonkDisplay

Comment 20

[Shelly Lin [:shelly]]

Comment on attachment 8593281 [details] [diff] [review]
Refactor in GonkDisplay

Hi mwu, I know a change to GonkDisplay involves changes to other files as well, but GonkDisplay is a pretty fundamental module so I'd like to get it discussed first. Our baseline is make it flexible of expanding features but not overly designed, try focusing on our use cases in practice with minimum changes to current.

- GonkDisplay manages an array of DisplayDevice.

Although it is possible to have multiple displays of same type connected in the same time, it is implemented as an one-on-one mapping for the reasons of 1: we don't have a use case in practice yet and 2: it doesn't sound like a huge change if we'd like to extend for this feature.

Maybe displayId is needed, Henry is investigating the expansion with nsIScreen.

Array is chosen for the reason of we won't have such many displays connected in the same time anyway.

- Move whatever is needed of the current default display to mDevices[PRIMARY]

- How should we deal with display's dpi?

Comment 21

[Shelly Lin [:shelly]]

Attachment: Refactor in Hwc

Hi Boris, thanks for your advice last time! Turns out that we don't need multiple mList for enabling multiple displays. Could you give us some feedback regarding to the changes we made here? And most importantly, we'd like to turn vsync back on for the primary display, what else changes do we need? (I'm fine with not having vsync on the external display...)

Comment 22

[Shelly Lin [:shelly]]

Comment on attachment 8594668 [details] [diff] [review]
Refactor in Hwc

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

::: widget/gonk/HwcComposer2D.h
@@ +87,5 @@
>      // Returns FALSE if the container cannot be fully rendered
>      // by this composer so nothing was rendered at all
>      bool TryRender(layers::Layer* aRoot,
> +                   bool aGeometryChanged,
> +                   int aDisplayType = 0) override;

mwu suggests we should pass in the nsWidget instead of a display type, we will address this issue after the refactor in nsWindow and nsScreenManager.

Comment 23

[Boris Chiou [:boris]]

Comment on attachment 8594668 [details] [diff] [review]
Refactor in Hwc

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

Looks OK to me. However, Sotaro updated HwcComposer2D recently, so I think you have to rebase it.

Comment 24

[Boris Chiou [:boris]]

(In reply to Shelly Lin [:shelly] from comment #21)
> Created attachment 8594668 [details] [diff] [review]
> Refactor in Hwc
> 
> Hi Boris, thanks for your advice last time! Turns out that we don't need
> multiple mList for enabling multiple displays. Could you give us some
> feedback regarding to the changes we made here? And most importantly, we'd
> like to turn vsync back on for the primary display, what else changes do we
> need? (I'm fine with not having vsync on the external display...)

Hi, Jerry,
Do you have any comment about vsync? Thanks.

Comment 25

[Shelly Lin [:shelly]]

It looks "okay" when I turn vsync back on, no crash or something, but the rendering on primary display seems laggy compare to the build without vsync.

Comment 26

[Boris Chiou [:boris]]

(In reply to Shelly Lin [:shelly] from comment #25)
> It looks "okay" when I turn vsync back on, no crash or something, but the
> rendering on primary display seems laggy compare to the build without vsync.

BTW, you can add a log (ex. printf_stderr) in HwcComposer2D::Vsync() to check the vsync signal is correct on different displays after enabling the preference.

Comment 27

[Michael Wu [:mwu]]

Comment on attachment 8593281 [details] [diff] [review]
Refactor in GonkDisplay

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

Sorry for the delay. I haven't focused too much on the details here since the general structure of the API needs to be revised. GonkDisplay is definitely the part that needs to be reviewed and nailed down first, so thanks for splitting this out.

::: widget/gonk/libdisplay/DisplayDevice.h
@@ +29,5 @@
> +
> +namespace mozilla {
> +class GonkDisplayJB;
> +
> +class DisplayDevice : public nsIDisplayDevice {

Would be nice to have nsIDisplayDevice.idl in this patch.

::: widget/gonk/libdisplay/GonkDisplay.h
@@ +114,3 @@
>  
> +protected:
> +    virtual DisplayDevice* GetDevice(const uint32_t aType) = 0;

This is the actual interface that we want to use. Adding aType to GetXdpi/GetSurfaceformat/etc. is backwards. Instead of GetGonkDisplay()->GetSurfaceformat(DISPLAY_PRIMARY), we want to do something like GetGonkDisplay()->GetDevice(DISPLAY_PRIMARY)->surfaceformat;

::: widget/gonk/libdisplay/GonkDisplayJB.h
@@ +86,2 @@
>      OnEnabledCallbackType mEnabledCallback;
> +    nsTArray<DisplayDevice> mDevices;

This can't be right. DisplayDevice is a refcounted object since it inherits from nsISupports and is expected to be used by JS code. If the JS side holds onto the DisplayDevice while the device is unplugged, it'll try to use a freed object. Instead of trying to make DisplayDevice a xpcom object, I recommend creating a separate object that copies the necessary information (or forwards to the appropriate code).

Comment 28

[Henry Chang [:hchang]]

Attachment: ScreenManager kun.png

Comment 29

[Henry Chang [:hchang]]

Attachment: Refactor of nsWindow/nsScreenGonk/nsScreenManagerGonk

Comment 30

[Henry Chang [:hchang]]

Comment on attachment 8597208 [details]
Refactor  of nsWindow/nsScreenGonk/nsScreenManagerGonk

<html>
  <head>
  <meta http-equiv="Refresh" content="0; url=https://github.com/elefant/gecko-dev/commit/34db0c050ead561b20257702110f0261d4b8916b">
  </head>
  <body>
    Redirect to the demo video on Youtube.
  </body>
</html>

Comment 31

[Henry Chang [:hchang]]

Attachment: Refactor of nsWindow/nsScreenGonk/nsScreenManagerGonk

Comment 32

[Henry Chang [:hchang]]

Attachment: nsWindow/nsScreen/nsScreenManager refactoring

Comment 33

[Henry Chang [:hchang]]

Attachment: nsWindow-refactor.diff

Comment 34

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #33)
> Created attachment 8598468 [details] [diff] [review]
> nsWindow-refactor.diff

This patch is to carry out attachment 8597207 [details]. 

:wmu, Sotaro, can I have your feedback regarding this change? The purpose
might be similar to Bug 1157874 but with a more generic extension. Thanks!

The primary goal is to take “multiple screens” case into account. To achieve the goals, following 
are the main tasks:

1) Decompose nsWindow and GonkDisplay. nsWindow no longer knows GonkDisplay. 
   Instead, a nsWindow would be associated with one nsIScreen (nsScreenGonk) 
   by screen id.

2) Extend nsScreenManagerGonk to be able to add/remove screens. Threes APIs     
   (nsScreenGonkManager::AddScreen/Remove/AddPrimaryScreens) are added to nsScreenManagerGonk. 
   Other modules could use these APIs to add/remvoe screens whenever needed. 
   For example, WifiDisplayManager will call nsScreenManagerGonk::AddScreen 
   when a wifi display sink device is connected.

3) Complete nsScreenManagerGonk::ScreenForId/ScreenForNativeWidget since they 
   are not trivial anymore.

The steps of adding a new screen and opening a new window on the new screen are

1. When a physical (e.g. HDMI) or virtual (e.g. Wifi Display) screen is connected, 
   call nsScreenManagerGonk::AddScreen.

2. nsScreenManagerGonk::AddDisplay internally calls GonkDisplay::AddDisplay first 
   to request gonk layer to create a new display context.

3. GonkDisplay then creates a DisplayDevice by the given type and graphic buffer 
   if needed.

4. GonkDisplay returns an id to nsScreenManagerGonk to acquire necessary information 
   for creating a nsScreenGonk.

5. nsScreenManagerGonk creates nsScreenGonk by the information retrieving from GonkDisplay.

6. Fire ‘display-change’ event with subject “nsIDisplayDevice”.

7. On receiving ‘display-change’ event in shell.js, call nsIWindowWatcher.openWindow 
   with display type (or id. TBD) to create a top level window. 

After nsWindow is created, all the information can be obtained from the screen that 
the window is assocated with, such as native window, natural bounds, etc.

Comment 35

[Sotaro Ikeda [:sotaro]]

(In reply to Henry Chang [:henry] from comment #34)
> (In reply to Henry Chang [:henry] from comment #33)
> > Created attachment 8598468 [details] [diff] [review]
> > nsWindow-refactor.diff
> 
> This patch is to carry out attachment 8597207 [details]. 
> 
> :wmu, Sotaro, can I have your feedback regarding this change? The purpose
> might be similar to Bug 1157874 but with a more generic extension. Thanks!

Bug 1157874's scope is intentionally limited to one thing "Remove screen related globals". I personally like incremental approach as I wrote in the past. One bug should limit its focus to one thing. Then a bug assignee could focus to one thing and could polish an implementation. From reviewer's point of view, it is easier to review.

"generic extension" makes bugs focus vague. It could have multiple problem. I think that it might be better to split more concrete and scope explicit bugs.

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Henry Chang [:henry] from comment #33)
> Created attachment 8598468 [details] [diff] [review]
> nsWindow-refactor.diff

With which code, is the patch created? The base code also seems to have some modification from m-c.

Comment 37

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> 
> "generic extension" makes bugs focus vague. It could have multiple problem.
> I think that it might be better to split more concrete and scope explicit
> bugs.

By limiting the scope of bug. We could talk more detailed things in the bugs. For example, in your patch nsWindow holds screen id. But in Bug 1157874, nsWindow holds nsScreenGonk.

Comment 38

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> Bug 1157874's scope is intentionally limited to one thing "Remove screen
> related globals". I personally like incremental approach as I wrote in the
> past.

For example, as a next step, we could remove "dependency to GonkDisplay of nsWindow".

Comment 39

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Shelly Lin [:shelly] from comment #25)
> It looks "okay" when I turn vsync back on, no crash or something, but the
> rendering on primary display seems laggy compare to the build without vsync.

It's related to bug 1155797.
I'm trying to fix that at nexus 5 device

Comment 40

[Henry Chang [:hchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> (In reply to Henry Chang [:henry] from comment #34)
> > (In reply to Henry Chang [:henry] from comment #33)
> > > Created attachment 8598468 [details] [diff] [review]
> > > nsWindow-refactor.diff
> > 
> > This patch is to carry out attachment 8597207 [details]. 
> > 
> > :wmu, Sotaro, can I have your feedback regarding this change? The purpose
> > might be similar to Bug 1157874 but with a more generic extension. Thanks!
> 
> Bug 1157874's scope is intentionally limited to one thing "Remove screen
> related globals". I personally like incremental approach as I wrote in the
> past. One bug should limit its focus to one thing. Then a bug assignee could
> focus to one thing and could polish an implementation. From reviewer's point
> of view, it is easier to review.
> 
> "generic extension" makes bugs focus vague. It could have multiple problem.
> I think that it might be better to split more concrete and scope explicit
> bugs.

Hi Sotaro,

Thanks for your feedback first!

The patch is not based on m-c but based on the WIP patches attached in this bug.
Sorry for confusion.

I totally agree the incremental approach. What I would like to present is the 
design concept but not the code. (Sorry again I forgot to mention this.) 
The code is the proof of concept and even testable. (I tested it and it works 
perfectly with all multi-screen scenario.)

As you can see in the patch, I almost rewrite nsScreenGonk/nsWindow/nsScreenManagerGonk
and the changes are tightly coupled. (I mean the "changes" are tightly coupled but not
the modules.) In order to avoid any unnecessary intermediate state as possible,
I tend to use one bug to address the redesign.

I am pretty willing to create another bug to do the redesign but I'd still
like to let relevant reviewers have a look how the enhanced nsScreenManagerGonk
deals with multiple screens/top level windows case.

Thanks!

Comment 41

[Shelly Lin [:shelly]]

Attachment: refactor-gonkdisplay-v2.diff

Comment 42

[Shelly Lin [:shelly]]

Comment on attachment 8599079 [details] [diff] [review]
refactor-gonkdisplay-v2.diff

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

Hi mwu, thanks for your comments last time, it might look conflicting with attachment 8598468 [details] [diff] [review], but once the skeleton is settled, rebasing and separating them into parts are relatively easy.

Comment 43

[Sotaro Ikeda [:sotaro]]

(In reply to Henry Chang [:henry] from comment #40)
> (In reply to Sotaro Ikeda [:sotaro] from comment #35)
> > (In reply to Henry Chang [:henry] from comment #34)
> > > (In reply to Henry Chang [:henry] from comment #33)
> > > > Created attachment 8598468 [details] [diff] [review]
> > > > nsWindow-refactor.diff
> > > 
> > > This patch is to carry out attachment 8597207 [details]. 
> > > 
> > > :wmu, Sotaro, can I have your feedback regarding this change? The purpose
> > > might be similar to Bug 1157874 but with a more generic extension. Thanks!
> > 
> > Bug 1157874's scope is intentionally limited to one thing "Remove screen
> > related globals". I personally like incremental approach as I wrote in the
> > past. One bug should limit its focus to one thing. Then a bug assignee could
> > focus to one thing and could polish an implementation. From reviewer's point
> > of view, it is easier to review.
> > 
> > "generic extension" makes bugs focus vague. It could have multiple problem.
> > I think that it might be better to split more concrete and scope explicit
> > bugs.
> 
> Hi Sotaro,
> 
> Thanks for your feedback first!
> 
> The patch is not based on m-c but based on the WIP patches attached in this
> bug.
> Sorry for confusion.
> 
> I totally agree the incremental approach. What I would like to present is
> the 
> design concept but not the code. (Sorry again I forgot to mention this.) 
> The code is the proof of concept and even testable. (I tested it and it
> works 
> perfectly with all multi-screen scenario.)
> 
> As you can see in the patch, I almost rewrite
> nsScreenGonk/nsWindow/nsScreenManagerGonk
> and the changes are tightly coupled. (I mean the "changes" are tightly
> coupled but not
> the modules.) In order to avoid any unnecessary intermediate state as
> possible,
> I tend to use one bug to address the redesign.

I disagree about it. You could add gonk's DisplayDevice class without adding multiple display support.

Comment 44

[Sotaro Ikeda [:sotaro]]

(In reply to Henry Chang [:henry] from comment #28)
> Created attachment 8597207 [details]
> ScreenManager kun.png

nsWindow holds screen id, but I prefer to hold nsScreen gonk. It might be better "DisplayDevice" is like "GonkDisplayDevice" or "GonkDisplay::DisplayDevice", because it is gonk specific.

It might be better that GonkDisplay manages only hwc displays, since GonkDisplay is similar to android's HWComposer. And nsScreenManagerGonk becomes more similar to android's SurfaceFlinger's DisplayDevices management. From this point of view, it might be better that nsScreenManagerGonk assigns display id.

When shell.js tries to open a window, if the display was already unplugged, how does the framework handle the case?

Is the diagram cared about the display mirroring?

Comment 45

[Shelly Lin [:shelly]]

(In reply to Sotaro Ikeda [:sotaro] from comment #44)
> (In reply to Henry Chang [:henry] from comment #28)
> > Created attachment 8597207 [details]
> > ScreenManager kun.png
> 
> nsWindow holds screen id, but I prefer to hold nsScreen gonk. It might be
> better "DisplayDevice" is like "GonkDisplayDevice" or
> "GonkDisplay::DisplayDevice", because it is gonk specific.
> 
I don't think the rename is necessary, it's already under the gonk specific folder.

> It might be better that GonkDisplay manages only hwc displays, since
> GonkDisplay is similar to android's HWComposer. And nsScreenManagerGonk
> becomes more similar to android's SurfaceFlinger's DisplayDevices
> management. From this point of view, it might be better that
> nsScreenManagerGonk assigns display id.
> 
Sounds reasonable.

> When shell.js tries to open a window, if the display was already unplugged,
> how does the framework handle the case?
>
WindowWatcher.openWindow returns null if it fails to open a window

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWindowWatcher#openWindow%28%29
https://dxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#735

And shell.js should handle this kind of error in any situation.

> Is the diagram cared about the display mirroring?
Yes, but mirroring is not in our first stage plan.

Comment 46

[Sotaro Ikeda [:sotaro]]

(In reply to Shelly Lin [:shelly] from comment #45)
> (In reply to Sotaro Ikeda [:sotaro] from comment #44)
> > (In reply to Henry Chang [:henry] from comment #28)
> > > Created attachment 8597207 [details]
> > > ScreenManager kun.png
> > 
> > nsWindow holds screen id, but I prefer to hold nsScreen gonk. It might be
> > better "DisplayDevice" is like "GonkDisplayDevice" or
> > "GonkDisplay::DisplayDevice", because it is gonk specific.
> > 
> I don't think the rename is necessary, it's already under the gonk specific
> folder.

I feel that just DisplayDevice is too generic name. If it is gonk specific class, it should not have a such too generic name that might conflict in future. It could prevent a possibility of unnecessary trouble.

I recently fixed the crash that caused by a naming conflict. It is Bug 1158692. In this case, a class that caused the problem was also in gonk specific folder.

Comment 47

[Michael Wu [:mwu]]

Comment on attachment 8599079 [details] [diff] [review]
refactor-gonkdisplay-v2.diff

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

It looks like Sotaro already made some comments. I agree with moving things out of GonkDisplay/libdisplay and into nsScreenManager where possible. At minimum, DisplayInfo/nsIDisplayInfo should not be in libdisplay, since the caller can set that up after adding/removing the display. If nsScreenManager manages these DisplayDevice objects, we can have GonkDisplay create them but not hold on to them. (so you would get GonkDisplay::NewDisplay rather than GonkDisplay::(Add|Remove)Display)

The odds of a name collision is actually lower for things in libdisplay since it isn't a part of libxul, but making the name less generic (GonkDisplayDevice) seems reasonable.

Things otherwise look reasonable here. Let's see how this patch looks with those changes.

::: widget/gonk/libdisplay/GonkDisplay.h
@@ +85,3 @@
>  
>      /**
> +     * Remove an existed DisplayDevice from the list of connceted displays.

typo

Comment 48

[Shelly Lin [:shelly]]

(In reply to Michael Wu [:mwu] from comment #47)
> Comment on attachment 8599079 [details] [diff] [review]
> refactor-gonkdisplay-v2.diff
> 
> Review of attachment 8599079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like Sotaro already made some comments. I agree with moving things
> out of GonkDisplay/libdisplay and into nsScreenManager where possible. At
> minimum, DisplayInfo/nsIDisplayInfo should not be in libdisplay, since the
> caller can set that up after adding/removing the display. If nsScreenManager
> manages these DisplayDevice objects, we can have GonkDisplay create them but
> not hold on to them. (so you would get GonkDisplay::NewDisplay rather than
> GonkDisplay::(Add|Remove)Display)
> 
Okay, make sense to me too. Since we have nsScreenManager manage screens and its connected displays, we shall remove GonkDisplay->GetDevice, and let widget be the interface of accessing screen/display information.

Once bug 1138288 is ready, we will start re-basing to what is latest then.

Comment 49

[Henry Chang [:hchang]]

Attachment: Bug1138287-Part1-nsWindow.patch

Comment 50

[Henry Chang [:hchang]]

Attachment: Bug1138287-Part2-GFX.patch

Comment 51

[Henry Chang [:hchang]]

Comment on attachment 8607443 [details] [diff] [review]
Bug1138287-Part1-nsWindow.patch

Hi mwu,

These 2 patches are created based on Sotaro's previous refactor on rendering part 
and my previous refactor on nsScreenManager part. 

The main changes of this patch are:

1) Enhance GonkDispay so that it could create native info for nsScreenManagerGonk
   to add a new screen by given type.

2) Extract nsScreenManagerGonk/nsScreenGonk implementation to a separate file.
   Only few change is made for nsScreen part. 

3) Extend nsWindow interfaces for the need of rendering. You can see how it
   works in the GFX part patch.

Thanks!

Comment 52

[Henry Chang [:hchang]]

Comment on attachment 8607444 [details] [diff] [review]
Bug1138287-Part2-GFX.patch

This patch is for rendering based on patch part 1. Basically we just change to
obtain the DisplaySurface from nsIWidget rather than from GonkDisplay singleton.
For wifi display rendering, the task would be done by just eglSwapBuffers.

Thanks!

Comment 53

[Michael Wu [:mwu]]

Comment on attachment 8607443 [details] [diff] [review]
Bug1138287-Part1-nsWindow.patch

Do you mind splitting the move to a separate file into its own patch? It's hard to see exactly what you changed in ScreenManager otherwise.

Comment 54

[Henry Chang [:hchang]]

(In reply to Michael Wu [:mwu] from comment #53)
> Comment on attachment 8607443 [details] [diff] [review]
> Bug1138287-Part1-nsWindow.patch
> 
> Do you mind splitting the move to a separate file into its own patch? It's
> hard to see exactly what you changed in ScreenManager otherwise.

Sure! Do you need me to file another bug to do that first?
Or just making that kind of patch is fine to you?

Thanks!

Comment 55

[Michael Wu [:mwu]]

Just do it in here.

Comment 56

[Shelly Lin [:shelly]]

Comment on attachment 8607443 [details] [diff] [review]
Bug1138287-Part1-nsWindow.patch

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +417,5 @@
> +                     GRALLOC_USAGE_HW_COMPOSER);
> +
> +        data.mNativeWindow = win;
> +        //FIXME!!
> +        //data.mXdpi = values[2] / 1000.f;

We get 0 from values[2] when query for external display.

::: widget/gonk/nsWindow.h
@@ +126,5 @@
> +    bool IsOffScreen();
> +    nsScreenGonk* GetScreen();
> +    int GetPrevDispAcquireFd();
> +
> +    float xdpi;

xdpi can be removed since no one is using it.

Comment 57

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607443 [details] [diff] [review]
Bug1138287-Part1-nsWindow.patch

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

::: widget/gonk/nsWindow.cpp
@@ +822,1 @@
>  {

The function name is not explicit what it means.

Comment 58

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607444 [details] [diff] [review]
Bug1138287-Part2-GFX.patch

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

The code under "/gfx/gl/" needs review of :jgilbert.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +787,4 @@
>      }
>  
>      SurfaceCaps caps = SurfaceCaps::Any();
> +    bool isoffscreen = (static_cast<nsWindow*>(aWidget))->IsOffScreen();

The code here is not specific to gonk. IsOffScreen() is defined only on gonk nsWindow. And I do not think that it is necessary to call the function here. The meaning of nsWindow::IsOffScreen() seems different from GLContext::IsOffscreen(). In GLContext, it seems to mean that rendering result to GL_FRAMEBUFFER is automatically used as screen rendering. If it is correct, in this context, DISPLAY_VIRTUAL should be on-screen, because all rendering results is automatically forwarded to virtual display consumer.

@@ +791,3 @@
>      nsRefPtr<GLContextEGL> glContext =
>          GLContextEGL::CreateGLContext(caps,
> +                                      nullptr, isoffscreen,

As the above comment, all GLContext for nsWindow seems to be on screen on GLContext.

Comment 59

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607444 [details] [diff] [review]
Bug1138287-Part2-GFX.patch

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1298,5 @@
>      return;
>    }
>  
> +  nsWindow* window = static_cast<nsWindow*>(mWidget);
> +  if (window->IsOffScreen()) {

This seems not correct. We need to actually check here is that DisplaySurface of nsWindow is rendered by using HWC.

Comment 60

[Sotaro Ikeda [:sotaro]]

It seems better to remove nsWindow::IsOffScreen(). And similar capability should be in nsScreenGonk. But even in nsScreenGonk, I am not fun of using IsOffScreen(), because the meaning is not clear. What we want to check is if DisplaySurfac is rendered by HWC or if nsScreenGonk is built-in local display.

The name of "built in display" is also used in android. The meaning seems clear than off screen.
http://androidxref.com/5.1.0_r1/xref/frameworks/base/services/core/java/com/android/server/display/LocalDisplayAdapter.java#47

Comment 61

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607444 [details] [diff] [review]
Bug1138287-Part2-GFX.patch

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

::: widget/gonk/HwcComposer2D.cpp
@@ +251,5 @@
> +HwcComposer2D::Hotplug(int aDisplay, int aConnected)
> +{
> +    nsRefPtr<nsScreenManagerGonk> screenManager = nsScreenManagerGonk::GetInstance();
> +    if (aConnected) {
> +        screenManager->AddScreen(GonkDisplay::DISPLAY_EXTERNAL);

Isn't it necessary to call this function on main thread?

Comment 62

[Sotaro Ikeda [:sotaro]]

By the way, the number of displays that can be handled by hwc is depended by hwc version. 

http://androidxref.com/5.1.0_r1/xref/frameworks/native/services/surfaceflinger/DisplayHardware/HWComposer.cpp#154

Comment 63

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607443 [details] [diff] [review]
Bug1138287-Part1-nsWindow.patch

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

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +603,5 @@
> +    NotifyDisplayChange(info);
> +}
> +
> +void
> +nsScreenManagerGonk::RemoveScreen(GonkDisplay::DisplayType aDisplayType)

Doesn't the argument prevent concurrent multiple VirtualDisplaySurfaces usage, does it?

Comment 64

[Henry Chang [:hchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #58)
> Comment on attachment 8607444 [details] [diff] [review]
> Bug1138287-Part2-GFX.patch
> 
> Review of attachment 8607444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code under "/gfx/gl/" needs review of :jgilbert.
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +787,4 @@
> >      }
> >  
> >      SurfaceCaps caps = SurfaceCaps::Any();
> > +    bool isoffscreen = (static_cast<nsWindow*>(aWidget))->IsOffScreen();
> 
> The code here is not specific to gonk. IsOffScreen() is defined only on gonk
> nsWindow. And I do not think that it is necessary to call the function here.
> The meaning of nsWindow::IsOffScreen() seems different from
> GLContext::IsOffscreen(). In GLContext, it seems to mean that rendering
> result to GL_FRAMEBUFFER is automatically used as screen rendering. If it is
> correct, in this context, DISPLAY_VIRTUAL should be on-screen, because all
> rendering results is automatically forwarded to virtual display consumer.
> 
> @@ +791,3 @@
> >      nsRefPtr<GLContextEGL> glContext =
> >          GLContextEGL::CreateGLContext(caps,
> > +                                      nullptr, isoffscreen,
> 
> As the above comment, all GLContext for nsWindow seems to be on screen on
> GLContext.

Hi Sotaro san,

It seems the "off screen" flag is not needed for the latest 
GLContextEGL::SwapBuffers() since it always calls sEGLLibrary.fSwapBuffers
and it was the only reason we want it to be "off screen"
in the first place.

Thanks!

Comment 65

[Sotaro Ikeda [:sotaro]]

> 
> Hi Sotaro san,
> 
> It seems the "off screen" flag is not needed for the latest 
> GLContextEGL::SwapBuffers() since it always calls sEGLLibrary.fSwapBuffers
> and it was the only reason we want it to be "off screen"
> in the first place.
> 
> Thanks!

Yup, it was removed by Bug 1152135 as a preparation for multiple display support.

Comment 66

[Henry Chang [:hchang]]

Attachment: Part 0 - Extract nsScreenGonk/nsScreenManagerGonk to a separate file

Comment 67

[Henry Chang [:hchang]]

Attachment: Part 1 - Enhance nsWindow/nsScreenGonk for multiple display rendering

Comment 68

[Henry Chang [:hchang]]

Hi mwu,

I've split the move to a separate file into its own patch (Part 0). Could you please do
the review again? Thanks!

Comment 69

[Michael Wu [:mwu]]

Comment on attachment 8609285 [details] [diff] [review]
Part 1 - Enhance nsWindow/nsScreenGonk for multiple display rendering

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

General approach looks fine. I'm not sure about moving nsScreenManagerGonk to a separate file, but we can consider that separately from the changes here.

::: widget/gonk/libdisplay/GonkDisplay.h
@@ +88,5 @@
>       */
>      virtual int GetPrevDispAcquireFd() = 0;
>  
> +    virtual NativeData GetNativeData(uint32_t aDisplayType,
> +                                     android::IGraphicBufferProducer* aProducer = nullptr) = 0;

With this new API, we can get rid of a bunch of functions in here, right?

Looks like we can remove SetDispReleaseFd, GetPrevDispAcquireFd, GetDispSurface, and GetNativeWindow.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +397,5 @@
> +        sp<BufferQueue> consumer = new BufferQueue(true, mAlloc);
> +        sp<IGraphicBufferProducer> producer = consumer;
> +#else
> +        sp<BufferQueue> consumer = new BufferQueue(true, mAlloc);
> +#endif

IIRC this consumer creation logic was moved into its own function, so you can use that instead of copying it here.

@@ +413,5 @@
> +                     NATIVE_WINDOW_SET_BUFFER_COUNT, 2);
> +        win->perform(win, NATIVE_WINDOW_SET_USAGE,
> +                     GRALLOC_USAGE_HW_FB |
> +                     GRALLOC_USAGE_HW_RENDER |
> +                     GRALLOC_USAGE_HW_COMPOSER);

I think we can actually skip the perform calls here. IIRC they're required in order to render the boot animation, but we're not going to play the boot animation on external displays.

@@ +417,5 @@
> +                     GRALLOC_USAGE_HW_COMPOSER);
> +
> +        data.mNativeWindow = win;
> +        //FIXME!!
> +        //data.mXdpi = values[2] / 1000.f;

Is values[2] invalid?

@@ +424,5 @@
> +                                                      surfaceformat, consumer);
> +
> +    } else if (aDisplayType == DISPLAY_VIRTUAL) {
> +        data.mNativeWindow = new Surface(aProducer);
> +        data.mDisplaySurface = nullptr;

VirtualDisplaySurface

::: widget/gonk/libdisplay/GonkDisplayJB.h
@@ +54,5 @@
>  
>      bool Post(buffer_handle_t buf, int fence);
>  
> +    virtual NativeData GetNativeData(uint32_t aDisplayType,
> +                                     android::IGraphicBufferProducer* aProducer = nullptr);

There's no corresponding change in GonkDisplayICS.h - this will break on ICS.

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +136,5 @@
>          mPhysicalScreenRotation = atoi(propValue) / 90;
>      }
>  
> +    mColorDepth = SurfaceFormatToColorDepth(mSurfaceFormat);
> +    mIsBlanked = aDisplayType == GonkDisplay::DISPLAY_PRIMARY;

We shouldn't need to keep track of blanking status. Something is wrong.

@@ +517,5 @@
>      return aDisplayType;
>  }
>  
> +namespace
> +{

nit: put { on the same line as namespace.

@@ +551,5 @@
> +private:
> +    virtual ~DisplayInfo() {}
> +
> +    uint32_t mId;
> +    int mFlags;

Slightly overengineered. We can just have a "bool mConnected" here.

@@ +556,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(DisplayInfo, nsIDisplayInfo, nsISupports)
> +
> +class NotifyTask : public nsIRunnable

Use nsRunnable

@@ +557,5 @@
> +
> +NS_IMPL_ISUPPORTS(DisplayInfo, nsIDisplayInfo, nsISupports)
> +
> +class NotifyTask : public nsIRunnable
> +{

nit: put { on the same line as class

@@ +571,5 @@
> +    virtual ~NotifyTask() {}
> +
> +    NS_IMETHOD Run()
> +    {
> +        nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-service;1"));

Try mozilla::services::GetObserverService()

@@ +578,5 @@
> +        }
> +        return os->NotifyObservers(mDisplayInfo, "display-changed", nullptr);
> +    }
> +private:
> +    nsCOMPtr<nsIDisplayInfo> mDisplayInfo;

We can just store the concrete type here.

@@ +618,5 @@
>          }
>      }
> +
> +    nsIDisplayInfo* info = new DisplayInfo(screenId, DisplayInfo::REMOVED);
> +    NotifyDisplayChange(info);

Passing the screenId and flags instead of nsIDisplayInfo to NotifyDisplayChange would simplify things a bit.

::: widget/gonk/nsWindow.cpp
@@ +52,4 @@
>  #include "mozilla/layers/InputAPZContext.h"
>  #include "mozilla/MouseEvents.h"
>  #include "mozilla/TouchEvents.h"
> +#include "nsThreadUtils.h"

What are all these includes for? Do they belong to the patch that moves the ScreenManager?

::: widget/gonk/nsWindow.h
@@ +125,5 @@
>  
> +    nsScreenGonk* GetScreen();
> +    int GetPrevDispAcquireFd();
> +
> +    int32_t surfaceformat;

This is unnecessary when GetScreen() is available.

Comment 70

[Shelly Lin [:shelly]]

Attachment: Part 0: Split implementation of screen into its own cpp file

Comment 71

[Shelly Lin [:shelly]]

Attachment: Part 1: Extend widget gonk to support multi-screen

Comment 72

[Shelly Lin [:shelly]]

Attachment: Part 2: Adapt gfx to the new changes

Comment 74

[Shelly Lin [:shelly]]

Attachment: Part 1: Extend widget gonk to support multi-screen

Comment 75

[Shelly Lin [:shelly]]

Attachment: Part 2: Adapt gfx to the new changes

Comment 76

[Shelly Lin [:shelly]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

Hi mwu, I've addressed most of your feedback, please find my comments inline. Thanks!

::: widget/gonk/libdisplay/GonkDisplay.h
@@ +48,5 @@
> +    };
> +
> +    struct NativeData {
> +        android::sp<ANativeWindow> mNativeWindow;
> +        android::sp<android::DisplaySurface> mDisplaySurface;

I think we can file a follow-up bug when we are actually using the VirtualDisplaySurface...

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +379,5 @@
> +        };
> +        mHwc->getDisplayAttributes(mHwc, aDisplayType, 0, attrs, values);
> +        int width = values[0];
> +        int height = values[1];
> +        // FIXME!! values[2] returns 0 for display type of external.

Haven't tested on other HDMI displays, but at least it returned 0 for my monitor. Should we make it fall back to a "default" value if it returns 0?

Comment 77

[Shelly Lin [:shelly]]

Comment on attachment 8611088 [details] [diff] [review]
Part 2: Adapt gfx to the new changes

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

Hi :jgilbert, sotaro suggested that you might want to take a look at the changes in gfx/gl, which is file "GLContextProviderEGL.cpp", thanks a lot!

Comment 78

[Shelly Lin [:shelly]]

Comment on attachment 8611088 [details] [diff] [review]
Part 2: Adapt gfx to the new changes

Hi mwu, these are mostly interface changes, and I think this part should be merged into Part 1 when it comes to checked-in right?

Comment 79

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8611088 [details] [diff] [review]
Part 2: Adapt gfx to the new changes

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

GLContextProviderEGL LGTM

Comment 80

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8611088 [details] [diff] [review]
Part 2: Adapt gfx to the new changes

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

GLContextProviderEGL LGTM
Someone needs to review the rest, though.

Comment 81

[Michael Wu [:mwu]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

This looks pretty close to me. Also request review from Sotaro for the next patch - I'd like more eyes on the HwcComposer2D changes. Thanks!

::: widget/gonk/HwcComposer2D.cpp
@@ +877,5 @@
>          LOGE("Multiple hwc prepare calls!");
>      }
> +
> +    if (!mScreen->IsPrimaryScreen()) {
> +        mHwc->blank(mHwc, displaytype, 0);

We shouldn't call blank on every prepare call. This belongs elsewhere.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +379,5 @@
> +        };
> +        mHwc->getDisplayAttributes(mHwc, aDisplayType, 0, attrs, values);
> +        int width = values[0];
> +        int height = values[1];
> +        // FIXME!! values[2] returns 0 for display type of external.

Weird.

A default value is fine for now (75?), but the device should have access to the size of the screen. Worth investigating later.

@@ +383,5 @@
> +        // FIXME!! values[2] returns 0 for display type of external.
> +        //int xdpi = values[2] / 1000.f;
> +        data.mXdpi = 81.5;
> +        CreateSurface(data.mNativeWindow, data.mDisplaySurface, width, height);
> +    } else if (aDisplayType == DISPLAY_VIRTUAL) {

This isn't actually implemented yet. Just crash if we hit this path.

Comment 82

[Shelly Lin [:shelly]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

Hi sotaro, would you take a look at the patch? Especially the blank issue in HwcComposer2D, from my observation, I have to blank the external display at least once at the stage of prepare, it hangs if I blank it at hotplug detection, any suggestion on that? Thank you very much!

Comment 83

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

::: widget/gonk/HwcComposer2D.cpp
@@ +784,5 @@
>              // GPU or partial OVERLAY Composition
>              return false;
>          } else if (blitComposite) {
>              // BLIT Composition, flip DispSurface target
>              GetGonkDisplay()->UpdateDispSurface(mDpy, mSur);

This swap main display's buffer. If this function is called on another display. Buffer state becomes inconsistent.

@@ +810,4 @@
>  {
>      // HWC module does not exist or mList is not created yet.
>      if (!mHwc || !mList) {
>          return GetGonkDisplay()->SwapBuffers(mDpy, mSur);

This swaps main display's buffer. when we support multiple display, mHwc should exist. But mList could happen even on another display. Bug 1144012 is going to remove mList case. But it might take time until Bug 1144012 is fixed.

@@ +877,5 @@
>          LOGE("Multiple hwc prepare calls!");
>      }
> +
> +    if (!mScreen->IsPrimaryScreen()) {
> +        mHwc->blank(mHwc, displaytype, 0);

Directly call this function seems not good. On android, blank() is called in HWComposer::setPowerMode().And on v1.4 hwc_hal, mHwc->setPowerMode() is called instead.
 http://androidxref.com/5.1.0_r1/xref/frameworks/native/services/surfaceflinger/DisplayHardware/HWComposer.cpp#777

On android, the setPowerMode is called from LocalDisplayAdapter from the comment, this function seems to take very long time until complete.
http://androidxref.com/5.1.0_r1/xref/frameworks/base/services/core/java/com/android/server/display/LocalDisplayAdapter.java#235

on gecko, related thing is handled by GonkDisplay:SetEnabled(), it seems better to extend this function for multiple displays.

In future, it seems better that additinal screen on/off could be controlled from hal::SetScreenEnabled().
 https://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp#757

@@ +889,5 @@
>  HwcComposer2D::Commit()
>  {
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };
> +    uint32_t displaytype = mScreen->GetDisplayType();
> +    displays[displaytype] = mList;

This set a list to only one display, other display's contents are null even when other hwc displays exist. Is there no problem to other display's rendering result on your test?
From qcom's hwc hal code, each display's rendering seems mostly independent.

@@ +972,5 @@
>      if (!mHwc) {
>          return false;
>      }
>  
> +    mScreen = (static_cast<nsWindow*>(aWidget))->GetScreen();

mScreen is actually valid only during Render() and TryRenderWithHwc(). To avoid unintended problems, mScreen should be set to nullptr outside of the function. RAII helper class could be used for it.

::: widget/gonk/libdisplay/GonkDisplayICS.cpp
@@ +201,5 @@
>  
>  void
>  GonkDisplayICS::UpdateDispSurface(EGLDisplay dpy, EGLSurface sur)
>  {
> +    StopBootAnimation();

UpdateDispSurface() is not used on ICS. It seems better to change this function to emply and put MOZ_CRASH().

@@ +215,5 @@
> +GonkDisplay::NativeData
> +GonkDisplayICS::GetNativeData(uint32_t aDisplayType,
> +                              android::IGraphicBufferProducer* aProducer)
> +{
> +    NativeData data;

ICS gonk supports only one display. It seems better to add MOZ_ASSERT(aDisplayType == DISPLAY_PRIMARY) here.

Comment 84

[Sotaro Ikeda [:sotaro]]

Additional comment. 

> HwcComposer2D::SetEGLInfo() need to care about display type.

Comment 85

[Sotaro Ikeda [:sotaro]]

(In reply to Shelly Lin [:shelly] from comment #82)
> Comment on attachment 8611087 [details] [diff] [review]
> Part 1: Extend widget gonk to support multi-screen
> 
> Review of attachment 8611087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi sotaro, would you take a look at the patch? Especially the blank issue in
> HwcComposer2D, from my observation, I have to blank the external display at
> least once at the stage of prepare, it hangs if I blank it at hotplug
> detection, any suggestion on that? Thank you very much!

I am wondering if thread context affect to it. In "hotplug detection" case, from which thread was the function called? It might be necessary from compositor thread.

Comment 86

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

::: widget/gonk/libdisplay/GonkDisplayICS.cpp
@@ +201,5 @@
>  
>  void
>  GonkDisplayICS::UpdateDispSurface(EGLDisplay dpy, EGLSurface sur)
>  {
> +    StopBootAnimation();

Sorry correction to my comment. MOZ_CRASH() is not necessary, just make the function is enough.

Comment 87

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

::: widget/gonk/HwcComposer2D.cpp
@@ +940,5 @@
>  HwcComposer2D::Reset()
>  {
>      LOGD("hwcomposer is already prepared, reset with null set");
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };
> +    mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays);

This comment is unrelated to the patch.
I checked the codeaurora's hwc_hal implementation. When hwc_display_contents_1_t is nullptr, hwc_hal do nothing. Therefore, this does not work as a reset.

Comment 88

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #85)
> (In reply to Shelly Lin [:shelly] from comment #82)
> > Comment on attachment 8611087 [details] [diff] [review]
> > Part 1: Extend widget gonk to support multi-screen
> > 
> > Review of attachment 8611087 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi sotaro, would you take a look at the patch? Especially the blank issue in
> > HwcComposer2D, from my observation, I have to blank the external display at
> > least once at the stage of prepare, it hangs if I blank it at hotplug
> > detection, any suggestion on that? Thank you very much!
> 
> I am wondering if thread context affect to it. In "hotplug detection" case,
> from which thread was the function called? It might be necessary from
> compositor thread.

This is a big difference from android's SurfaceFlinger. multi thread access to hwc hal on gonk might cause a problem, because on android, hwc hal is accessed only from SurfaceFlinger thread.

Comment 89

[Shelly Lin [:shelly]]

Comment on attachment 8611087 [details] [diff] [review]
Part 1: Extend widget gonk to support multi-screen

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

::: widget/gonk/HwcComposer2D.cpp
@@ +784,5 @@
>              // GPU or partial OVERLAY Composition
>              return false;
>          } else if (blitComposite) {
>              // BLIT Composition, flip DispSurface target
>              GetGonkDisplay()->UpdateDispSurface(mDpy, mSur);

Does that mean HwcComposer2D should maintain a separate set of mDpy and mSur for external display?

@@ +810,4 @@
>  {
>      // HWC module does not exist or mList is not created yet.
>      if (!mHwc || !mList) {
>          return GetGonkDisplay()->SwapBuffers(mDpy, mSur);

When it comes to the stage of rendering external display, mHwc is not null (mHal->HasHwc() returns true) and mList should be initialized. Therefore |GetGonkDisplay()->SwapBuffers(mDpy, mSur)| will not be called on non-primary display, and I don't think Bug 1144012 affects what is changed here.

@@ +889,5 @@
>  HwcComposer2D::Commit()
>  {
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };
> +    uint32_t displaytype = mScreen->GetDisplayType();
> +    displays[displaytype] = mList;

I didn't see any problem on my test though.

@@ +940,5 @@
>  HwcComposer2D::Reset()
>  {
>      LOGD("hwcomposer is already prepared, reset with null set");
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };
> +    mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays);

If so, does that mean the original implementation does not work as reset either?

Comment 90

[Sotaro Ikeda [:sotaro]]

(In reply to Shelly Lin [:shelly] from comment #89)
> Comment on attachment 8611087 [details] [diff] [review]
> Part 1: Extend widget gonk to support multi-screen
> 
> Review of attachment 8611087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +784,5 @@
> >              // GPU or partial OVERLAY Composition
> >              return false;
> >          } else if (blitComposite) {
> >              // BLIT Composition, flip DispSurface target
> >              GetGonkDisplay()->UpdateDispSurface(mDpy, mSur);
> 
> Does that mean HwcComposer2D should maintain a separate set of mDpy and mSur
> for external display?

Yes.

> 
> @@ +810,4 @@
> >  {
> >      // HWC module does not exist or mList is not created yet.
> >      if (!mHwc || !mList) {
> >          return GetGonkDisplay()->SwapBuffers(mDpy, mSur);
> 
> When it comes to the stage of rendering external display, mHwc is not null
> (mHal->HasHwc() returns true) and mList should be initialized. Therefore
> |GetGonkDisplay()->SwapBuffers(mDpy, mSur)| will not be called on
> non-primary display, and I don't think Bug 1144012 affects what is changed
> here.

Current HwcComposer2D code does not ensure mList is non null. It actually happens early stage of b2g booting. Therefore the code have to handle this case. Adding the following seems reasonable fix.
- Do no call GetGonkDisplay()->SwapBuffers(), if it is not primary display.

> 
> @@ +889,5 @@
> >  HwcComposer2D::Commit()
> >  {
> >      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };
> > +    uint32_t displaytype = mScreen->GetDisplayType();
> > +    displays[displaytype] = mList;
> 
> I didn't see any problem on my test though.

Thanks for the information.

> 
> @@ +940,5 @@
> >  HwcComposer2D::Reset()
> >  {
> >      LOGD("hwcomposer is already prepared, reset with null set");
> >      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };
> > +    mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays);
> 
> If so, does that mean the original implementation does not work as reset
> either?

It seems to have a meaning on ICS gonk. But recent gonk do nothing.

Comment 91

[Sotaro Ikeda [:sotaro]]

> > 
> > @@ +810,4 @@
> > >  {
> > >      // HWC module does not exist or mList is not created yet.
> > >      if (!mHwc || !mList) {
> > >          return GetGonkDisplay()->SwapBuffers(mDpy, mSur);
> > 
> > When it comes to the stage of rendering external display, mHwc is not null
> > (mHal->HasHwc() returns true) and mList should be initialized. Therefore
> > |GetGonkDisplay()->SwapBuffers(mDpy, mSur)| will not be called on
> > non-primary display, and I don't think Bug 1144012 affects what is changed
> > here.
> 
> Current HwcComposer2D code does not ensure mList is non null. It actually
> happens early stage of b2g booting. Therefore the code have to handle this
> case. Adding the following seems reasonable fix.
> - Do no call GetGonkDisplay()->SwapBuffers(), if it is not primary display.

It could make the code more explicit.

Comment 92

[Shelly Lin [:shelly]]

Attachment: Part 1: Support multi-screen on Gonk platform

Hi mmu and sotaro,
I've merged the previous patch (attachment 8611088 [details] [diff] [review]) into this patch (since they can't build separately), and it has carried r+ from jgilbert and matt.woodrow.

This version has address the previous review feedback and the backward compatible issue with ICS. Thank you!

Comment 93

[Shelly Lin [:shelly]]

Attachment: Part 2: Support multi-screen on Gonk platform

Sorry, update nit fix for the previous patch.
Part 0 will rename to Part 1 shortly.

Comment 94

[Michael Wu [:mwu]]

Comment on attachment 8614596 [details] [diff] [review]
Part 2: Support multi-screen on Gonk platform

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

Looks fine to me. Over to Sotaro for a closer look at the HWC changes.

::: widget/gonk/HwcComposer2D.cpp
@@ +799,2 @@
>  {
> +    nsRefPtr<nsScreenGonk> screen = (static_cast<nsWindow*>(aWidget))->GetScreen();

nit: I don't think the parenthesis around the static_cast is necessary.

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +583,5 @@
> +
> +    NS_IMETHOD Run()
> +    {
> +        nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +        if (!os) {

I don't think you should check if getting the observer service succeeded. If it didn't, something has gone horribly wrong and a crash is more reasonable.

Comment 95

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8614596 [details] [diff] [review]
Part 2: Support multi-screen on Gonk platform

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

Looks good! Good job. But some comments are not addressed yet. They are small problems. Therefore it is not necessary to be addressed in this bug.

::: widget/gonk/HwcComposer2D.cpp
@@ +802,3 @@
>      // HWC module does not exist or mList is not created yet.
> +    // GonkDisplay->SwapBuffers is available for primary display only.
> +    if ((screen->IsPrimaryScreen() && !mHwc) || !mList) {

"!screen->IsPrimaryScreen() && !mList" case is still not handled correctly.
In theory, it could happen. In reality, it could rarely happen. Then it is OK in this bug.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +386,5 @@
> +        // FIXME!! values[2] returns 0 for external display, which doesn't
> +        // sound right, Bug 1169176 is the follow-up bug for this issue.
> +        data.mXdpi = values[2] ? values[2] / 1000.f : DEFAULT_XDPI;
> +        CreateSurface(data.mNativeWindow, data.mDisplaySurface, width, height);
> +        mHwc->blank(mHwc, HWC_DISPLAY_EXTERNAL, 0);

We need to use setPowerMode() if HWC is v 1.4 and above.
The following explains it.
http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/hwcomposer.h#606

Comment 96

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8614596 [details] [diff] [review]
Part 2: Support multi-screen on Gonk platform

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

::: widget/gonk/HwcComposer2D.cpp
@@ +802,3 @@
>      // HWC module does not exist or mList is not created yet.
> +    // GonkDisplay->SwapBuffers is available for primary display only.
> +    if ((screen->IsPrimaryScreen() && !mHwc) || !mList) {

Sorry, my comment here was wrong. I did not recognized that GonkDisplay::SwapBuffers() is updated.

Comment 97

[Shelly Lin [:shelly]]

Attachment: Part 2: Support multi-screen on Gonk platform

Carry r+ from mwu, jgilbert,and matt.woodrow.

Hi sotaro, thanks for pointing out the part I missed!

I've add a PowerOnDisplay function to avoid duplicating code, and let me double check on this, are you good with the change of

>      // HWC module does not exist or mList is not created yet.
> +    // GonkDisplay->SwapBuffers is available for primary display only.
> +    if ((screen->IsPrimaryScreen() && !mHwc) || !mList) {

in Hwc? bug 1144012 should aware of the changes here too.

Comment 98

[Henry Chang [:hchang]]

Attachment: Part 1: Split implementation of screen into its own cpp file (rebased, carry r+ from mwu)

Comment 99

[Shelly Lin [:shelly]]

Attachment: Part 3: Add an option to set screen ID in windowWatcher.openWindow

Hi roc, this is actually a missing change of bug 1138290, the one which adds an option to setup screen Id in windowwatcher, could you take a quick look on this? Thanks.

Comment 100

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8615176 [details] [diff] [review]
Part 2: Support multi-screen on Gonk platform

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

Looks good :-)

::: widget/gonk/HwcComposer2D.cpp
@@ +802,3 @@
>      // HWC module does not exist or mList is not created yet.
> +    // GonkDisplay->SwapBuffers is available for primary display only.
> +    if ((screen->IsPrimaryScreen() && !mHwc) || !mList) {

screen->IsPrimaryScreen() could be removed. My comment was based on old SwapBuffers().

Comment 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8615217 [details] [diff] [review]
Part 3: Add an option to set screen ID in windowWatcher.openWindow

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

::: embedding/components/windowwatcher/nsWindowWatcher.cpp
@@ +760,5 @@
> +        // "display-changed" event, it is also platform-dependent.
> +      #ifdef MOZ_WIDGET_GONK
> +        int retval = WinHasOption(features.get(), "mozDisplayId", 0, nullptr);
> +        windowCreator2->SetScreenId(retval);
> +      #endif

#ifdef/#endif start in the first column.

Comment 102

[Shelly Lin [:shelly]]

Attachment: Part 2: Support multi-screen on Gonk platform (carry r+ from mwu,sotaro,jgilbert,matt.woodrow)

Try server error fixed.

Comment 103

[Shelly Lin [:shelly]]

Comment on attachment 8616638 [details] [diff] [review]
Part 2: Support multi-screen on Gonk platform (carry r+ from mwu,sotaro,jgilbert,matt.woodrow)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +806,2 @@
>  {
> +    nsScreenGonk* screen = static_cast<nsWindow*>(aWidget)->GetScreen();

Use raw pointer to avoid thread-safety issue.

Comment 104

[Shelly Lin [:shelly]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6738376ba5f3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1f6a15d1c8

Comment 105

[Shelly Lin [:shelly]]

Attachment: Part 3: Add an option to set screen ID in windowWatcher.openWindow (carry r+ from roc)

Nit fix per comment of roc.

Comment 106

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/3def91c9ff24
https://hg.mozilla.org/integration/b2g-inbound/rev/fed42e7cad94
https://hg.mozilla.org/integration/b2g-inbound/rev/8870e950afc5

Comment 107

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3def91c9ff24
https://hg.mozilla.org/mozilla-central/rev/fed42e7cad94
https://hg.mozilla.org/mozilla-central/rev/8870e950afc5