1872892 - macOS Youtube HDR colours washed out with "Ambient mode" enabled

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

Attachment: Screenshot (Ambient Mode Enabled)

It appears that on macOS, Youtube incorrectly renders HDR video to appear washed out when "Ambient mode" option (in the gear menu) is enabled, as well as when end-of-video suggestions are hovered. If ambient mode is disabled, or the video is placed in "theatre mode", the issue appears to not occur unless suggestions are hovered. I noticed this specifically on https://www.youtube.com/watch?v=siPpOwrorm4.

It seems somewhat likely that this is an issue which is happening when we try to composite a semi-transparent element (e.g. the dark overlay when hovering end-of-video suggestions or the coloured glow for ambient mode), we end up collapsing the HDR content to SDR, and the video becomes noticeably de-saturated.

This issue doesn't occur on Chrome, and Safari appears to not offer HDR quality settings on the video. At time of writing I am using Nightly 123.0a1 (2024-01-01).

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Screenshot (Ambient Mode Disabled)

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Leaving a ni? for :bradwerth who might know what's going on.

Comment 3

[Brad Werth [:bradwerth]]

I'll figure this out.

Comment 4

[Brad Werth [:bradwerth]]

Confirmed that I can reproduce.

Comment 5

[Brad Werth [:bradwerth]]

For me, the problem only reproduces in default mode, not theater mode, not fullscreen.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

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

For me, the problem only reproduces in default mode, not theater mode, not fullscreen.

This also matches my experience as described in comment 0 - The ambient mode glow effect is not applied in theatre mode nor fullscreen.

Comment 7

[Brad Werth [:bradwerth]]

Yes, confirmed that Ambient Mode is preventing the video from being handled as an external/native texture, which is a requirement of isolating it into an HDR-capable video layer. This is certainly related to the work in Bug 1849680. Bug 1855573 is open to fix this issue for an in-tree test. I'll make this Bug blocked on that.

Comment 8

[Brad Werth [:bradwerth]]

Skipping this check on underlays is sufficient to fix the issue. I'll check with Glenn on what this check is attempting to accomplish and how should it be modified (rather than skipped entirely).

Comment 9

[Glenn Watson [:gw]]

The issue here is that we don't detect the ambient canvas as being opaque (thus the opaque backdrop check fails), so we don't promote it. Promoting the surface on a transparent slice will cause the rounding of the underlay cutout to not work. In this case, we're being overly conservative (the ambient canvas pixels are opaque, but we don't detect that, and it's difficult to efficiently detect that). One solution would be to merge the picture cache slices and move the underlay prior.

Comment 10

[connor.mccaffrey]

i can confirm this is happening on a 2017 MacBookPro 13 4 thunderbolt ports that does not even have an HDR display, with that said it looks good once this bug is out of the way but i also noticed this bug which initially lead to me just wanting to turn HDR off, which i cant do.

for me, if i use the cinema view then shrink the window i can watch the colours switch from being good to washed out.

Dull Image on normal player - https://imgur.com/a/Tt77tFf

Full Screen (colours look correct) - https://imgur.com/X5M8N55

Comment 11

[Brad Werth [:bradwerth]]

(In reply to Glenn Watson [:gw] from comment #9)

The issue here is that we don't detect the ambient canvas as being opaque (thus the opaque backdrop check fails), so we don't promote it.

I'm trying to visualize the bad outcome if we promote this as an underlay when self.backdrop.kind.is_none(). We know the surface itself is opaque, because all YuvImage which seek promotion are opaque. The clip chain is applied on top of it, so we don't care what that looks like. Why do we care about the presence of the backdrop? The underlay certainly still appears underneath the clip. Why do we care about the rest of the slice?

Comment 12

[Brad Werth [:bradwerth]]

Well, this issue can be fixed by ignoring the presence of the backdrop, and by prioritizing the promotion of the YuvImage prims before any of the Image prims. I'll upload a patch that does this and we'll work it out in reviews.

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1872892: Allow YuvImage underlays in more circumstances.

This patch makes two changes:

1. Underlays with masks will allow promotion even if there is no
   backdrop on the current slice.

2. Image prims will not promote until all of the promotable YuvImage
   prims have been considered for promotion. This is similar to the
   original implementation of underlays in Bug 1849680, where the presence
   of any YuvImage prim would prevent all overlay promotion (which is the
   only type considered for Image prims). The new behavior will allow Image
   prims to be promoted as long as all the YuvImages have been processed.

Comment 14

[Brad Werth [:bradwerth]]

(In reply to Glenn Watson [:gw] from comment #9)

The issue here is that we don't detect the ambient canvas as being opaque (thus the opaque backdrop check fails), so we don't promote it.

Looks like at least in this case, the problem is that when we calculate the visible_local_rect of the TileCacheInstance, we are using the local_rect, when we should be using the local_clip_rect. This is preventing spanning backdrops from covering the visible area, and therefore the backdrop isn't eligible and the promotion later fails.

Comment 15

[Glenn Watson [:gw]]

I have a vague feeling that selecting a spanning backdrop based on the local clip can cause issues that we've had previously. However, I can't recall what those were (maybe I'm thinking of something else). Let's land it with that change and see how it goes.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0ef7889e164
Allow YuvImage underlays in more circumstances. r=gw

Comment 17

[Sandor Molnar[:smolnar]]

Backed out for causing crashes @ webrender::picture::TileCacheInstance::update_prim_dependencies

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

Push with failures

Failure log -> REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/1727172-1.xhtml == layout/reftests/bugs/1727172-1-ref.html | crash-check | This test left crash dumps behind, but we weren't expecting it to!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5886d166601b
Allow YuvImage underlays in more circumstances. r=gw

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/5886d166601b

Comment 21

[Ina Popescu, Desktop QA]

I've replicated this issue using Nightly 123.0a1 on macOS 13 following the STR from Comment 0.
Verified as fixed in the latest Firefox 128.0b9 version under the same configuration, as the issue no longer occurs.