-
Notifications
You must be signed in to change notification settings - Fork 51
block additional mapping for smpte 12m timecode #348
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
Conversation
|
extremely excited to see this is happening |
|
|
||
| SMPTE 12 timecode values can be stored in the `BlockMore Element` to associate the content of a Matroska Block with a particular timecode value. | ||
|
|
||
| The Block Additional Mapping contains a full binary representation of a 64 bit SMPTE timecode value expressed exactly as defined in Section 7 and 8 of SMPTE 12M [@!SMPTE.12M.2014]. For convenience, here are the bit assignments from SMPTE 12M, for information: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we legally allowed to copy parts of this spec ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am copying parts of https://tools.ietf.org/html/rfc5484#section-6.2, but perhaps I should clarify that the text for convenience is derived from RFC5484 rather than a proprietary SMPTE document. ping to @mcr on this issue.
| | 56--57 | Tens of hours | | ||
| | 58 | Binary group flag BGF1 | | ||
| | 59 | Binary group flag BGF2 | | ||
| | 60--63 | Eighth binary group | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given it's "codec" opaque data format for Matroska, you should specify if it's in Little Endian or Big Endian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx, ping to @JeromeMartinez for a suggestion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference it is big endian in e.g. Dolby E, DV, GXF, MXF (SDTI or Ancillary data), MPEG-4 Visual.
So please big endian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dericed apologizes I wrote a bit too quickly.
In the listed formats, time code is not exactly SMPTE ST 12-1 (fields order are different, and none have the same order...), by "big endian" I mean that most significant bit of a byte is the most significant bit of tens or units, this is not like in Vorbis (the only format I am aware of that inverts totally the bits).
But "bit 0" is the most significant bit ("Units of frames 8" in SMPTE ST 12-1, which is more oriented to computers), so "big endian" wording may be misleading. maybe saying something like that bit 0 in this list is mapped to the most significant bit of the first byte.
Note that RFC 5484 does not make it explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeromeMartinez On page 15 of SMPTE ST-12-1, referencing the storage of the time address it says "The lowest numbered bit of each group corresponds to the least significant bit of each BCD digit" and on page 27, it referencing to the storage of the 4 bits within the binary groups and says "The lowest numbered bit of each group corresponds to the least significant bit of that group." Doesn't that imply that 'big 0' is the least significant bit ("Units of frames 1")?
|
|
||
| ### Timecode Description | ||
|
|
||
| SMPTE 12 timecode values can be stored in the `BlockMore Element` to associate the content of a Matroska Block with a particular timecode value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SMPTE ST 12-1 (source) is the new name of the format since SMPTE changed the naming of doc (no more "M", explicit info if it is RP, RDD, ST doc...), I suggest to use the new nae as we have no legacy stuff, also in other parts of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed locally, will force update soon
|
|
||
| ### BlockAddIDType | ||
|
|
||
| The BlockAddIDType value reserved for timecode is 12. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still reluctant with small BlockAddIDType values, due to risk of conflicts.
Anyway, this value should be added in a dedicated doc which lists the used BlockAddIDType instead of having to skim the whole spec for finding all already used values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding a summary table of all registered BlockAddIDType values, though only one so far. Would you prefer 121 over 12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer 121 over 12?
I would :), while understanding that "12" is more well known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
62ebf6a to
3afc719
Compare
|
Thanks to @JeromeMartinez and @robUx4 for reviews and @rfraimow for encouragement. The PR is updated based on comments. The mapping for smpte st12-1 is still missing a definition on what endianness to use for storage, waiting on @JeromeMartinez to suggest one. |
|
A few updates to respond to @JeromeMartinez's comments. |
| cat $^ | grep -v '^---' > $@ | ||
|
|
||
| $(OUTPUT_CODEC).md: index_codec.md codec_specs.md subtitles.md block_additional_mappings_intro.md rfc_backmatter_codec.md | ||
| $(OUTPUT_CODEC).md: index_codec.md codec_specs.md subtitles.md block_additional_mappings_intro.md block_additional_mappings/*.md rfc_backmatter_codec.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might conflict with #353, whichever comes first. But sometimes git is smart and notice the change is exactly the same and assumes the change will have no effect. Not sure about Github.
|
Nudging again on this PR. I also referring to this topic in a recent email to cellar at https://mailarchive.ietf.org/arch/msg/cellar/IQpWvbNgrOoP5K-odndjSdKFb2U. In my view, an interest to handle timecode in an additional way, doesn't necessarily block consideration of handling timecode as side data. |
robUx4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with me. The endianness is not a big deal as it's already defined by the external spec and existing content.
|
FYI, I made an example of this at https://github.com/Matroska-Org/matroska-test-files/pull/5/files. I set MaxBlockAdditionID to 5 in anticipate of the reservation we need to do for Google's use of HDR. |
|
Is there any reason for not merging this PR? |
|
No reason. I think #353 is the same with additional changes so we can merge this one and it will only have the extra commits to merge. |
This doesn't build properly as the SMPTE.12M citation doesn't work. At the moment I'm not remembering well how to cite in using the older version of mmark and xml2rfc so I'm just putting this here for comment. I'll rebase after this repo is at the newer mmark and xml2rfc.