ietf-wg-cellar / matroska-specification

Matroska specification.
http://ietf-wg-cellar.github.io/matroska-specification
Other
121 stars 45 forks source link

Adjust FieldOrder enum labels to better match description texts #819

Closed t-rapp closed 2 months ago

t-rapp commented 4 months ago

In #162 the description for FieldOrder values have been fixed to match current practice. After the change the enum labels for FieldOrder values "9" and "14" do not align with the longer description text anymore, so adjust the labels accordingly.

t-rapp commented 4 months ago

Sidenote: Maybe the definition could also be extended for FieldOrder values "1" and "6" to make it explicit that they shall be used when fields are stored separately - in contrast to interleaved field storage.

dericed commented 4 months ago

The original field order descriptions were based on the vocabulary list used under the QuickTime specification. There was a long discussion of it here, including this post from David Singer who works in standards at Apple.

robUx4 commented 4 months ago

IMO as long as the description is correct, the label doesn't matter too much.

Given we're in a phase of RFC editor, maybe we can change it during the last edits. Or it could be en errata. But we also generate code from these names, for example the enums in libmatroska 1.x (current stable version): https://github.com/Matroska-Org/libmatroska/blob/v1.x/matroska/KaxSemantic.h#L934. Changing the label would break the API (or we need to add some hacks to keep the old name one way or another).

robUx4 commented 3 months ago

IMO as long as the description is correct, the label doesn't matter too much.

I just realize the tff/bff labels don't match the text. One of them does need to be changed. The confusion originates from fa55cbe3bd2c0e27fb54980a8cd72e5b49755822 which was mostly a formatting change.

Not sure when this element was introduced but it was defined like that in the original schema introduction: b23e90ee978bfa26066076980281945a8e16cead.

Looking at the old "official" spec it was there before the spec was generated from that schema: https://web.archive.org/web/20160331001805/https://matroska.org/technical/specs/index.html . So the values and meaning should match that. Unless these values are similar to other formats.

robUx4 commented 3 months ago

So the changes are the opposite, the text should match the label, and not the other way around.

t-rapp commented 3 months ago

There was agreement in the discussion at vrecord/#170 that the actual practice for field-order values in MOV is:

I don't know exactly when, but at some point between v4.3 and v6.1 the MOV and Matroska muxer implementation in FFmpeg was changed so that TFF is now written with value "9" instead of "1", and BFF with "14" instead of "6" (for the common interlaced field storage), which was the original reason for me to dig into the Matroska specs and these old discussions.

Another thing that still causes confusion (for me), is the slightly different definition for "field order" / "field dominance" used with MXF. That was also the reason why the constant names in FFmpeg haven't been adjusted accordingly, IIRC.

robUx4 commented 3 months ago

So the meaning was changed in #162, but the labels were left unchanged. This is unfortunate. But it seems to match some QuickTime spec, so I guess it should match that as well.

It's currently not used in VLC. The other main user of the generated spec is mkvtoolnix. It seems that it's also not using the define so it should not be impacted. However the documentation in mkvtoolnix still uses the old meaning. Do you agree with this changed @mbunkus ?

mbunkus commented 3 months ago

I do. I'll adjust MKVToolNix accordingly.

robUx4 commented 3 months ago

OK, let's discuss the merging in the meeting tonight. As the RFC is pending publication, this may become an errata.

robUx4 commented 3 months ago

So this will go in the AUTH48 phase of the document, which is the last edits we make before the RFC is published.

robUx4 commented 2 months ago

Merging as we go the AUTH48 review and we can integrate last minute changes.