New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 873287 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

Clarify display::Screen::GetPrimaryDisplay() behavior

Project Member Reported by sergeyu@chromium.org, Aug 10

Issue description

After https://crrev.com/581987 ScreenBase::GetPrimaryDisplay() returns a Display instance with id=255 when there are no displays connected. This is not right thing to do because screen implementations may not know that this is a special display_id value and return it for connected displays. 

A better solution would be to return a null display instance (i.e. id = kInvalidDisplayId). It would also make it easy for callers to detect when there are no displays connected.
 
Description: Show this description
Components: UI
Can you be more specific about the issue? You'll need a display (real or non real) to be able to open a display, and a client should update itself if it depends on a display.
Components: -UI UI>Shell>Display OS>Kernel>Display
Labels: OS-Chrome OS-Fuchsia
Copying my comments regarding this discussion from the CL here for posterity:

"A valid display means one with ID not equal to kInvalidDisplayId, but rather is equal to the newly added kDefaultDisplayId. We need to distinguish between two scenarios. If we get a kInvalidDisplayId that means something terrible has happened ---> It means a bug in our code.

But if we get kDefaultDisplayId, it means we're in an expected state such as when displays are being replaced during the switch to Unified Mode, or when a Chromebox boots up without a display connected."

"Display IDs are not random. They have a known format as extracted from the display's EDID [1]:

   | manufacturer_id | product_code_hash | connector_index |
The least significant 8 bits are occupied with the connector_index.

The connector_index itself is [2]

   | device_index | display_index |
I don't think a display ID of 0xFF (that is 0x00000000000000FF) is ever possible. You have to have a display with 0 manufacturer_id, 0 product_code_hash, and connected to 16th device, and the 16th connector.

[1]: https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/drm_util.cc?type=cs&q=CreateDisplaySnapshot&g=0&l=461

[2]: https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/drm_util.cc?dr=C&q=%22ConnectorIndex%22&g=0&l=193"
That argument only applies to Chrome OS (windows and other platforms have to do something else). Sergey is working on the fuchsia port.

We can change the value of kDefaultDisplayId to a more general value that works for all platforms. But I still think we need to make a clear difference between an invalid state (a fatal bug in our code) and a valid state where there is no "physical" display is connected.
No physical display does not mean no display for apps. Applications should still be able to run correctly and to do that you need a display.

For example, if you turns of your display, it may be seen as it is disconnected. Even in that case, it shouldn't be removed until new display configuration is seen, because when you turn it on back, it should stay the same as before.

That's being said, I agree that the current value is cros specific and we may need more flexible way to specify the default
display.


It seems like we already have a flexible way, by overriding Screen::GetPrimaryDisplay().

X11 uses "0" for its default display id. Windows hashes a string for its default display (and all other displays for that matter).

I suppose it doesn't matter what the id is if it's only used when there are no other displays, but I'd still lean toward letting the subclasses deal with this since they all have different ways of generating ids.
afakhry@, I don't think kInvalidDisplayId should be treated as fatal error. If it was a fatal error then we would have NOTREACHED in the default Display constructor. Just searching for kInvalidDisplayId shows that there already plenty of code that handles Display with id==kInvalidDisplayId as non-fatal. There are many cases when this a null Display instances are useful. This is also Display::is_valid() that makes it easier to handle them.

Currently there is a CHECK in WindowTreeHostManager::GetPrimaryDisplayId() that asserts that there is always a "primary" display. That assertion is invalid because there are cases when there is no primary display. IMO the right fix for crbug.com/866714 would be to remove that CHECK.

oshima@, This bug is mainly about Screen::GetPrimaryDisplay() being required to return a valid display, even when system has no displays. It could be that the behavior you are suggesting makes sense on some platforms, but I don't think it should be required everywhere. Screen::GetPrimaryDisplay() should be allowed to return invalid Display instance when there is no "primary" display. When there is no primary display it means that of the displays that exist none can be considered primary. It doesn't mean that all windows should be destroyed. 

If there are cases when faking a display in no-display case makes sense then it should be platform-specific detail, not a requirement for all Screen implementations. Also it would be better to call it more something more specific instead of "default display" (e.g. kFakeTempDisplayId) and put it somewhere where it's clear it's ash/chrome-os specific.
sergeyu@, there should always be an display while OS is running because window / UI has to be on some display. It doesn't have to be *physical* display. You should be able to create a window, maximize it on Windows even if you don't have a physical display connected to the machine. All (at least major) OSses always has "main/primary" display even no display is connected, including Windows/Mac/Linux and Android.

kInvalidDisplayId should not be visible to client code, hence it should be fatal if it becomes visible to a client. On the other hand, framework code does check because this can happen while configugring displays, but gfx::Screen shouldn't return an invalid display.
There are cases when we need chrome process running when it doesn't have any visible window UI and there may be no displays. For example on Fuchsia we'd like to have webrunner service that allows other processes to run web content. Caller may choose not to display that content on display. Background pages in extensions and service workers also can run without any UI. Even if some platforms simulate primary/main displays when no displays exist it doesn't mean that we need to do that in chrome on all platforms. I'm not sure how Windows and Mac handle this case, but on Linux there is no concept of a default display (though currently chrome can only run in context of an X session, which must be connected to a display, physical or not. but that's chrome's limitation not platform's).

> kInvalidDisplayId should not be visible to client code, hence it 
> should be fatal if it becomes visible to a client.

See my previous reply - there is plenty of code that uses kInvalidDisplayId and doesn't handle it as fatal error. There is also Display::is_invalid(). display.h doesn't say anything about kInvalidDisplayId being fatal.

> On the other 
> hand, framework code does check because this can happen while 
> configugring displays, but gfx::Screen shouldn't return an invalid display.

Why? What if the code above display layer wants to detect when there is no display?
> There are cases when we need chrome process running when it doesn't have any visible window UI 

That's different issues. chrome can run wihtouth UI using --headless. And if a program doesn't really need a display, there is no need to call GetPrimaryDisplay.

> there is plenty of code that uses kInvalidDisplayId and doesn't handle it as fatal error

Yes, because OS layer (ash/display  management code) has to handle this case, but not chrome itself.
It is also used as a "not specified", "or not matched", but not used for "no physical display" situation.

> Why? What if the code above display layer wants to detect when there is no display?

Low level code that provides Screen implementation can use that way. (ui/display/manager ash/ for example)

Note that "display" is a logical concept. It may or may not be physical, and that's pretty common among
major OSes. As I mentioned, otherOS provides a primary displya/main display and there is no need to check
if exists.

https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?rcl=82f038dd29328069c2b89d674b707c42e90e6bd5&l=710
https://cs.chromium.org/chromium/src/ui/display/mac/screen_mac.mm?rcl=82f038dd29328069c2b89d674b707c42e90e6bd5&l=247

If there is a need for an app to detect the type of display (physical, remote, offscreen etc), we can provide that api.



Forgot to mention about Linux.

> but on Linux there is no concept of a default display

*Linux* doesn't provide a window service as a part of OS, and multi
display support on linux has been very poor. That's being said,
X + xrandr does provide a display with disconnected state.
(Standard) Wayland protcol doesn't expose display information at all,
but that's different issue. (We extended this for ARC++)

For application, it's really up to the toolkit they uses on linux,
but you shold be able to get some display information as long as
the window service is up and running.
It looks like we are conflicting two separate questions:
1. Can the list of displays be empty? (i.e. can GetAllDisplay() return an empty list)
2. Can GetPrimaryDisplay() return an invalid Display instance? (when there is no primary display, regardless of how many displays are connected).

You seem to argue that then answer to (1) is No, but this bug is about GetPrimaryDisplay().

My point is that GetPrimaryDisplay() should be allowed to return invalid display when there is no "primary" display, even when the list of displays is not empty.

Anyway, see my comment on CR for why the current approach doesn't work well on Fuchsia: https://chromium-review.googlesource.com/c/chromium/src/+/1171586#message-888083d4afa7e9f3762a8153b5ddfceec10ba524
Primary is UX/WM concept, so it's up to each platfrom to determine which onein display list should be primary. For example, Android simply returns display 0.

I think we should have VC because I'm also confused because the bug says "when no displa is connected", which seems to be unrelated to your problem?
Summary: Clarify display::Screen::GetPrimaryDisplay() behavior (was: Clarify display::Screen::GetPrimaryDisplay() behavior when no display is connected)
> Primary is UX/WM concept, so it's up to each platfrom to determine which onein
> display list should be primary. For example, Android simply returns display 0.

What if the system doesn't have a concept of "primary" display, i.e. all displays are equal?
If the answer is "return the first one", then the question is: what if there are no displays?


> I think we should have VC because I'm also confused because the bug says "when 
> no displa is connected", which seems to be unrelated to your problem?

Sorry, I see how this can be confusing. Removed "when no display is connected" from the bug title. The comments in screen.h say the following:
  // Returns the primary display. It is guaranteed that this will return a
  // display with a valid display ID even if there is no display connected.
  // A real display will be reported via DisplayObserver when it is connected.
  virtual Display GetPrimaryDisplay() const = 0;

The problem here is that GetPrimaryDisplay() is not allowed to return invalid Display. That restrictions doesn't make sense on all platforms.
Could you please setup VC? I think it's much easier to discuss solution, and resolve misunderstanding (if any).

Like I said before, the display at gfx::Screen level is logical, and there is no situation where no displays exist.

Sign in to add a comment