1764201 - video-dynamic-range should match against capabilities of the window's display, not the main display

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Brad Werth [:bradwerth]]

Our matching logic for video-dynamic-range is keying off the main display when it should instead use the display that is actually hosting the window. This will better support multi-monitor setups.

Comment 2

[Greg]

In the case of mirroring, I assume the primary monitor will be used to determine the results (ie, such as the "optimize for" monitor in MacOS).

This bug sounds likely the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=1812647, which results in YouTube showing inferior video in FireFox (ie streaming HDR content to an SDR monitor instead of showing SDR-optimized content on an SDR monitor).

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 1: Give gfxPlatform knowledge of all the screens' metrics, not just primary screen.

Comment 4

[Brad Werth [:bradwerth]]

Some dependencies are S2 Bugs, so this should be S2 also.

Comment 5

[5silentrain]

I see that the fix for this bug has finally gotten off the ground. When can we expect this bug to be fixed? Thank you!

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 2: Expand gfxPlatform::SupportsHDR to support queries based on screen index.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 3: Make gfxWindowsPlatform track and report HDR support per-screen.

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 2: Make Gecko_MediaFeatures_VideoDynamicRange check per-screen HDR.

This makes both macOS and Windows report video-dynamic-range:high if the
platform itself can support it (any screen is HDR), and then makes the
media query additionally check that the actual screens are HDR capable.

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 1: Make widget::Screen track if the screen is HDR capable.

This duplicates existing HDR checks in gfxPlatformMac and
gfxWindowsPlatform. A later part of this patch series will remove that
redundant code, as it replaces calls to gfxPlatform functions with calls
to ScreenManager functions.

Comment 10

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 3: Remove screen info stuff from gfxPlatform.

The gfxPlatform::GetScreen* methods have always been redundant. Removing
them in favor of ScreenManager or nsDeviceContext methods will
discourage future code authors from adding more redundancy in
gfxPlatform.

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1764201 Part 1: Make DeviceManagerDx::Get initialize the singleton, if necessary.

This removes a timing dependency that otherwise requires gfxPlatform to
be initialized before this singleton can be accessed.

Comment 12

[Brad Werth [:bradwerth]]

Bug 1885599 doesn't need to be a blocker. The updated patches can handle a missing DeviceManagerDx, concluding in such a case that the screen(s) are not HDR.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d21d14a2df3
Part 1: Make widget::Screen track if the screen is HDR capable. r=geckoview-reviewers,ahale,m_kato
https://hg.mozilla.org/integration/autoland/rev/ff751c2ce8ef
Part 2: Make Gecko_MediaFeatures_VideoDynamicRange check per-screen HDR. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ea6921c88429
Part 3: Remove screen info stuff from gfxPlatform. r=jgilbert,geckoview-reviewers

Comment 14

[Sandor Molnar[:smolnar]]

Backed out for causing multiple failures, including build bustages @ widget/windows/ScreenHelperWin.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/adfe57981526527ef9d85979937d06f6e8b5c9ea

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/widget/windows/ScreenHelperWin.cpp(79,12): error: use of undeclared identifier 'DXGI_OUTPUT_DESC1

Failure log -> [liblp]bool android::fs_mgr::WritePrimaryMetadata(int, const android::fs_mgr::LpMetadata &, uint32_t, const std::string &, const std::function<bool (int, const std::string &)> &) write 740 bytes failed: Operation not permitted

Failure log -> TEST-UNEXPECTED-FAIL | runjunit.py | The previous test failed because of an error in the test harness | org.mozilla.geckoview.test.rule.TestHarnessException

Failure log Wr2

Failure log xpc

Failure log WD

Comment 15

[Brad Werth [:bradwerth]]

One problem is that the removal of gfxPlatform::PopulateScreenInfo removed the call to gfxVars::SetScreenDepth with a value from the primary screen. This is needed in GLContextProviderEGL::CreateConfigScreen to have a sane bit depth value. Possible fixes:

1. Restore gfxPlatform::PopulateScreenInfo just to preserve the setting of gfxVars screen depth for the primary screen.
2. Make GLContextProviderEGL::CreateConfigScreen pull primary screen scene depth from the widget::ScreenManager.
3. Modify GLContextProviderEGL::CreateConfigScreen to take a screen parameter, and use that to get the specific screen's depth from the widget::ScreenManager.

Option 3 is the best, most forward-looking choice. I fear it may be a bunch of code modification that snarls up this patch further. I'm tempted to pursue Option 2 (preserving current behavior) and file a new bug for the improvements to GLContextProviderEGL::CreateConfigScreen.

Comment 16

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #15)

One problem is that the removal of gfxPlatform::PopulateScreenInfo removed the call to gfxVars::SetScreenDepth with a value from the primary screen. This is needed in GLContextProviderEGL::CreateConfigScreen to have a sane bit depth value. Possible fixes:

1. Restore gfxPlatform::PopulateScreenInfo just to preserve the setting of gfxVars screen depth for the primary screen.
2. Make GLContextProviderEGL::CreateConfigScreen pull primary screen scene depth from the widget::ScreenManager.
3. Modify GLContextProviderEGL::CreateConfigScreen to take a screen parameter, and use that to get the specific screen's depth from the widget::ScreenManager.

It wasn't effective to try Option 2, because the ScreenManager can only be instantiated on the main thread, and GLContextProviderEGL::CreateConfigScreen runs on the render thread. So instead pursuing Option 1.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09f6a0b25938
Part 1: Make widget::Screen track if the screen is HDR capable. r=geckoview-reviewers,ahale,m_kato
https://hg.mozilla.org/integration/autoland/rev/88d751bcbf03
Part 2: Make Gecko_MediaFeatures_VideoDynamicRange check per-screen HDR. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f42d24696e43
Part 3: Remove screen info stuff from gfxPlatform. r=jgilbert,geckoview-reviewers

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/09f6a0b25938
https://hg.mozilla.org/mozilla-central/rev/88d751bcbf03
https://hg.mozilla.org/mozilla-central/rev/f42d24696e43