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 BlockAdditionMapping to describe each BlockAdditional data #287

Merged
merged 16 commits into from
Oct 22, 2019

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Nov 18, 2018

This is similar to a TrackID + CodecID + CodecPrivate + TrackName but for extra frame data.

The Timecode and RAWCooked values may be removed and done in a separate spec. They need to define properly how to interpret the BlockAddIDExtraData binary data.

@JeromeMartinez
Copy link
Contributor

IMO it should be in https://github.com/Matroska-Org/matroska-specification/pull/276/files directly.

enum restriction: I am currently not in favor to restrict the list, as we don't limits e.g. tags.

@robUx4
Copy link
Contributor Author

robUx4 commented Nov 18, 2018

IMO it should be in https://github.com/Matroska-Org/matroska-specification/pull/276/files directly.

What do you mean by "it" ? As discussed on CELLAR putting a name and differentiate string/binary for something that can already be done with BlockAdditional doesn't seem like the right way to go. The lace number may be an issue, but it can certainly be added to BlockMore.

enum restriction: I am currently not in favor to restrict the list, as we don't limits e.g. tags.

The enum is open-ended, there can be other values. But if I'm going to mention that, then I'll probably remove Timecode and RAWCooked to mention that currently only value 0 is defined.

@JeromeMartinez
Copy link
Contributor

What do you mean by "it" ?

OK, I am lost now.
I understand that both PRs resolves half of the request, so IMO both PRs should be merged in one and discussed in a single place.
This PR alone is not useful, if I understand it well, because there isn't the associated Block part.

@robUx4 robUx4 force-pushed the block_addition_mapping branch from 2c6fd74 to cc2d785 Compare November 18, 2018 14:40
@robUx4
Copy link
Contributor Author

robUx4 commented Nov 18, 2018

This PR solves the current situation where there are BlockAddID and BlockAdditional but it doesn't say what to do with the data. Now we say the current use is a complement of data to give to the codec. But there can be other cases of metadata attached to each frame that don't fall in this category.

The PRs are separated. #276 should be updated to use this new system (thus it comes after).

@t-rapp
Copy link
Contributor

t-rapp commented Nov 19, 2018

The enum is open-ended, there can be other values. But if I'm going to mention that, then I'll probably remove Timecode and RAWCooked to mention that currently only value 0 is defined.

I would appreciate it if Timecode would not be mentioned here. It looks like a premature standardization of timecode storage format to me. Rather allow users to make their own BlockAddIDType definition (if really necessary) possibly with some reserved range for later standardization.

@mjbshaw
Copy link
Contributor

mjbshaw commented Nov 20, 2018

It would be nice to clarify how new BlockAddIDTypes can be registered. We at Google are considering adding some new side metadata for frames within WebM (using BlockAdditions). I don't have any concrete details I can share about this metadata right now, but it would be nice to know what the procedure is for standardizing it so we (and others) can utilize that procedure.

@dericed
Copy link
Contributor

dericed commented Nov 25, 2018

enum restriction: I am currently not in favor to restrict the list, as we don't limits e.g. tags.

The enum is open-ended, there can be other values. But if I'm going to mention that, then I'll probably remove Timecode and RAWCooked to mention that currently only value 0 is defined.

The EBML specification implies that enum lists are restrictive, not open. https://github.com/Matroska-Org/ebml-specification/blob/master/specification.markdown#restriction-element

ebml_matroska.xml Outdated Show resolved Hide resolved
ebml_matroska.xml Outdated Show resolved Hide resolved
notes.md Outdated Show resolved Hide resolved
@dericed dericed force-pushed the block_addition_mapping branch from cc2d785 to e6d2191 Compare November 25, 2018 22:20
@dericed
Copy link
Contributor

dericed commented Nov 25, 2018

Hope you don't mind me working in the same branch @robUx4. I added a few commits here to start to define how to define BlockAdditions for Codec Mappings. So far, I think they are used for A_WAVPACK4 (for the lossless part) and VP8/VP9 (for alpha encodings). I haven't found others, but do they exist? Then IIUC correctly, the values of 2 and greater are arbitrarily used to relate BlockAdditional to the new BlockAddition mappings.

Copy link
Contributor Author

@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.

That's what collaboration is for :)
I don't think there are other uses of BlockAdditions but then I forgot about VPx alpha...

codec_specs.md Outdated Show resolved Hide resolved
codec_specs.md Outdated Show resolved Hide resolved
codec_specs.md Outdated Show resolved Hide resolved
ebml_matroska.xml Outdated Show resolved Hide resolved
@robUx4 robUx4 added the spec_main Main Matroska spec document target label Dec 18, 2018
@dericed
Copy link
Contributor

dericed commented Jul 30, 2019

@robUx4 can you review the approach I'm testing in 3d5e52d. This moves the Block Additional Mappings to be handled similarly to Codec Mappings, rather than enumerating them in the schema.

@dericed dericed force-pushed the block_addition_mapping branch 2 times, most recently from e4d7c12 to 74294a2 Compare September 14, 2019 21:35
@dericed
Copy link
Contributor

dericed commented Sep 14, 2019

@robUx4 I updated this PR to start a block_additional_mapping (timecode as a string in this example) as a standalone document. I think the timecode document could be expanded to allow BlockAddIDExtraData to support different types of timecode such as expressions of SMPTE 12M or QuickTime's timecode descriptor of QuickTime Timecode Sample Data via https://github.com/cellar-wg/matroska-specification/pull/112/files.

However in this PR, timecode is mostly here as a first example while the PR more so focuses on the Block Additional Mapping section and how to define the mappings.

notes.md Outdated Show resolved Hide resolved
notes.md Outdated Show resolved Hide resolved
@mjbshaw
Copy link
Contributor

mjbshaw commented Sep 24, 2019

I can finally talk about what I alluded to earlier. YouTube now ingests and serves HDR10+ dynamic metadata. BlockAddID == 4 is used for VP9 to signal that the BlockAdditional payload is ITU T.35 metadata as defined by ITU-T T.35 terminal codes. I think it might make sense to have some kind of "registry" or something for BlockAddIDType. Without that I think various muxer implementations will diverge in the IDs they use for various types of metadata.

@dericed
Copy link
Contributor

dericed commented Oct 12, 2019

Hi @robUx4, I did a little redesign of this PR so now there's a Block Additional Mapping introduction in its own document with an example in XML. In the Makefile this is add along with all markdown within the block_additional_mapping directory into the end of the Codec document. Please re-review.

@dericed
Copy link
Contributor

dericed commented Oct 12, 2019

My recent updates don't accommodate @mjbshaw's comment at #287 (comment) as I think that would be better handled in a later PR as we'll have to discuss reserving BlockAddID=4.

ebml_matroska.xml Outdated Show resolved Hide resolved
@robUx4
Copy link
Contributor Author

robUx4 commented Oct 13, 2019

I can finally talk about what I alluded to earlier. YouTube now ingests and serves HDR10+ dynamic metadata. BlockAddID == 4 is used for VP9 to signal that the BlockAdditional payload is ITU T.35 metadata as defined by ITU-T T.35 terminal codes. I think it might make sense to have some kind of "registry" or something for BlockAddIDType. Without that I think various muxer implementations will diverge in the IDs they use for various types of metadata.

Thanks for the tip @mjbshaw !
I think we should handle independant BlockAdditional types (ie not related as separate specs), just as it should be done for codecs (see the AV1 specs). At first we'll probably include a lot of common ones in the main codec specs.

BlockAddID of 1 is already covered by these changes. For the value of 4 it should theoretically be combined with a BlockAddIDValue in the Track header. It seems unlikely YouTube will support that, since the spec is not even finalized yet. So if we want to be coherent we should treat value 4 as value 1, as a reserved/preexisting value. 1 is codec specific (ie data passed to the decoder with each frame), while 4 seems to be more metadata added to the frame.

MaxBlockAdditionID is not supported by WebM either, so we can't really rely on that to know in advance what BlockAddID will be found in the file or not.

On our side we should probably mention that BlockAddID value 4 is also a reserved value and explain what it does. I created #345 so we don't forget to do it once this PR is merged.

@dericed
Copy link
Contributor

dericed commented Oct 16, 2019

I reviewed the recent changes and this seems ok to me. There is more work to do on timecode; however that isn't the focus on this PR. I suggest to merge and then we'll follow up with both timecode refinement and HDR.

@dericed dericed mentioned this pull request Oct 19, 2019
Copy link
Contributor Author

@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 adjustments are needed. Or we can split this PR to cover the specific case of timecode BlockAddition separately.

ebml_matroska.xml Outdated Show resolved Hide resolved
block_additional_mappings/timecode.md Outdated Show resolved Hide resolved
block_additional_mappings/timecode.md Outdated Show resolved Hide resolved
@dericed dericed force-pushed the block_addition_mapping branch from cfae516 to dc368ae Compare October 22, 2019 15:34
@dericed
Copy link
Contributor

dericed commented Oct 22, 2019

This PR is ready for a review. I removed the timecode block additional mapping (which was here as an example) to separate the discussions, but this focuses on the new elements for block additional mappings and how they are defined.

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 22, 2019

LGTM

@dericed
Copy link
Contributor

dericed commented Oct 22, 2019

Merging then :)

@dericed dericed merged commit 87ffd39 into master Oct 22, 2019
@dericed dericed deleted the block_addition_mapping branch February 23, 2021 20:53
@robUx4 robUx4 mentioned this pull request Apr 4, 2021
robUx4 added a commit to robUx4/matroska-specification that referenced this pull request Dec 1, 2024
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.
robUx4 added a commit that referenced this pull request Dec 8, 2024
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.
robUx4 added a commit to robUx4/matroska-specification that referenced this pull request Dec 14, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format addition spec_main Main Matroska spec document target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants