Closed Bug 1787561 Opened 4 months ago Closed 3 months ago

Useful HDR telemetry requires GetColorDepth support in GPUVideoImage

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox105 --- fixed
firefox106 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(4 files)

HDR telmetry is emitted properly in the test_accumulated_play_time.html test, but not in real-world scenarios like YouTube or during playback of local files. This is because different scenarios use different Image types and only MacIOSurfaceImage provides a real value for GetColorDepth. Specifically:

  • YouTube HDR video uses GPUVideoImage.
  • Local HDR files use PlanarYCbCrImage.

Both of these need to implement GetColorDepth for HDR telemetry to be usefully collected.

Why is YouTube using a different code path than local files? I wouldn't expect where we get the file to make a difference in how we play it. Is the codec different?

Flags: needinfo?(bwerth)

This ensures that the Image that the client receives has the color depth
information that was sent to the GPU, even though the client can't query
the GPU directly.

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

HDR telmetry is emitted properly in the test_accumulated_play_time.html test, but not in real-world scenarios like YouTube or during playback of local files. This is because different scenarios use different Image types and only MacIOSurfaceImage provides a real value for GetColorDepth.

It looks like the only issue is that videos decoded in the RDD process, and sent to the client in a RemoteImageHolder had no way to carry color depth information. It must be that test_accumulated_play_time.html was loading videos in such a way that the RDD process was not involved. The patch D155902 seems to be enough to fix the problem. Now, how to test?

Flags: needinfo?(bwerth)
Summary: Useful HDR telemetry requires GetColorDepth support in GPUVideoImage and PlanarYCbCrImage → Useful HDR telemetry requires GetColorDepth support in GPUVideoImage
Attachment #9292145 - Attachment description: WIP: Bug 1787561 Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through. → Bug 1787561 Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through.

This is necessary scaffolding for testing of the HDR telemetry in a way
that involves the RDD process. This is important for matching real-world
conditions.

Depends on D155902

This is a signifier for the correct emission of HDR telemetry values,
though it doesn't check the telemetry directly. It confirms that the video
element is generating increasing time values while playing HDR video, and
those values are used to emit the telemetry.

Depends on D156245

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afef5857aea6
Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through. r=media-playback-reviewers,gfx-reviewers,alwu,aosmond
https://hg.mozilla.org/integration/autoland/rev/c1375f03dc75
Part 2: Expose HDR telemetry to HTMLMediaElement as a Chrome property. r=alwu,emilio
https://hg.mozilla.org/integration/autoland/rev/a41ab0c5f32f
Part 3: Test the values used by HDR telemetry. r=alwu

Backed out for causing mochitest failures on browser_tab_visibility_and_play_time.js
Backout link
Push with failures
Link to failure log
Failure line :
TEST-UNEXPECTED-FAIL | dom/media/test/browser/browser_tab_visibility_and_play_time.js | Test timed out -

Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a830d3c1fb0
Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through. r=media-playback-reviewers,gfx-reviewers,alwu,aosmond
https://hg.mozilla.org/integration/autoland/rev/f50558486f41
Part 2: Expose HDR telemetry to HTMLMediaElement as a Chrome property. r=alwu,emilio
https://hg.mozilla.org/integration/autoland/rev/8c2a1a7e10a0
Part 3: Test the values used by HDR telemetry. r=alwu
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
Flags: needinfo?(bwerth)

Comment on attachment 9292145 [details]
Bug 1787561 Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through.

Beta/Release Uplift Approval Request

  • User impact if declined: 105 release will not emit HDR telemetry for YouTube video. We will have to push back our analytics plan until 106, which may impact our decision making for adding HDR support for other platforms.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The code changes the composition of cross-process messages for video, which is an infrequent change. This is an area that may not get as much test scrutiny.
  • String changes made/needed:
  • Is Android affected?: Yes
Attachment #9292145 - Flags: approval-mozilla-beta?
Attachment #9292748 - Flags: approval-mozilla-beta?
Attachment #9292749 - Flags: approval-mozilla-beta?

This needs rebasing for uplift.

Flags: needinfo?(bwerth)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

This needs rebasing for uplift.

I'll fix the bad merge by landing the patch in Bug 1790281, then nominate this patch plus that patch for uplift.

Flags: needinfo?(bwerth)
Attachment #9292145 - Flags: approval-mozilla-beta?
Attachment #9292748 - Flags: approval-mozilla-beta?
Attachment #9292749 - Flags: approval-mozilla-beta?
Regressions: 1790436

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

This needs rebasing for uplift.

I'll fix the bad merge by landing the patch in Bug 1790281, then nominate this patch plus that patch for uplift.

And also the other regression, Bug 1790436. A patch is awaiting review there. These three Bugs will all need to be uplifted together.

Comment on attachment 9292145 [details]
Bug 1787561 Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through.

Beta/Release Uplift Approval Request

  • User impact if declined: We will not get telemetry on HDR usage in 105 Release, which we need to make future HDR plans at end of year.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1790281, Bug 1790436
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Though not much code is changed, this is exercising under-tested code paths. The patch already triggered two regressions in Nightly.
  • String changes made/needed:
  • Is Android affected?: Yes
Attachment #9292145 - Flags: approval-mozilla-beta?
Attachment #9292748 - Flags: approval-mozilla-beta?
Attachment #9292749 - Flags: approval-mozilla-beta?

Comment on attachment 9292145 [details]
Bug 1787561 Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through.

This missed the Fx105 RC build and should get some bake time especially in light of the regressions we hit on Nightly. Moving the approval request over to Release for consideration for the planned dot release due to ship on October 4. Also, as mentioned in comment 11, this still isn't going to graph cleanly to Release as-landed. Please either post rebased patches or at least a link to a Try push of the rebased patch stack.

Flags: needinfo?(bwerth)
Attachment #9292145 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Attachment #9292748 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Attachment #9292749 - Flags: approval-mozilla-beta? → approval-mozilla-release?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)

Comment on attachment 9292145 [details]
Bug 1787561 Part 1: Make GPUVideoImage track color depth, and make RemoteImageHolder pass it through.

This missed the Fx105 RC build and should get some bake time especially in light of the regressions we hit on Nightly. Moving the approval request over to Release for consideration for the planned dot release due to ship on October 4. Also, as mentioned in comment 11, this still isn't going to graph cleanly to Release as-landed. Please either post rebased patches or at least a link to a Try push of the rebased patch stack.

Got it. I'll make a unified patch containing these three parts plus fixes from the regressions, rebase it against mozilla-release and nominate that patch.

Flags: needinfo?(bwerth)

This patch includes rebased patches from Bug 1787561, Bug 1790281,
and Bug 1790436. It makes GPU-managed video report HDR telemetry
correctly.

Attachment #9292145 - Flags: approval-mozilla-release?
Attachment #9292748 - Flags: approval-mozilla-release?
Attachment #9292749 - Flags: approval-mozilla-release?

Comment on attachment 9294504 [details]
Bug 1787561: REBASED RELEASE Make HDR telemetry work for GPU-managed video, add tests.

Beta/Release Uplift Approval Request

  • User impact if declined: We will be unable to get HDR telemetry from Release 105, which we need to evaluate future HDR work at end of year.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Patch has a tricky landing history; original patches caused regressions in Nightly. This patch is combined 5 parts from multiple bugs, rebased onto mozilla-release.
  • String changes made/needed:
  • Is Android affected?: Yes
Attachment #9294504 - Flags: approval-mozilla-release?

Comment on attachment 9294504 [details]
Bug 1787561: REBASED RELEASE Make HDR telemetry work for GPU-managed video, add tests.

Approved for 105.0.2, thanks for the rebased roll-up patch.

Attachment #9294504 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.