Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for variable length extension metadata blocks #106

Merged
merged 6 commits into from
Jan 7, 2022

Conversation

quietvoid
Copy link
Owner

Implements fixes for #104, #105

L8, L9, and L10 metadata blocks can have variable lengths, to avoid serializing unnecessary metadata.
That's useful in the case of allowing preset mastering display primaries, as well as being able to support custom primaries.

@quietvoid
Copy link
Owner Author

@saindriches I added your changes, just need to actually test it and fix stuff.

dolby_vision/src/rpu/extension_metadata/blocks/level9.rs Outdated Show resolved Hide resolved
dolby_vision/src/rpu/extension_metadata/blocks/level9.rs Outdated Show resolved Hide resolved
src/dovi/tests.rs Outdated Show resolved Hide resolved
src/dovi/tests.rs Outdated Show resolved Hide resolved
@quietvoid
Copy link
Owner Author

quietvoid commented Jan 6, 2022

Feels a bit odd that the primaries are over 15 bits, since the documentation uses values based on 50 000 max.
Probably have to verify with an XML reference.

@quietvoid quietvoid force-pushed the dmv2_fixes branch 2 times, most recently from 37abee9 to d64b845 Compare January 7, 2022 04:56
@quietvoid
Copy link
Owner Author

Looks like there's no way to validate L10.
However L9 passes the tests:

Test case results (' - ' = not applicable or skipped by configuration):
Test case Id    1: FAIL (Video codec compliance)
Test case Id    2:   -  (Frame continuity)
Test case Id    3: PASS (VUI validation)
Test case Id    4:   -  (Dual layer validation)
Test case Id    5: PASS (RPU NAL unit presence and position)
Test case Id    6: PASS (RPU header compliance)
Test case Id    7: PASS (Basic DM metadata ('L0') - intrinsic)
Test case Id    8: PASS (Basic DM metadata ('L0') - source comparison)
Test case Id    9: PASS (L1 metadata - intrinsic)
Test case Id   10: PASS (L1 metadata - source comparison)
Test case Id   11: PASS (L2 metadata - source comparison)
Test case Id   12: FAIL (L4 metadata presence)
Test case Id   13: PASS (L6 metadata presence and stability)
Test case Id   14: PASS (Comparison with reference RPU)
Test case Id   15: PASS (L5 metadata presence)
Test case Id   16: PASS (Extended display management metadata general validation)
Test case Id   17: PASS (L3 metadata - source comparison)
Test case Id   18: PASS (L8 metadata - intrinsic)
Test case Id   19: PASS (L8 metadata - source comparison)
Test case Id   20: PASS (L9 metadata - intrinsic)
Test case Id   21: PASS (L9 metadata - source comparison)
Test case Id   22: PASS (L10 metadata - intrinsic)
Test case Id   23:   -  (ST 2094-10 syntax check)
Test case Id   24:   -  (ST 2094-10 ext_block_level 1)
Test case Id   25:   -  (ST 2094-10 ext_block_level 2)
Test case Id   26:   -  (ST 2094-10 ext_block_level 5)

@saindriches
Copy link
Contributor

It's able to verify L10 with an XML metadata, it checks target displays used by L8 and compare correspond L10 with XML data, and will raise an error if values don't match.

Use cmv4_0_2_custom_displays.xml with changed values in TargetDisplay:

Level 8 Display Management Metadata mismatch of RPU and external source reference file [E243].
Found Level 10 for target_display_index 255 in RPU but no legal corresponding custom target display in reference XML/MXF.

It will also raise an error if L8 and L10 trims do not correspond (that means any target_display_id which is predefined or not used by L8 trims also should not appear in L10, and any target in L8 which is not predefined must appear in L10), so it should be OK.

quietvoid and others added 4 commits January 7, 2022 10:21
Support parsing L8/L9 from metadata XML
  A predefined list of primaries is added for parsing primary_index of L9/L10.

Co-authored-by: Rainbaby <rainbaby@outlook.jp>
Co-authored-by: quietvoid <39477805+quietvoid@users.noreply.github.com>
@quietvoid quietvoid marked this pull request as ready for review January 7, 2022 15:22
primary_index
} else {
// FIXME: Why are the target primaries offset by the preset source primaries?
primary_index + level9::PREDEFINED_COLORSPACE_PRIMARIES.len()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why this is.
Probably going to have to experiment with different primary values.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways, this is starting to get pretty big so I'd rather merge before fixing more.

Copy link
Contributor

@saindriches saindriches Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for late, L10 should only use the PREDEFINED_COLORSPACE_PRIMARIES, while the PREDEFINED_REALDEVICE_PRIMARIES part is only for L9 from my test. That means L9 may have a source_primary_index: 18 but L10 doesn’t.

If a target display uses primaries from the real devices list, then verifier do not recognize a target_primary_index derived from that list as predefined, we should use 255 in that moment.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to play around with both primary/source index values.
If there's an issue with the current implementation, can create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants