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

BlockAddIDValue 4 is not recommended #745

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeromeMartinez
Copy link
Contributor

As BlockAddIDValue of 4 may be interpreted as ITU T.35 without checking BlockAddIDType (reason we mandate BlockAddIDValue when BlockAddIDType is 4, right?), I suggest to recommend to avoid BlockAddIDValue of 4 when it is not ITU T.35.

Idea is from a comment at https://ffmpeg.org/pipermail/ffmpeg-devel/2023-March/307718.html

@robUx4 robUx4 added the spec_codecs Codec Matroska spec document target label Mar 25, 2023
@robUx4
Copy link
Contributor

robUx4 commented Mar 25, 2023

The values of BlockAddID that are 2 of greater have no semantic meaning

Any value of 2 or higher just means that BlockAddIDValue has to be used and nothing else. Giving 4 a special meaning here sounds like going backward. However, looking at libwebm, they completely ignore the BlockAddIDValue.

Looking at the WebM spec, they don't support the element either.

It might be better to have a generic rule that says the BlockAddID and BlockAddIDValue should have the same value (except for BlockAddID 0 and 1), for the same compatibility WebM issues.

@jamrial
Copy link

jamrial commented Mar 25, 2023

It might be better to have a generic rule that says the BlockAddID and BlockAddIDValue should have the same value (except for BlockAddID 0 and 1), for the same compatibility WebM issues.

But that's already the case. BlockAddIDValue maps a value to a BlockAddIDType, so that all BlockAdditional with BlockAddID equal to it within the track are to be interpreted as the type referenced by said BlockAddIDType.
That means that i could add a mapping assigning BlockAddIDValue 4 to BlockAddIDType 121, so that all BlockAdditional with BlockAddID 4 would need to be interpreted as SMPTE ST 12-1 timecodes, but a WebM oriented parser would most assuredly ignore that and try to interpret those BlockAdditional as HDR10+ in a ITU-T T.35 payload, and fail doing so.
This change is recommending not using 4 as BlockAddIDValue in a mapping if it's not for the BlockAddIDType 4 for the sake of compatibility with WebM.

@robUx4
Copy link
Contributor

robUx4 commented Mar 25, 2023

But that's already the case. BlockAddIDValue maps a value to a BlockAddIDType,

You're right, that's written in a different section:

where the BlockAddID Element of BlockAdditional Element
equals the BlockAddIDValue of the associated Track's BlockAdditionalMapping Element.

Technically in modern Matroska the most important field is BlockAddIDType, but WebM doesn't know about that either.

So you're (both) right about not using the value 4 for anything else than ITU-T T.35 payload. However I see 2 caveats

  • we are defining the Matroska specs, not the WebM spec. It seems harsh to enforce this for muxer that will never write WebM. If they do want to write ITU-T T.35 payload in WebM they can't use the other elements we would use, so they need a special case anyway
  • if WebM decides to use values 2, 3, 5, 6, 42, etc we're not going to block these values in Matroska (especially if they are already in use)

So I would prefer to mention that WebM does things differently, especially the missing elements, as the date of writing. But not block the value 4 in Matroska.

codec_specs.md Outdated Show resolved Hide resolved
codec_specs.md Outdated Show resolved Hide resolved
@robUx4
Copy link
Contributor

robUx4 commented Mar 25, 2023

BTW, it would be interresting (actually important) to know whether mkvmerge ever wrote Matroska files with ITU T.35 data without the corresponding BlockAddIDType, in other words the WebM way with a matroska DocType. If that's the case it's no longer a WebM specific issue,

cc @mbunkus

@JeromeMartinez
Copy link
Contributor Author

we are defining the Matroska specs, not the WebM spec.

In that case, why do we mandate "BlockAddIDValue MUST be 4." for ITU T T.35?

I think that this PR is for making explicit that 4 has a special meaning because it is mandated for ITU T T.35, so if we don't make explicit that value 4 is special because we are not dealing with WebM stuff, ITU T T.35 could use any value.

if WebM decides to use values 2, 3, 5, 6, 42, etc we're not going to block these values in Matroska (especially if they are already in use)

True, we could add later other unrecommended values if WebM future extension does not use BlockAddIDType. It is a choice. But again, if we don't care about WebM, I see no reason to block the BlockAddIDValue 4 for ITU T T.35.

@robUx4
Copy link
Contributor

robUx4 commented Mar 25, 2023

In that case, why do we mandate "BlockAddIDValue MUST be 4." for ITU T T.35?

If there's a reason it should be in #390

@JeromeMartinez
Copy link
Contributor Author

If there's a reason it should be in

My understanding is that BlockAddID of 4 is in practice reserved to ITU T T.35 for avoiding interference with WebM.
We need to decide if we want to recommend that value only for ITU T T.35 or not.

  • If yes: this PR makes sense IMO, in order to be explicit everywhere (in BlockAddIDType part and BlockAddID part)
  • If no: need a PR for removing this mandatory value for ITU T T.35 in BlockAddIDType part.

with current version it is something in the middle, like if we don't want to really decide :-p.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 25, 2023

BTW, it would be interresting (actually important) to know whether mkvmerge ever wrote Matroska files with ITU T.35 data without the corresponding BlockAddIDType

No. MKVToolNix only received support for those elements in August 2020 while adding support for Dolby Vision. mkvmerge can create the elements for Dolby Vision streams and it will keep existing elements no matter their value if they're present in source files, but it doesn't create them outside of Dolby Vision support.

@robUx4
Copy link
Contributor

robUx4 commented Mar 25, 2023

IMO the BlockAddIDValue MUST be 4 (and BlockAddIDValue MUST be 1) text should mention it's because of WebM compatibility. Then repeating both constraints in the section you're modifying would be useful too.

BTW the value 1 mentions it's for opaque data (whatever that means) and the WebM spec says it's reserved (mysterious). But in fact that value is for the alpha layer for VP8 and VP9. It's even a VP8/VP9 bitstream (that libavcodec can't handle, only libvpx can). We should probably clarify that as well. When combined with these codecs (and possibly AV1) the BlockAddIDValue of 1 means something special in WebM (and in Matroska too if simply remuxed without interpreting the data).

@jamrial
Copy link

jamrial commented Mar 25, 2023

If no: need a PR for removing this mandatory value for ITU T T.35 in BlockAddIDType part.

Please don't do that. It will make writing demuxing implementations that only care about HDR10+ much harder (Actually having to create a mapping array to know what BlockAddID corresponds to it). Lets recommend people to not use 4 for anything other than ITU-T T.35, and not redefine the type constrains.

@JeromeMartinez
Copy link
Contributor Author

It will make writing demuxing implementations that only care about HDR10+ much harder (Actually having to create a mapping array to know what BlockAddID corresponds to it).

I didn't check your FFmpeg patch about that, but if you don't handle a mapping array, you need at least to flag if BlockAddID 4 is ITU T T.35 or not, as currently BlockAddID 4 does not necessary mean that it is ITU T T.35, so at least you need to have a flag, so not a big difference between a flag and a mapping array.

a demuxer SHOULD NOT consider BlockAddID 4 as ITU T T.35, only if BlockAddIDType is 4.

@JeromeMartinez
Copy link
Contributor Author

(and at long term FFmpeg will need this mapping array, for e.g. timecodes)

@jamrial
Copy link

jamrial commented Mar 25, 2023

I didn't check your FFmpeg patch about that, but if you don't handle a mapping array, you need at least to flag if BlockAddID 4 is ITU T T.35 or not, as currently BlockAddID 4 does not necessary mean that it is ITU T T.35, so at least you need to have a flag, so not a big difference between a flag and a mapping array.

I'm doing as much, yes. A simple flag that ensures any BlockAddID with value 4 are only parsed if a mapping with both BlockAddIDType and BlockAddIDValue having a value of 4 is present for the track, or if the file is WebM. Otherwise they are ignored.

(and at long term FFmpeg will need this mapping array, for e.g. timecodes)

Yes, when the time comes, an array will have to be implemented.

@JeromeMartinez
Copy link
Contributor Author

text should mention it's because of WebM compatibility

I added a mention in BlockAddIDType part.

BTW the value 1 mentions

It seems clear enough to me, so not sure about what to add there.

@robUx4
Copy link
Contributor

robUx4 commented Mar 27, 2023

In one part of the text the requirement is a SHOULD and the other it's a MUST (we already established BlockAddID and BlockAddIDValue MUST have the same value). We have to pick one. IMO it's a SHOULD because people not caring of WebM remuxing (not using any of their codec) should not be forced to set MaxBlockAdditionID to 4 because WebM did it wrong.

@JeromeMartinez
Copy link
Contributor Author

In one part of the text the requirement is a SHOULD and the other it's a MUST

It is not incompatible as a format X could use BlockAddIDValue of 4 (doing something not recommended) while not caring about T.35, and impact of MUST for T.35 only forces the remuxer to change the BlockAddIDValue of this format X.

But it shows that we are a bit in the middle, we want to keep some compatibility with WebM and at the same time we don't really care, and we don't choose between SHOULD and MUST.

For coherency, I would put SHOULD everywhere, but @jamrial has another opinion. in my opinion we should not have reserved value without Matroska related meaning, and the mapping array would have to be done for any other format, in practice if on T.35 is handled it is mostly replacing a bool ("is BlockAddIDValue of 4 is for T.35") by a int ("BlockAddIDValue for T.35 is Y") so we should change that now, before an implementation is wide spread.

@jamrial
Copy link

jamrial commented Mar 27, 2023

It is not incompatible as a format X could use BlockAddIDValue of 4 (doing something not recommended) while not caring about T.35, and impact of MUST for T.35 only forces the remuxer to change the BlockAddIDValue of this format X.

A remuxer is in fact encouraged to pretty much always change the BlockAddIDValue value for any mapping it needs to create (as long as they follow the constrains for each type) in order to get the lowest possible MaxBlockAdditionID, as the description for that element recommends to do.
Say the input has timecodes (BlockAddIDType 121) mapped with a BlockAddIDValue of 9999, it would be silly to reuse that just because you're copying the stream unchanged. The remuxer would use the lowest available value >= 2, and the constrain currently in place for BlockAddIDType 4 alongside the recommendation added by this MR would make it simply skip using 4 for it as long as it can or cares about muxing ITU-T T.35 payloads, or knows it will do it for this specific Track, or because it already did.

But it shows that we are a bit in the middle, we want to keep some compatibility with WebM and at the same time we don't really care, and we don't choose between SHOULD and MUST.

We are already constraining BlockAddIDType 1 to BlockAddIDValue 1 for alleged backwards compatibility, which is strictly speaking that WebM uses it for Alpha channel in VP8 and VP9 (I don't think there was a Matroska specific scenario where BlockAddID 1 was used before the mapping elements were introduced). Doing the same for BlockAddIDType 4 was considered acceptable back then, and IMO still is. We're not constraining the BlockAddIDValue 4 to be used exclusively for ITU-T T.35, we're recommending people to not use it for other types if possible. This is in contrast with BlockAddIDValue 1 which is in fact only allowed to be used for BlockAddIDType 0 and 1, hence the >= 2 allowed range.

So in short, the combination of BlockAddIDType == 4 MUST have BlockAddIDValue == 4 and BlockAddIDType != 4 SHOULD NOT use BlockAddIDValue == 4 is completely acceptable. I see no benefit at all and quite a few headaches by removing the constrain.

@@ -88,6 +88,10 @@ The values of `BlockAddID` that are 2 of greater have no semantic meaning, but s
associate the `BlockMore Element` with a `BlockAdditionMapping` of the associated Track.
See (#block-additional-mapping) on Block Additional Mappings for more information.

The value of 4 for `BlockAddID` (so also `BlockAddIDValue`) **SHOULD NOT** be used when `BlockAddIDType` is not 4,
Copy link

Choose a reason for hiding this comment

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

Maybe instead of

The value of 4 for BlockAddID (so also BlockAddIDValue) SHOULD NOT be used when BlockAddIDType is not 4

you could simply say

It is recommended to not use the value of 4 for BlockAddID (and by extension BlockAddIDValue) when BlockAddIDType is not 4

To avoid using the SHOULD NOT keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK with that, although it should be **RECOMMENDED** as it's a normative term in the case.

@@ -977,7 +981,8 @@ Block type identifier: 4
Block type name: ITU T.35 metadata

Description: the `BlockAdditional` data is interpreted as ITU T.35 metadata, as defined by ITU-T T.35
terminal codes. `BlockAddIDValue` **MUST** be 4.
terminal codes. `BlockAddIDValue` **MUST** be 4 as some WebM-oriented demuxers may ignore `BlockAddIDType`
Copy link

Choose a reason for hiding this comment

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

I don't know if this addition is a good idea. BlockAddIDValue MUST be 4, period. The reason why does not need to be included. It wasn't for BlockAddIDType 1.

@robUx4
Copy link
Contributor

robUx4 commented Mar 28, 2023

A remuxer is in fact encouraged to pretty much always change the BlockAddIDValue value for any mapping it needs to create (as long as they follow the constrains for each type) in order to get the lowest possible MaxBlockAdditionID, as the description for that element recommends to do.

This is even more to when remuxing from Matroska to WebM (the other way around can be totally transparent). WebM has a lot of missing elements, only supports a few codec, also has constraints on the interleaving (it is recommended that an audio block starts each Cluster). Such a muxer needs to make sure all the WebM constraints.

What we're defining here is an easier way to make that translation. But in the end, since this is just a recommendation on the Matroska side, the not recommended case needs to be handled. So whether it's a SHOULD or RECOMMENDED doesn't make much difference. If it becomes a MUST then it can be assumed the source Matroska file would be bogus/invalid. This would become retroactive on all Matroska files having T.35 content. It's probably OK as mkvmerge was already doing it properly.

But as said before, I'm not very keen on imposing WebM rules (at least bogus ones) in Matroska.

@jamrial
Copy link

jamrial commented Apr 8, 2023

So in short, the combination of BlockAddIDType == 4 MUST have BlockAddIDValue == 4 and BlockAddIDType != 4 SHOULD NOT use BlockAddIDValue == 4 is completely acceptable. I see no benefit at all and quite a few headaches by removing the constrain.

Ok, thinking a bit more about this, i realize now that forcing BlockAddIDType 4 to always have BlockAddIDValue 4 means we're forcing a given Track to only have a single kind of ITU-T T.35 BlockAdditions, more likely than not HDR10+ payloads since that's currently what seems to have real world use (Does it, though? Does Google serve this on Youtube? Or just the one sample as a PoC?), but not necessarily.
Is this desirable? Are there no other kinds of T.35 payloads that could be needed by a Block, like for example atsc closed captions, which would need to be signaled with some other BlockAddID value also mapped to BlockAddIDType 4?

@mbunkus
Copy link
Contributor

mbunkus commented Apr 9, 2023

Ok, thinking a bit more about this, i realize now that forcing BlockAddIDType 4 to always have BlockAddIDValue 4 means we're forcing a given Track to only have a single kind of ITU-T T.35 BlockAdditions

I don't follow:

  • If you need a BlockAddition with an ITU-T T.35 payload that isn't the existing HDR content, just reserve a different type for it with a narrower/more precise description than the existing type 4, e.g. "ITU-T T.35 chunky bacon data as specified by document ITU-T T.35 2026.C revision 2 section 15 subsection 2 table 12".
  • If you need several BlockAdditions for the same BlockGroup with HDR content that is compatible, just use two BlockMore with ID 4 (and only one mapping with ID 4, type 4).

In other words, maybe the type description for type 4 is too broad; maybe it should be much more narrow & concrete.

@jamrial
Copy link

jamrial commented Apr 10, 2023

Yeah, you're right. I can add two BlockMore just fine to a Block both using the enforced id 4 and with different kind of payloads. Nevermind then.

@JeromeMartinez
Copy link
Contributor Author

In other words, maybe the type description for type 4 is too broad; maybe it should be much more narrow & concrete.

It should be broad, because it could be considered as opaque data by a remuxer. All a remuxer needs to know is that it is ITU-T T35.

My understanding is that in that case it is a generic method for all block types, we add more BlockMore.

BTW, HDR10+ metadata parsing has been added in MediaInfo, and it is ready to support more ITU-T T35 content (what we found there up to now, in e.g. HEVC, is not only captions, but also other HDR formats like SL-HDR and HDR Vivid, and active format description).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec_codecs Codec Matroska spec document target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants