feat(DASH): Add video transfer characteristics. #1210

sr1990 · 2023-05-27T06:27:36Z

This PR is related to #1035

packager/mpd/base/mpd_utils.cc

packager/mpd/base/period.cc

packager/mpd/base/mpd_utils.cc

github-actions · 2023-07-24T22:02:10Z

Closing due to inactivity. If the author would like to continue this PR, they can reopen it (preferred) or start a new one (if needed).

joeyparrish · 2023-07-24T22:05:38Z

The robot is not smart enough to recognize new commits as activity. I'll update it soon. Reopening the PR.

joeyparrish · 2023-07-24T22:11:19Z

packager/mpd/base/mpd_utils.cc

@@ -162,6 +162,16 @@ std::string GetAdaptationSetKey(const MediaInfo& media_info,
  if (!ignore_codec) {
    key.append(":");
    key.append(GetBaseCodec(media_info));
+
+    if (media_info.video_info().has_transfer_characteristics()) {


@sr1990, I added a comment to the Dolby section of this, and made it so that explicit transfer characteristics take priority over the assumption of PQ for Dolby. Please take a look, and let me know if you agree.

joeyparrish · 2023-07-24T22:13:36Z

packager/mpd/base/period.cc

+    // - Common CCIP values.
+    // Dolby vision:
+    // https://professionalsupport.dolby.com/s/article/How-to-signal-Dolby-Vision-in-MPEG-DASH
+    if (media_info.video_info().has_transfer_characteristics()) {


@sr1990, I made the same change here. Please take a look.

joeyparrish · 2023-07-24T22:15:56Z

@sr1990, I will merge this if you approve of my edits. If not, we can discuss it further. Thanks!

joeyparrish · 2023-07-24T23:04:24Z

My change caused a test failure where the golden output changed from transfer characteristics of 16 to 2. Why is that?
Should a Dolby codec string really override an explicit setting where media_info.video_info().has_transfer_characteristics()? Is the test input mp4 wrong for this scenario?

sr1990 · 2023-07-25T01:16:10Z

@joeyparrish Thanks for reviewing.
I don't think the input mp4 is wrong as I had checked Dolby Profile 5 content at https://developer.dolby.com/tools-media/sample-media/video-streams/dolby-vision-streams/ which also has the transfer function set to 2 (unspecified). The content is inaccessible now. Please let me know if you can access it.

Found https://developer.apple.com/av-foundation/High-Dynamic-Range-Metadata-for-Apple-Devices.pdf which mentions that for DV Profile 5:
• Color Transfer Function Index shall be set to 2 (Unspecified) in the colr atom.
Not sure why it is set to 2.

Looks like HLS's GetVideoRange method was written based on the above reasoning.
Also, to keep DASH code consistent with GetVideoRange method:

shaka-packager/packager/hls/base/media_playlist.cc

Line 554 in d5ca6e8

std::string MediaPlaylist::GetVideoRange() const {

,
I think we should give the codec string higher priority over transfer characteristics in case of Dolby.
What do you think?

joeyparrish · 2023-07-26T15:44:45Z

I don't think the input mp4 is wrong as I had checked Dolby Profile 5 content at https://developer.dolby.com/tools-media/sample-media/video-streams/dolby-vision-streams/ which also has the transfer function set to 2 (unspecified).

Found https://developer.apple.com/av-foundation/High-Dynamic-Range-Metadata-for-Apple-Devices.pdf which mentions that for DV Profile 5: • Color Transfer Function Index shall be set to 2 (Unspecified) in the colr atom. Not sure why it is set to 2.

I think we should give the codec string higher priority over transfer characteristics in case of Dolby.
What do you think?

Okay, then this is a peculiarity of Dolby, and Dolby codec strings really should have higher priority. But we should add a comment to the code in those places to explain it, because it's not at all obvious. I suggest something like:

  // Apple's docs for DV profile 5 state: "Color Transfer Function Index shall
  // be set to 2 (Unspecified) in the colr atom."  So we can't trust the colr
  // atom for Dolby inputs, and must set the transfer function based on the
  // codec string alone.

sr1990 · 2023-07-27T06:13:40Z

@joeyparrish Sounds good. Right now we are getting the transfer characteristics from SPS VUI and not colr atom. Looks like #1205 and #1251 are working on reading and writing the colr atom.
I think we can complete this PR right now without the comment and once PRs for colr reading and writing are done, add the colr box for dolby vision as per the spec (even if the input does not contain the colr box) and with the comment.
OR if you prefer, we can complete this PR after mentioned PRs are done.

For adding the colr box for dolby p5 content, in
MP4Muxer::GenerateVideoTrak
check

if (video_info->codec() == kCodecH265DolbyVision &&
   video_info->codec_string().find("dvh1.05") != std::string::npos)) {
1. Get HEVCDecoderConfigurationRecord from video_info->codec_config()
2. Check if HEVCDecoderConfigurationRecord has profile set to Main 10
3. If above is true, add a colr atom with
• Color Primaries set to 2 (Unspecified).
• Color Transfer Function Index set to 2 (Unspecified).
• Color Matrix Index set to 2 (Unspecified).
}

sr1990 · 2023-08-28T01:52:12Z

@joeyparrish, based on conversation at #1255

Dolby Vision profile 5 must be PQ. The VUI information is optional, 
usually we don't care the value of transfer_characteristics, 
both 2(UNSPECIFIED) and 16 (PQ) are valid.

I think we should keep the original code and add the above comment w/o writing the colr atom.
Please let me know what you think.

joeyparrish · 2023-09-01T00:00:55Z

Sounds good.

This reverts commit 77fbcb16d25e69439b0ead6a4aaab29219a08536.

This reverts commit 00f2890566b71c4c143bccbc3c37e8da1a1f8d23.

This reverts commit 783f70b8a114fa7af81e417795b3ea2cba2bdcbe.

This reverts commit c5a6009c03188e393e6ada0f5bb61da10b57f27e.

sr1990 · 2023-09-05T06:07:19Z

Sounds good.

Updated.

joeyparrish · 2023-09-08T21:41:29Z

@sr1990, thank you for your patience and persistence!

build in CMake branch is broken at least locally on OS X after #1210 was merged which introduced some new warnings and reintroduced base::nullopt.

sr1990 changed the title ~~feat(DASH):Add video transfer characteristics.~~ feat(DASH): Add video transfer characteristics. May 27, 2023

vish91 reviewed Jun 5, 2023

View reviewed changes

packager/mpd/base/mpd_utils.cc Show resolved Hide resolved

joeyparrish requested changes Jul 5, 2023

View reviewed changes

packager/mpd/base/period.cc Outdated Show resolved Hide resolved

packager/mpd/base/mpd_utils.cc Show resolved Hide resolved

packager/mpd/base/mpd_utils.cc Outdated Show resolved Hide resolved

joeyparrish added type: enhancement New feature or request status: waiting on response Waiting on a response from the reporter(s) of the issue labels Jul 5, 2023

sr1990 force-pushed the transfer_char1 branch from a633188 to 3391a02 Compare July 22, 2023 04:24

github-actions bot closed this Jul 24, 2023

github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 24, 2023

joeyparrish reopened this Jul 24, 2023

joeyparrish reviewed Jul 24, 2023

View reviewed changes

joeyparrish approved these changes Jul 24, 2023

View reviewed changes

sr1990 mentioned this pull request Aug 11, 2023

feat: support Dolby Vision profile 8.x signaling for HLS #1255

Closed

sr1990 and others added 8 commits September 2, 2023 11:52

feat(DASH):Add video transfer characteristics.

066ad92

Improving readability.

e0366c1

Add comment and flip order for Dolby special case

88696b9

Fix line endings

0f41665

Flip Dolby special case order

144f237

Fix linter

8511551

Revert "Fix linter"

2bfd6ef

This reverts commit 77fbcb16d25e69439b0ead6a4aaab29219a08536.

Revert "Flip Dolby special case order"

bb3d188

This reverts commit 00f2890566b71c4c143bccbc3c37e8da1a1f8d23.

sr1990 added 3 commits September 2, 2023 11:52

Revert "Fix line endings"

4c58a1a

This reverts commit 783f70b8a114fa7af81e417795b3ea2cba2bdcbe.

Revert "Add comment and flip order for Dolby special case"

da478b6

This reverts commit c5a6009c03188e393e6ada0f5bb61da10b57f27e.

Adding comment.

9f44b11

sr1990 force-pushed the transfer_char1 branch from 413c945 to 9f44b11 Compare September 2, 2023 19:08

sr1990 requested a review from joeyparrish September 8, 2023 21:30

joeyparrish approved these changes Sep 8, 2023

View reviewed changes

joeyparrish merged commit 8465f5f into shaka-project:main Sep 8, 2023
12 of 21 checks passed

cosmin added a commit to cosmin/shaka-packager that referenced this pull request Sep 18, 2023

fix cmake build after merge of shaka-project#1210

f952eac

cosmin mentioned this pull request Sep 18, 2023

fix: apply cmake fixes after merge of #1210 #1264

Merged

cosmin added a commit that referenced this pull request Sep 19, 2023

fix: apply cmake fixes after merge of #1210 (#1264)

8b87804

build in CMake branch is broken at least locally on OS X after #1210 was merged which introduced some new warnings and reintroduced base::nullopt.

github-actions bot added the status: archived Archived and locked; will not be updated label Nov 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(DASH): Add video transfer characteristics. #1210

feat(DASH): Add video transfer characteristics. #1210

sr1990 commented May 27, 2023

github-actions bot commented Jul 24, 2023

joeyparrish commented Jul 24, 2023

joeyparrish Jul 24, 2023

joeyparrish Jul 24, 2023

joeyparrish commented Jul 24, 2023

joeyparrish commented Jul 24, 2023

sr1990 commented Jul 25, 2023

joeyparrish commented Jul 26, 2023

sr1990 commented Jul 27, 2023 •

edited

Loading

sr1990 commented Aug 28, 2023

joeyparrish commented Sep 1, 2023

sr1990 commented Sep 5, 2023

joeyparrish commented Sep 8, 2023

feat(DASH): Add video transfer characteristics. #1210

feat(DASH): Add video transfer characteristics. #1210

Conversation

sr1990 commented May 27, 2023

github-actions bot commented Jul 24, 2023

joeyparrish commented Jul 24, 2023

joeyparrish Jul 24, 2023

Choose a reason for hiding this comment

joeyparrish Jul 24, 2023

Choose a reason for hiding this comment

joeyparrish commented Jul 24, 2023

joeyparrish commented Jul 24, 2023

sr1990 commented Jul 25, 2023

joeyparrish commented Jul 26, 2023

sr1990 commented Jul 27, 2023 • edited Loading

sr1990 commented Aug 28, 2023

joeyparrish commented Sep 1, 2023

sr1990 commented Sep 5, 2023

joeyparrish commented Sep 8, 2023

sr1990 commented Jul 27, 2023 •

edited

Loading