Add AVC and HEVC codec mappings with BlockAdditionMapping 2 #390

JeromeMartinez · 2020-05-26T08:16:36Z

Another alternative of #377, using BlockAddIDType for storing 4-byte ISOBMFF values.
Discussion on #373 (comment).

codec_specs.md

mbunkus · 2020-05-26T12:12:45Z

ebml_matroska.xml

@@ -150,7 +150,7 @@
    <extension webm="1"/>
  </element>
  <element name="BlockAddID" path="\Segment\Cluster\BlockGroup\BlockAdditions\BlockMore\BlockAddID" id="0xEE" type="uinteger" range="not 0" default="1" minOccurs="1" maxOccurs="1">
-    <documentation lang="en" purpose="definition">An ID to identify the BlockAdditional level. A value of 1 means the BlockAdditional data is interpreted as additional data passed to the codec with the Block data.</documentation>
+    <documentation lang="en" purpose="definition">An ID to identify the BlockAdditional level. If BlockAddIDType of the corresponding block is 0, BlockAddID is considered as the corresponding BlockAddIDType.</documentation>


Same objection about the wording being confusing as in codec_specs.md.

mbunkus · 2020-05-26T12:13:34Z

ebml_matroska.xml

@@ -317,11 +317,11 @@
    <extension webm="0"/>
  </element>
  <element name="BlockAdditionMapping" path="\Segment\Tracks\TrackEntry\BlockAdditionMapping" id="0x41E4" type="master" minver="4">
-    <documentation lang="en" purpose="definition">Contains elements that describe each value of <a href="https://www.matroska.org/technical/specs/index.html#BlockAddID">BlockAddID</a> found in the Track.</documentation>
+    <documentation lang="en" purpose="definition">Contains elements that extend the track format, by adding content either to frames (with <a href="https://www.matroska.org/technical/specs/index.html#BlockAddID">BlockAddID</a>) or to the format definition</documentation>


Same objection as in 389: "the format definition" is a very ambiguous term, one that I wouldn't understand as an implementer. The "format definition" is e.g. ISO 14496-12. We don't extend that definition. What we do extend is codec-specific data present in the track headers.

MikeChenMM · 2020-05-28T22:53:50Z

codec_specs.md

+
+Description: the `BlockAddIDExtraData` data is interpreted as `DOVIDecoderConfigurationRecord` structure as defined in [Dolby Vision Streams File Format](ttps://www.dolby.com/us/en/technologies/dolby-vision/dolby-vision-bitstreams-within-the-iso-base-media-file-format-v2.1.2.pdf).
+
+### hvcE


Please note, Dolby Vision is also possible (and is defined) for AVC streams, not only for HEVC.
In case of AVC related IDs are dvcC and avcE . We probably should change dvcC to be allowed on both HEVC and AVC, and define avcE for DV on AVC.

That's a very good point, Mike. @JeromeMartinez can you please adjust your PR accordingly? The PDF even talks about using Dolby Vision with other codecs such as VP9.

Additionally the PDF also mentions dvvC atoms in addition to dvcC when talking about Dolby Vision configuration boxes. Are they relevant to us somehow?

I don't see where I limit dvcC to a specific Codec ID, I limit only for mvcC and hvcE.
I'll add dvvC and avcE to the list.

Please note, Dolby Vision is also possible (and is defined) for AVC streams, not only for HEVC.
In case of AVC related IDs are dvcC and avcE . We probably should change dvcC to be allowed on both HEVC and AVC, and define avcE for DV on AVC.

HEVC Dolby Vision is not supported? I have seen a (MKV HEVC) movie online and it says Dolby Vision + HDR

HEVC Dolby Vision is not supported?

The text says the opposite, it is explicitly supported for both AVC and HEVC (other formats could be supported by just adding the corresponding 4CC for the enhanced layer configuration).

JeromeMartinez · 2020-05-29T22:54:03Z

@mbunkus @MikeChenMM I updated the PR, please review again.

mbunkus

Apart from those two small things I'm fine with the current version. Thanks a lot!

I'd really like to see a review from @robUx4 , though, before we merge any of our proposals.

codec_specs.md

JeromeMartinez · 2020-05-30T20:57:17Z

Apart from those two small things [...]

Fixed.

I'd really like to see a review from @robUx4 , though, before we merge any of our proposals.

@robUx4 🙏

mbunkus

I'm sorry to request changes yet again, but due to merging #392 your PR doesn't apply anymore — solely due to the changed URLs to the new web site layout in the XML. Please rebase.

Otherwise I'm fine with it now.

Thanks!

JeromeMartinez · 2020-06-01T16:37:10Z

Please rebase.

Done.

mbunkus

Looks fine to me now. Thanks again!

JeromeMartinez · 2020-06-07T09:10:51Z

is it OK to merge this PR now?
a review from @robUx4 would be great ;-).

codec_specs.md

robUx4

The link to the DolbyVision spec should be turned into a reference.

There's also the question of the picture vs field terminology which I'm not sure about.

JeromeMartinez · 2020-06-07T20:15:17Z

@robUx4 I modified the PR accordingly, please review again.
Let me know if you prefer "elementary stream (ES)" instead of "picture".

robUx4 · 2020-06-08T14:19:08Z

Elementary Stream seems to be the whole stream rather than a picture. So if anything I would prefer picture.

I think it should be explained how to handled the interlacing a bit further if it can be one field, one picture (double field). At in which case should the FlagInterlaced of the Track should be set or not.

Just like for AV1 we should also explain how the fields of AVCDecoderConfigurationRecord should be mapped to the Matroska field. It's not a blocker for this PR but we should open an issue so we don't forget to do it.

JeromeMartinez · 2020-06-08T17:29:12Z

I think it should be explained how to handled the interlacing a bit further if it can be one field, one picture (double field). At in which case should the FlagInterlaced of the Track should be set or not.

FlagInterlaced (and other elements) setting is not indicated for others formats which can have similar issues e.g. MPEG-2 Video (ok, indicated "where block boundaries are still to be defined", so not the best example) or MPEG-4 Visual or ProRes (not possible to have 1 field per block, so easier, but still a lack of documentation), so this request seems generic to all formats supported in Matroska, and it is more an help for implementers than a codec mapping spec (see below) shouldn't it be a global request not blocking this PR rather than a blocker for this PR?

I added a commit for making explicit the idea behind the word picture, without more description e.g. about the mapping to other Matroska fields in order to avoid redundancy (more documentation about it would be in dedicated sections), would it be acceptable?

Just like for AV1 we should also explain how the fields of AVCDecoderConfigurationRecord should be mapped to the Matroska field. It's not a blocker for this PR but we should open an issue so we don't forget to do it.

AV1 documentation is the only one with such verbose description, more an help for developers than what should be in a specification, actually the only one with its own .md file, not sure it is good to do that in the standard itself.

It's not a blocker for this PR but we should open an issue so we don't forget to do it.

I tried to write an issue for that, as well as for FlagInterlaced, but actually I don't see the exact goal: should I open an issue per Matroska field, an issue per supported format, a global issue for all Matroska fields and all supported formats? It changes a lot the amount of content in codec_specs.md (or other md files linked to codec specs).

In my opinion there is no change really to do to this PR as is (so it could be merged), but rather a global quality update on all codec mappings, which should have its own issue tickets.

JeromeMartinez · 2020-06-14T09:25:15Z

nudge.
As AVC & HEVC codec mappings seems the only remaining issue, it is also possible to split the PR in 2 and merge the BlockAddiitonMapping part only, let me know if this split helps in merging the BlockAddiitonMapping part.
If Dolby Vision part is also an issue, the PR could also be limited to update BlockAddiitonMapping description + Opaque data + ITU T.35 metadata.

…ement

dyspr0sium · 2020-06-19T07:23:03Z

It may be of relevance to note that the original link for "Dolby Vision Streams Within the ISO Base Media File Format" is now broken, and has been moved to here.

It was added in ietf-wg-cellar#390 but that value is not allowed since ietf-wg-cellar#287. The Opaque data (used by WavPack) don't make use of that value since it's for data unknown to the codec.

It was added in #390 but that value is not allowed since #287. The Opaque data (used by WavPack) don't make use of that value since it's for data unknown to the codec.

It was added in ietf-wg-cellar#390 but that value is not allowed since ietf-wg-cellar#287. The Opaque data (used by WavPack) don't make use of that value since it's for data unknown to the codec.

JeromeMartinez mentioned this pull request May 26, 2020

Write down how extra data is saved for usage with AVC & HEVC #373

Closed

JeromeMartinez force-pushed the AVC_and_HEVC_BlockAddition2 branch from b9be6e4 to 354df6d Compare May 26, 2020 08:54

mbunkus requested changes May 26, 2020

View reviewed changes

mbunkus requested a review from robUx4 May 28, 2020 18:19

MikeChenMM reviewed May 28, 2020

View reviewed changes

JeromeMartinez force-pushed the AVC_and_HEVC_BlockAddition2 branch from 354df6d to 66a994b Compare May 29, 2020 22:53

JeromeMartinez force-pushed the AVC_and_HEVC_BlockAddition2 branch from 66a994b to 78a99c3 Compare May 29, 2020 23:12

mbunkus requested changes May 30, 2020

View reviewed changes

codec_specs.md Outdated Show resolved Hide resolved

codec_specs.md Outdated Show resolved Hide resolved

JeromeMartinez force-pushed the AVC_and_HEVC_BlockAddition2 branch from 78a99c3 to e255aad Compare May 30, 2020 20:56

mbunkus requested changes Jun 1, 2020

View reviewed changes

JeromeMartinez added 2 commits June 1, 2020 17:34

Add AVC and HEVC codec mappings

93f9726

More description of Block Addition Mappings

64dd1c9

JeromeMartinez force-pushed the AVC_and_HEVC_BlockAddition2 branch from e255aad to cd59963 Compare June 1, 2020 15:41

mbunkus approved these changes Jun 1, 2020

View reviewed changes

retokromer approved these changes Jun 1, 2020

View reviewed changes

robUx4 reviewed Jun 7, 2020

View reviewed changes