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

Issue 836351 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

macOS hw decoded frames bt709 frames have inconsistent transfer function as overlays

Project Member Reported by ccameron@chromium.org, Apr 24 2018

Issue description

When we draw bt709 hardware decoded frames as overlays, the WindowServer interprets bt709 to mean gamma=1.961

When we draw the same frames using GL, we use the sRGB transfer function.

See discussion at [1], pasted below
    // With respect to rendering BT709
    //  * SMPTE 1886 suggests that we should be using gamma 2.4.
    //  * Most displays actually use a gamma of 2.2, and most media playing
    //    software uses the sRGB transfer function.
    //  * User studies shows that users don't really care.
    //  * Apple's CoreVideo uses gamma=1.961.
    // Bearing all of that in mind, use the same transfer funciton as sRGB,
    // which will allow more optimization, and will more closely match other
    // media players.
[1] https://cs.chromium.org/chromium/src/ui/gfx/color_space.cc?rcl=d493f21106da2a51325e1e9efc0e0f1215285974&l=761

This can result in flickering if we rapidly switch between GL and CA compositing -- see attached synthetic video which switches modes for 1 of every 5 frames.
 
gamma.mov
3.5 MB View Download
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ff73532d13aeea8d125042e3ff44074fda7fadc

commit 8ff73532d13aeea8d125042e3ff44074fda7fadc
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Apr 24 21:10:58 2018

Use Apple interpretation of bt709 for hardware decoded video frames

The Apple interpretation as gamma=1.961 will be used when the video
frame is displayed as an overlay, so use that interpretation in the GL
compositing path as well.

Bug:  836351 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I7aee20d08e8162dd2bbfdc20ba3235789a5a6216
Reviewed-on: https://chromium-review.googlesource.com/1026430
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553277}
[modify] https://crrev.com/8ff73532d13aeea8d125042e3ff44074fda7fadc/media/gpu/vt_video_decode_accelerator_mac.cc
[modify] https://crrev.com/8ff73532d13aeea8d125042e3ff44074fda7fadc/ui/gfx/color_space.cc
[modify] https://crrev.com/8ff73532d13aeea8d125042e3ff44074fda7fadc/ui/gfx/color_space.h
[modify] https://crrev.com/8ff73532d13aeea8d125042e3ff44074fda7fadc/ui/gfx/color_space_win.cc

Cc: viswa.karala@chromium.org
Labels: Needs-Feedback
@Christopher Cameron: Could you please provide sample test file/URL which reproduces the issue with actual and excepted behaviour which helps us in verifying the fix from TE end.

Thanks!
Re #2, I've tested this manually, but it required a custom build to verify. I'll make sure that it is verified by the internal team that reported it.

Comment 4 by q...@chromium.org, Apr 25 2018

Cc: q...@chromium.org
Labels: -Needs-Feedback Merge-Request-67
Verified on 68.0.3406.0. Thank you for the fix.

M67 still has this bug, though.
Project Member

Comment 5 by sheriffbot@chromium.org, Apr 26 2018

Labels: -Merge-Request-67 Merge-Reject-67 Hotlist-Merge-Reject
The bug is marked as P3 or Feature. It should not be merged as M67 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
qaz@ may have a higher priority for this

Comment 7 by q...@chromium.org, Apr 27 2018

Labels: -Pri-3 -Hotlist-Merge-Reject -Merge-Reject-67 Merge-Request-67 Pri-1
This represents a significant user experience degradation in a significant usecase (screencasting). Pumping priority and re-requesting.
Project Member

Comment 8 by sheriffbot@chromium.org, Apr 27 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by gov...@chromium.org, Apr 27 2018

Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comments #4 and #7. Please merge ASAP.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 27 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71337d68efb202fc4239ee68994a0ed6dde58693

commit 71337d68efb202fc4239ee68994a0ed6dde58693
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Apr 27 18:41:02 2018

Use Apple interpretation of bt709 for hardware decoded video frames

The Apple interpretation as gamma=1.961 will be used when the video
frame is displayed as an overlay, so use that interpretation in the GL
compositing path as well.

TBR=ccameron@chromium.org

(cherry picked from commit 8ff73532d13aeea8d125042e3ff44074fda7fadc)

Bug:  836351 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I7aee20d08e8162dd2bbfdc20ba3235789a5a6216
Reviewed-on: https://chromium-review.googlesource.com/1026430
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#553277}
Reviewed-on: https://chromium-review.googlesource.com/1033552
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#356}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/71337d68efb202fc4239ee68994a0ed6dde58693/media/gpu/vt_video_decode_accelerator_mac.cc
[modify] https://crrev.com/71337d68efb202fc4239ee68994a0ed6dde58693/ui/gfx/color_space.cc
[modify] https://crrev.com/71337d68efb202fc4239ee68994a0ed6dde58693/ui/gfx/color_space.h
[modify] https://crrev.com/71337d68efb202fc4239ee68994a0ed6dde58693/ui/gfx/color_space_win.cc

Status: Fixed (was: Assigned)
Merged to 67

Comment 12 by q...@chromium.org, Apr 27 2018

Thanks.

Sign in to add a comment