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.

Sign up for GitHub

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

Jump to bottom

Add AVC and HEVC codec mappings #377

Closed
wants to merge 2 commits into from
Closed

Add AVC and HEVC codec mappings #377

wants to merge 2 commits into from

Conversation

JeromeMartinez
Copy link
Contributor

AVC and HEVC are widely used in Matroska, but without explicit codec mapping.

This PR adds the mapping of AVC and HEVC in Matroska, including the definition of extension blocks in CodecPrivate.

Fix #373.

@JeromeMartinez JeromeMartinez requested review from mbunkus and robUx4 May 10, 2020 11:46
retokromer
retokromer reviewed May 10, 2020
View reviewed changes
codec_specs.md Outdated Show resolved Hide resolved
retokromer
retokromer reviewed May 10, 2020
View reviewed changes
Copy link
Contributor

@retokromer retokromer left a comment

Choose a reason for hiding this comment

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

two nits

codec_specs.md Outdated Show resolved Hide resolved
@steffenmanden steffenmanden mentioned this pull request May 10, 2020
Closed
@steffenmanden
Copy link

Still lacks a couple of reviews @mbunkus and @robUx4 :)

mbunkus
mbunkus requested changes May 17, 2020
View reviewed changes
Copy link
Contributor

@mbunkus mbunkus left a comment

Choose a reason for hiding this comment

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

Thanks. One request:

Could you insert a section somewhere else describing the general way how we allow extending CodecPrivate? Then simply refer to that section from the AVC & HEVC mapping sections. That prevents mistakes by only having a single definition. And it makes the mapping more succinct.

The mapping section would then only say that "CodecPrivate can be extended as described in section XYZ. Valid extensions are: 1. mvcC as described in ISO …, 2. potentialOtherExtension as described in …"

@JeromeMartinez
Copy link
Contributor Author

I modified the patch with a dedicated Private Data Extension Blocks chapter, and codec descriptions link to it.

In a 2nd patch (in this PR) I add the 3 currently known and used block identifiers.

Please review again.

@rcombs
Copy link
Contributor

rcombs commented May 21, 2020

I'm a bit concerned about this Dolby Vision data being placed in CodecPrivate, and particularly having it placed directly after the MP4-style configuration data with no other boundary. This data will be useful to the high-level application (for detecting Dolby Vision streams, distinguishing BL from EL) in addition to being used by the decoder itself. This means the demuxer will want to expose the contents of the DOVIDecoderConfigurationRecord to the application as stream metadata. By placing it after a variable-length structure with no central length code, this adds a requirement that the demuxer internally parses the AVCDecoderConfigurationRecord/HEVCDecoderConfigurationRecord to determine its length, skip it, then read the extension blocks. This also means the Matroska CodecPrivate's contents would differ from that used in ISOBMFF, which means existing tools converting Matroska to MP4 will confusingly produce output with Matroska-style data.
I'd be inclined to either move these into a dedicated "private data extensions" element (consistent with how these are represented in other containers), or add an element indicating the offset of the extension blocks (which still has the potential backwards-compatibility issues but at least doesn't complicate parsing).

@mbunkus
Copy link
Contributor

mbunkus commented May 22, 2020

I'm not too happy that we're discussing this here instead of the corresponding issue #373. I'll continue there.

@mbunkus mbunkus mentioned this pull request May 22, 2020
Closed
robUx4
robUx4 reviewed May 22, 2020
View reviewed changes
codec_specs.md Outdated Show resolved Hide resolved
robUx4
robUx4 reviewed May 22, 2020
View reviewed changes
codec_specs.md Outdated Show resolved Hide resolved
robUx4
robUx4 reviewed May 22, 2020
View reviewed changes
codec_specs.md Show resolved Hide resolved
robUx4
robUx4 requested changes May 22, 2020
View reviewed changes
Copy link
Contributor

@robUx4 robUx4 left a comment

Choose a reason for hiding this comment

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

Some more references needs and allowing only certain extensions per codec.

robUx4
robUx4 reviewed May 22, 2020
View reviewed changes
codec_specs.md Outdated Show resolved Hide resolved
robUx4
robUx4 requested changes May 22, 2020
View reviewed changes
Copy link
Contributor

@robUx4 robUx4 left a comment

Choose a reason for hiding this comment

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

The ISO reference doesn't seem to work.

@JeromeMartinez JeromeMartinez force-pushed the AVC_and_HEVC branch 2 times, most recently from 1f50061 to 83ee8ae Compare May 22, 2020 09:37
@robUx4 robUx4 added codec mapping spec_codecs Codec Matroska spec document target labels May 22, 2020
mbunkus added a commit that referenced this pull request May 25, 2020
@mbunkus
allow extensions to CodecPrivate via BlockAdditionMapping
26bcfe4 
This prepares the element specs for the storage of elements such as
the mvcC extension for AVC video or the dvcC and hvcE extensions for
HEVC video. It does so by

* changing the verbiage to make BlockAdditionMapping not only
  referring to BlockAdditions in tracks,
* making BlockAddIDValue non-mandatory which would indicate that the
  BlockAdditionMapping is actually an extension to CodecPrivate and
* includes a first registered value for BlockAddIDType which signals
  that BlockAddIDExtraData contains a type & content like similar
  header atoms in ISO Base Media File Format files.

See #373 for the discussion how to store things. Will affect PR #377
which will have to be adjusted to make use of these elements.
@mbunkus mbunkus mentioned this pull request May 25, 2020
Closed
This was referenced May 25, 2020
Closed
Merged
@robUx4
Copy link
Contributor

robUx4 commented Jun 7, 2020

Closing in favor of #390 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robUx4 robUx4 robUx4 requested changes

@mbunkus mbunkus mbunkus requested changes

@retokromer retokromer retokromer approved these changes

Assignees
No one assigned
Labels
codec mapping spec_codecs Codec Matroska spec document target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write down how extra data is saved for usage with AVC & HEVC
6 participants
@JeromeMartinez @steffenmanden @rcombs @mbunkus @robUx4 @retokromer