Closed
Bug 1138288
Opened 9 years ago
Closed 9 years ago
Extend nsScreenManagerGonk to support multiple screens
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
2.2 S12 (15may)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: shelly, Assigned: hchang)
References
Details
Attachments
(1 file, 9 obsolete files)
18.89 KB,
patch
|
Details | Diff | Splinter Review |
In order to support multi-screen on b2g, nsScreenGonk and nsScreenManagerGonk should extend itself of returning not only the primary screen.
Comment 1•9 years ago
|
||
In current architecture, there are most three screens, DISPLAY_PREIMARY, DISPLAY_VIRTUAL and DISPLAY_EXTERNAL for each. So in this patch, I use mDisplayType
Updated•9 years ago
|
Assignee: nobody → kechen
Comment 2•9 years ago
|
||
Comment 1 is not complete, please ignore that. In current architecture, there are three display types, DISPLAY_PREIMARY, DISPLAY_VIRTUAL and DISPLAY_EXTERNAL, and most one screens for each type. So in this patch, I use the display types as the screen id. In this way we don't need to add additional function in nsIScreen. We don't consider the rotation and ScreenForRect condition in current phase.
Updated•9 years ago
|
Attachment #8575027 -
Attachment description: [WIP]0001-Support-ScreenManagerGonk-and-ScreenGonk-to-multi-sc.patch → [WIP]Support ScreenManagerGonk and ScreenGonk to multi screen
Comment 3•9 years ago
|
||
Hi Shelly can you help and give me some advices to my modification? Thank you.
Attachment #8575027 -
Attachment is obsolete: true
Flags: needinfo?(slin)
Reporter | ||
Comment 4•9 years ago
|
||
Looks good overall, thanks Kevin :) I don't see any big problem here, but since the basic structure of nsWindow is not solid yet, let's leave it until then.
Flags: needinfo?(slin)
Assignee | ||
Updated•9 years ago
|
Assignee: kechen → hchang
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8577823 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8599150 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fcdb094c2da
Assignee | ||
Comment 8•9 years ago
|
||
Hi mwu, This is the formal patch based on m-c and is simplified from attachment 8598468 [details] [diff] [review]. My plan is to land this patch first as the basis of further work in Bug 1138287. You can find the real world use case in attachment 8597207 [details]. Following is the summary of this patch: 1) The basic idea is to extend nsScreenManagerGonk to make it capable of adding/removing screens. By now we can only add primary type screen. But Bug 1138287 will be addressing the use of the extension. 2) nsWindow creation and nsScreenGonk creation is independent. The only requirement is nsWindow must be opened in a created nsScreenGonk. 3) nsWindow originally grabs screen information from GonkDisplay. This patch gets rid of this dependency. nsWindow will be associated with a nsScreenGonk and obtain the required information like bound/native window from the screen. 4) nsScreenManagerGonk maintains a nsScreenGonk list rather than mOneScreen. All the ScreenForXxx operation should loop throught the list and find the matched one. The only exception is ScreenForRect since we don't know how to handle this case. 5) We use "display type" to add/remove display. Since one display type may have multiple instances in the future, there is a conversion from display type to screen id. Right now it's nsScreenManagerGonk::GetIdFromType. You can also find the enhancement of this patch if we have the enhanced GonkDisplay, which can be requested to "AddDisplay" and returns the screen id in Bug 1138287. This enhancement will be addressed in that bug.
Attachment #8599151 -
Attachment is obsolete: true
Attachment #8599260 -
Flags: review?(mwu)
Comment 9•9 years ago
|
||
Comment on attachment 8599260 [details] [diff] [review] Bug1138288.patch Review of attachment 8599260 [details] [diff] [review]: ----------------------------------------------------------------- Bug 1157874 will be landing soon - can you rebase on that?
Attachment #8599260 -
Flags: review?(mwu)
Comment 10•9 years ago
|
||
It might be better that nsScreenGonk creation is independent from widget(nsWindow) creation.
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #9) > Comment on attachment 8599260 [details] [diff] [review] > Bug1138288.patch > > Review of attachment 8599260 [details] [diff] [review]: > ----------------------------------------------------------------- > > Bug 1157874 will be landing soon - can you rebase on that? Sure! I will do that. Thanks!
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #10) > It might be better that nsScreenGonk creation is independent from > widget(nsWindow) creation. Hi Sotaro, That's my intension! (See (2) in comment 8). The only exception of current patch is primary screen. I don't mind find a proper place to put "Add primary screen" prior to opening the top level window if there is. Btw, what's your argument on "maintaining a nsIScreen rather than the id"? I am fine to both and I only worry about the if there would be any circular reference issue. Thanks!
Comment 13•9 years ago
|
||
(In reply to Henry Chang [:henry] from comment #12) > (In reply to Sotaro Ikeda [:sotaro] from comment #10) > > It might be better that nsScreenGonk creation is independent from > > widget(nsWindow) creation. > > Hi Sotaro, > > That's my intension! (See (2) in comment 8). The only exception of current > patch > is primary screen. I don't mind find a proper place to put "Add primary > screen" > prior to opening the top level window if there is. > > Btw, what's your argument on "maintaining a nsIScreen rather than the id"? nsWindow does not need to know screenId except to find out nsIScreen. Therefore basically no need to hold screen id. By holding screenId, nsWindow does not need to get nsIScreen each time usage. And it could extend the lifetime of nsIScreen even when the nsIScreen is removed from nsScreenManagerGonk. > I am fine to both and I only worry about the if there would be any circular > reference issue. As a safe guard, it is safer to remove circular reference. Though, the circular reference does not cause a problem actually, because nsWindow::Destroy() is explicitly called during the widget destruction, that break the circular reference. nsScreenGonk is lower layer service than nsWindow. Therefore, it seems better that nsScreenGonk holds
Comment 14•9 years ago
|
||
>
> nsWindow does not need to know screenId except to find out nsIScreen.
> Therefore basically no need to hold screen id. By holding screenId, nsWindow
> does not need to get nsIScreen each time usage. And it could extend the
> lifetime of nsIScreen even when the nsIScreen is removed from
> nsScreenManagerGonk.
It could make the screen destroy handling simpler.
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #13) > (In reply to Henry Chang [:henry] from comment #12) > > (In reply to Sotaro Ikeda [:sotaro] from comment #10) > > > It might be better that nsScreenGonk creation is independent from > > > widget(nsWindow) creation. > > > > Hi Sotaro, > > > > That's my intension! (See (2) in comment 8). The only exception of current > > patch > > is primary screen. I don't mind find a proper place to put "Add primary > > screen" > > prior to opening the top level window if there is. > > > > Btw, what's your argument on "maintaining a nsIScreen rather than the id"? > > nsWindow does not need to know screenId except to find out nsIScreen. > Therefore basically no need to hold screen id. By holding screenId, nsWindow > does not need to get nsIScreen each time usage. And it could extend the > lifetime of nsIScreen even when the nsIScreen is removed from > nsScreenManagerGonk. > > > I am fine to both and I only worry about the if there would be any circular > > reference issue. > > As a safe guard, it is safer to remove circular reference. Though, the > circular reference does not cause a problem actually, because > nsWindow::Destroy() is explicitly called during the widget destruction, that > break the circular reference. nsScreenGonk is lower layer service than > nsWindow. Therefore, it seems better that nsScreenGonk holds You are right! Considering the case of removing screens, nsWindow needs to hold a strong nsIScreen reference. Thanks for reminding me!
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8599260 -
Attachment is obsolete: true
Attachment #8599803 -
Flags: review?(mwu)
Comment 17•9 years ago
|
||
Comment on attachment 8599803 [details] [diff] [review] Bug1138288.patch (rebased) Review of attachment 8599803 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsScreenManagerGonk.h @@ +68,5 @@ > protected: > + uint32_t mId; > + int32_t mPixelDepth; > + int32_t mColorDepth; > + void* mNativeWindow; ANativeWindow is reference counted object. It is normally held like "sp<ANativeWindow> mNativeWindow;". Why does it hold as void pointer? Is there a no risk of this pointer becomes dangling pointer?
Comment 18•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #17) > ANativeWindow is reference counted object. It is normally held like > "sp<ANativeWindow> mNativeWindow;". In gecko it becomes "android::sp<ANativeWindow>".
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #18) > (In reply to Sotaro Ikeda [:sotaro] from comment #17) > > ANativeWindow is reference counted object. It is normally held like > > "sp<ANativeWindow> mNativeWindow;". > > In gecko it becomes "android::sp<ANativeWindow>". Makes sense. Thanks for this review!
Comment 20•9 years ago
|
||
Comment on attachment 8599803 [details] [diff] [review] Bug1138288.patch (rebased) Review of attachment 8599803 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsScreenManagerGonk.h @@ +110,3 @@ > > bool mInitialized; > + nsTArray<nsCOMPtr<nsScreenGonk>> mScreens; use nsRefPtr for concrete classes ::: widget/gonk/nsWindow.cpp @@ +889,5 @@ > > // nsScreenGonk.cpp > > +nsScreenGonk::nsScreenGonk(uint32_t aId, > + const ScreenConfiguration& aConfig, Using ScreenConfiguration here doesn't seem to help since the data needs to be packed into a ScreenConfiguration first. @@ +892,5 @@ > +nsScreenGonk::nsScreenGonk(uint32_t aId, > + const ScreenConfiguration& aConfig, > + uint32_t aPhysicalRotation, > + float aDpi, > + void* aNativeWindow) This is a ANativeWindow*. You can move the querying for width/height to the constructor here and reduce the number of args. Also, the format can be derived here by querying for NATIVE_WINDOW_FORMAT. @@ +927,5 @@ > +nsScreenGonk::GetId() > +{ > + uint32_t id; > + GetId(&id); > + return id; return mId; @@ +948,5 @@ > +nsScreenGonk::GetRect() > +{ > + nsIntRect rect; > + GetRect(&rect.x, &rect.y, &rect.width, &rect.height); > + return rect; return mScreenBounds; @@ +975,5 @@ > nsScreenGonk::GetPixelDepth(int32_t *aPixelDepth) > { > // XXX: this should actually return 32 when we're using 24-bit > // color, because we use RGBX. > + *aPixelDepth = mPixelDepth; Let's just use mColorDepth for everything for now. @@ +999,5 @@ > +{ > + return mDpi; > +} > + > +void* Also an ANativeWindow* @@ +1165,5 @@ > > NS_IMETHODIMP > nsScreenManagerGonk::GetPrimaryScreen(nsIScreen **outScreen) > { > + for (size_t i = 0; i < mScreens.Length(); i++) { Just return mScreens[0]. @@ +1259,5 @@ > + } > + > + char propValue[PROPERTY_VALUE_MAX]; > + property_get("ro.sf.hwrotation", propValue, "0"); > + uint32_t physicalRotation = atoi(propValue) / 90; AFAIK, this rotation only applies to the primary display.
Attachment #8599803 -
Flags: review?(mwu)
Assignee | ||
Comment 21•9 years ago
|
||
Thanks for the review, mwu! Please see my inline reply to some of your comments: (In reply to Michael Wu [:mwu] from comment #20) > Comment on attachment 8599803 [details] [diff] [review] > Bug1138288.patch (rebased) > > ::: widget/gonk/nsWindow.cpp > @@ +889,5 @@ > > > > // nsScreenGonk.cpp > > > > +nsScreenGonk::nsScreenGonk(uint32_t aId, > > + const ScreenConfiguration& aConfig, > > Using ScreenConfiguration here doesn't seem to help since the data needs to > be packed into a ScreenConfiguration first. > > @@ +892,5 @@ > > +nsScreenGonk::nsScreenGonk(uint32_t aId, > > + const ScreenConfiguration& aConfig, > > + uint32_t aPhysicalRotation, > > + float aDpi, > > + void* aNativeWindow) > > This is a ANativeWindow*. You can move the querying for width/height to the > constructor here and reduce the number of args. Also, the format can be > derived here by querying for NATIVE_WINDOW_FORMAT. > My design guideline of nsScreenGonk/nsScreenManagerGonk is to let nsScreenManagerGonk be a screen factory. All the screen information is queried and collected by nsScreenManagerGonk. nsScreenGonk would just act like a plain plain object. This design guideline results in 1) We have to construct nsScreenGonk with all screen configuration through either a list of parameters or a parameter object. I was creating yet another ScreenConfiguration-like object but changed to reuse hal::ScreenConfiguration in the current patch. Now that there are 5 arguments, maybe I should use a parameter object again. 2) Any assumption like "pixel depth is equivalent to color depth" would be made in nsScreenManagerGonk. nsScreenGonk assumes nothing. > @@ +927,5 @@ > > +nsScreenGonk::GetId() > > +{ > > + uint32_t id; > > + GetId(&id); > > + return id; > > return mId; > > @@ +948,5 @@ > > +nsScreenGonk::GetRect() > > +{ > > + nsIntRect rect; > > + GetRect(&rect.x, &rect.y, &rect.width, &rect.height); > > + return rect; > > return mScreenBounds; > I know there introduces some overhead to these getters. However, I think it's better to have a single entry for these getters. For example, if I rename mScreenBounds to mBounds, I only need to change one function but not two. > @@ +975,5 @@ > > nsScreenGonk::GetPixelDepth(int32_t *aPixelDepth) > > { > > // XXX: this should actually return 32 when we're using 24-bit > > // color, because we use RGBX. > > + *aPixelDepth = mPixelDepth; > > Let's just use mColorDepth for everything for now. > > @@ +1165,5 @@ > > > > NS_IMETHODIMP > > nsScreenManagerGonk::GetPrimaryScreen(nsIScreen **outScreen) > > { > > + for (size_t i = 0; i < mScreens.Length(); i++) { > > Just return mScreens[0]. > Since I am not making the assumption that the primary screen would be always added firstly, I loop through the entire screen list to make sure I get the primary one. If it could be a performance issue, maybe I can use an additional member variable to point to the primary screen. (However, I won't stick to not returning mScreens[0] :p )
Flags: needinfo?(mwu)
Comment 22•9 years ago
|
||
(In reply to Henry Chang [:henry] from comment #21) > (In reply to Michael Wu [:mwu] from comment #20) > > Comment on attachment 8599803 [details] [diff] [review] > > Bug1138288.patch (rebased) > > > > ::: widget/gonk/nsWindow.cpp > > @@ +889,5 @@ > > > > > > // nsScreenGonk.cpp > > > > > > +nsScreenGonk::nsScreenGonk(uint32_t aId, > > > + const ScreenConfiguration& aConfig, > > > > Using ScreenConfiguration here doesn't seem to help since the data needs to > > be packed into a ScreenConfiguration first. > > > > @@ +892,5 @@ > > > +nsScreenGonk::nsScreenGonk(uint32_t aId, > > > + const ScreenConfiguration& aConfig, > > > + uint32_t aPhysicalRotation, > > > + float aDpi, > > > + void* aNativeWindow) > > > > This is a ANativeWindow*. You can move the querying for width/height to the > > constructor here and reduce the number of args. Also, the format can be > > derived here by querying for NATIVE_WINDOW_FORMAT. > > > > My design guideline of nsScreenGonk/nsScreenManagerGonk is to let > nsScreenManagerGonk be a screen factory. All the screen information > is queried and collected by nsScreenManagerGonk. nsScreenGonk > would just act like a plain plain object. This design guideline results in > > 1) We have to construct nsScreenGonk with all screen configuration through > either a list of parameters or a parameter object. I was creating yet > another > ScreenConfiguration-like object but changed to reuse > hal::ScreenConfiguration > in the current patch. Now that there are 5 arguments, maybe I should use > a parameter object again. > > 2) Any assumption like "pixel depth is equivalent to color depth" would > be made in nsScreenManagerGonk. nsScreenGonk assumes nothing. > Ok, but I don't see the advantage of having all logic in nsScreenManagerGonk. It's just making the construction of nsScreenGonk more awkward. Move things to nsScreenGonk and simplify the construction of nsScreenGonk. > > @@ +927,5 @@ > > > +nsScreenGonk::GetId() > > > +{ > > > + uint32_t id; > > > + GetId(&id); > > > + return id; > > > > return mId; > > > > @@ +948,5 @@ > > > +nsScreenGonk::GetRect() > > > +{ > > > + nsIntRect rect; > > > + GetRect(&rect.x, &rect.y, &rect.width, &rect.height); > > > + return rect; > > > > return mScreenBounds; > > > > I know there introduces some overhead to these getters. However, I think > it's better to have a single entry for these getters. For example, if > I rename mScreenBounds to mBounds, I only need to change one function > but not two. > The overhead is not the concern. It's probably going to be optimized away. I only care about the number of lines of code. If getting rid of these getters entirely would reduce the number of lines, then do that. That would likely reduce the number of lines. I don't want any helper functions that add more lines of code overall. > > @@ +975,5 @@ > > > nsScreenGonk::GetPixelDepth(int32_t *aPixelDepth) > > > { > > > // XXX: this should actually return 32 when we're using 24-bit > > > // color, because we use RGBX. > > > + *aPixelDepth = mPixelDepth; > > > > Let's just use mColorDepth for everything for now. > > > > @@ +1165,5 @@ > > > > > > NS_IMETHODIMP > > > nsScreenManagerGonk::GetPrimaryScreen(nsIScreen **outScreen) > > > { > > > + for (size_t i = 0; i < mScreens.Length(); i++) { > > > > Just return mScreens[0]. > > > > Since I am not making the assumption that the primary screen would > be always added firstly, I loop through the entire screen list to > make sure I get the primary one. If it could be a performance issue, > maybe I can use an additional member variable to point to the primary > screen. (However, I won't stick to not returning mScreens[0] :p ) Why would the primary screen not get added first? Don't design for cases that don't exist.
Flags: needinfo?(mwu)
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8599803 -
Attachment is obsolete: true
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8601361 -
Attachment is obsolete: true
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8601415 -
Attachment is obsolete: true
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8601422 [details] [diff] [review] Bug1138288 v2 Hi mwu, I've addressed your comments in the patch v2. Could you please have it a look again? Thanks!
Attachment #8601422 -
Flags: review?(mwu)
Comment 27•9 years ago
|
||
Comment on attachment 8601422 [details] [diff] [review] Bug1138288 v2 Review of attachment 8601422 [details] [diff] [review]: ----------------------------------------------------------------- I get what you're doing with renaming mVirtualBounds -> mScreenBounds and mScreenBounds to mNaturalBounds. The real trouble I think comes just from mScreenBounds, though - it's not obvious whether it's referring to the virtual screen that the app uses, or the "real" screen that GLES and the rest of the graphics stack sees. So I would recommend only doing one renaming - mScreenBounds to mNaturalBounds. Or anything else is probably fine, as long as it's not as ambiguous as mScreenBounds. ::: widget/gonk/nsWindow.cpp @@ +902,3 @@ > // nsScreenGonk.cpp > > +nsScreenGonk::nsScreenGonk(uint32_t aId, android::sp<ANativeWindow> aNativeWindow) Having a smart pointer as an arg is a bit weird. I think we should go with a raw pointer here.
Attachment #8601422 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 28•9 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #27) > Comment on attachment 8601422 [details] [diff] [review] > Bug1138288 v2 > > Review of attachment 8601422 [details] [diff] [review]: > ----------------------------------------------------------------- > > I get what you're doing with renaming mVirtualBounds -> mScreenBounds and > mScreenBounds to mNaturalBounds. The real trouble I think comes just from > mScreenBounds, though - it's not obvious whether it's referring to the > virtual screen that the app uses, or the "real" screen that GLES and the > rest of the graphics stack sees. So I would recommend only doing one > renaming - mScreenBounds to mNaturalBounds. Or anything else is probably > fine, as long as it's not as ambiguous as mScreenBounds. > > ::: widget/gonk/nsWindow.cpp > @@ +902,3 @@ > > // nsScreenGonk.cpp > > > > +nsScreenGonk::nsScreenGonk(uint32_t aId, android::sp<ANativeWindow> aNativeWindow) > > Having a smart pointer as an arg is a bit weird. I think we should go with a > raw pointer here. Thanks for the review!
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8601422 -
Attachment is obsolete: true
Assignee | ||
Comment 30•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43f2f4e45391
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c3446a630074
Keywords: checkin-needed
Reporter | ||
Comment 32•9 years ago
|
||
Why is this not merge to m-c yet? If something went wrong please let us know.
Flags: needinfo?(ryanvm)
Comment 33•9 years ago
|
||
Looks like this got merged last Thursday and the bug didn't get marked.
Flags: needinfo?(ryanvm)
Comment 34•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3446a630074
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S12 (15may)
Comment 35•9 years ago
|
||
Comment on attachment 8602545 [details] [diff] [review] Bug1138288 v3 (r+'d) Review of attachment 8602545 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +1280,5 @@ > + mScreens.AppendElement(screen); > +} > + > +void > +nsScreenManagerGonk::RemoveScreen(uint32_t aDisplayType) Doesn't the argument prevent support concurrent multiple VirtualDisplaySurfaces usage, does it?
You need to log in
before you can comment on or make changes to this bug.
Description
•