Clarification about when CodecPrivate is mandatory #446

JeromeMartinez · 2020-12-27T22:24:59Z

More wording about when CodecPrivate is mandatory, following https://mailarchive.ietf.org/arch/msg/cellar/8vUqFFDT-Dqu1kc2ZwE_qOBvH8M/.

Also comply to the sentence:

Each encoding supported for storage in Matroska MUST have a defined Initialization.

And use the same wording Private Data everywhere.

I am not 100% sure about the Initialization: none (when nothing was previously written, I consider that there is no init), please double-check.

ebml_matroska.xml

codec_specs.md

JeromeMartinez · 2020-12-28T21:59:07Z

@mbunkus please review again.

mbunkus

Thanks. I just noticed two more things, one of which is factually wrong in the original already, the other being a question of style.

I'd be fine having both of those issues resolved in separate MRs (especially the one about verbiage as there are a lot more pieces of text that would benefit from similar scrutiny) and accepting this one as-is. Thoughts?

mbunkus · 2020-12-28T22:04:23Z

codec_specs.md

 For more information, see (#ssa-ass-subtitles) on SSA/ASS.

+Initialization: The `Private Data` contains the [Script Info] and [V4 Styles] sections.


Unfortunately, the original text was wrong. For S_TEXT/ASS the two sections are [Script Info] (this one was correct) and [V4+ Styles] (this one was incorrect; it's [V4 Styles] without the + for SSA but [V4+ Styles] with the + for ASS). So let's correct it while we touch this part anyway.

Thanks!

mbunkus · 2020-12-28T22:07:35Z

codec_specs.md

-**SHOULD NOT** be written and **MUST** be ignored. Data that is defined Initialization to be
+**SHOULD NOT** be written and **MUST** be ignored. If the encoding does require any form of Initialization,
+then `CodecPrivate Element` **MUST** be written and **MUST** be provided to the decoder.
+Data that is defined Initialization to be


I know that this sentence wasn't changed, but to be honest, I didn't like the original anyway. Maybe we can change it right now, too. On the other hand changing verbiage might be better handled as a separate MR.

If we wanted to change it now, the following might be better:

The Initialization data to be stored in the CodecPrivate Element is referred to as Private Data.

Again, I'd be fine accepting this MR without this particular change.

If we wanted to change it now, the following might be better

Obvious bad typos, I fixed them in the same commit. Please review again.

robUx4

Mostly OK apart from 'none' which IMO should also accept the empty value (meaning default value in Matroska). Especially for codecs that can have 'none' and some actual value. Or maybe they should NOT have 'none' but define the default value instead. (which would also save a tiny bit of space).
It might be tricky to define a default value though. It could be a large binary blob (Vorbis).

robUx4 · 2021-01-02T09:18:15Z

ebml_matroska.xml

@@ -398,7 +398,7 @@ If this Element is used, then any Language Elements used in the same TrackEntry
 see [@!I-D.ietf-cellar-codec] for more info.</documentation>
  </element>
  <element name="CodecPrivate" path="\Segment\Tracks\TrackEntry\CodecPrivate" id="0x63A2" type="binary" maxOccurs="1">
-    <documentation lang="en" purpose="definition">Private data only known to the codec.</documentation>
+    <documentation lang="en" purpose="definition">Private data only known to the codec. This element **MUST NOT** be present if the codec mapping specification defines no initialization or an initialization `none`, else **MUST** be present.</documentation>


IMO present and with a size of 0 would also mean 'none'. For example in VP9 you can have one to give the 10/12 bits profiles. But there may be none. IMO writing an empty field is better than no field at all in this case. It's clear that it's the default profile. (there are tons of 10 bits VP9 files with no CodecPrivate and there's no way to tell if they are 8 or 10 bits).

This is also in line with

If the encoding does require any form of Initialization then CodecPrivate Element MUST be written

V_PRORES would probably benefit from that as well.

Since we're trying to stay away from "empty elements" this comment is not valid anymore.

The VP9 case still stands. Although there is some definition of what to put in the CodecPrivate, in most cases the element is not there at all. It doesn't fall in neither the no initialization nor the initialization to 'none' categories. So in the end it should be up to the codec to decide when it makes sense to put it and what it means when it's not there (for VP9 that means 8 bits and some unknown profile).

poke

IMO this addition is not good, it's up to the codec to tell what it means if it's present or not.

codec_specs.md

robUx4 · 2021-01-02T09:29:47Z

codec_specs.md


 For each line containing the timestamp and file position data is read from the appropriate
 position in the .sub file. This data consists of a MPEG program stream which in turn
 contains SPU packets. The MPEG program stream data is discarded, and each SPU packet
 is put into one Matroska frame.

+Initialization: The .idx file is stripped of all empty lines, of all comments and of lines beginning with `alt:` or `langidx:`.
+The line beginning with `id:` **SHOULD** be transformed into the appropriate Matroska track language element


The SHOULD should probably be a MUST and mention that the line is discarded. But the wording for this whole codec is questionable and could be fixed separately.

poke

If we leave SHOULD we have to explain in which case do use it and in which case we don't.

robUx4 · 2021-11-28T08:42:22Z

ebml_matroska.xml

@@ -398,7 +398,7 @@ If this Element is used, then any Language Elements used in the same TrackEntry
 see [@!I-D.ietf-cellar-codec] for more info.</documentation>
  </element>
  <element name="CodecPrivate" path="\Segment\Tracks\TrackEntry\CodecPrivate" id="0x63A2" type="binary" maxOccurs="1">
-    <documentation lang="en" purpose="definition">Private data only known to the codec.</documentation>
+    <documentation lang="en" purpose="definition">Private data only known to the codec. This element **MUST NOT** be present if the codec mapping specification defines no initialization or an initialization `none`, else **MUST** be present.</documentation>


poke

IMO this addition is not good, it's up to the codec to tell what it means if it's present or not.

robUx4 · 2021-11-28T08:43:41Z

codec_specs.md


 For each line containing the timestamp and file position data is read from the appropriate
 position in the .sub file. This data consists of a MPEG program stream which in turn
 contains SPU packets. The MPEG program stream data is discarded, and each SPU packet
 is put into one Matroska frame.

+Initialization: The .idx file is stripped of all empty lines, of all comments and of lines beginning with `alt:` or `langidx:`.
+The line beginning with `id:` **SHOULD** be transformed into the appropriate Matroska track language element


poke

If we leave SHOULD we have to explain in which case do use it and in which case we don't.

robUx4 · 2021-11-28T08:43:58Z

codec_specs.md


 For each line containing the timestamp and file position data is read from the appropriate
 position in the .sub file. This data consists of a MPEG program stream which in turn
 contains SPU packets. The MPEG program stream data is discarded, and each SPU packet
 is put into one Matroska frame.

+Initialization: The .idx file is stripped of all empty lines, of all comments and of lines beginning with `alt:` or `langidx:`.
+The line beginning with `id:` **SHOULD** be transformed into the appropriate Matroska track language element
+and is discarded. The `Private Data` contains all remaining lines but the ones containing timestamps and file positions.


Should be CodecPrivate not Private Data.

JeromeMartinez force-pushed the CodecPrivate_Mandatory branch from 4e84b9f to e51e151 Compare December 27, 2020 22:44

mbunkus requested changes Dec 27, 2020

View reviewed changes

ebml_matroska.xml Outdated Show resolved Hide resolved

codec_specs.md Outdated Show resolved Hide resolved

JeromeMartinez force-pushed the CodecPrivate_Mandatory branch from e51e151 to 52698e8 Compare December 28, 2020 11:05

mbunkus requested changes Dec 28, 2020

View reviewed changes

Clarification about when CodecPrivate is mandatory

1d11067

JeromeMartinez force-pushed the CodecPrivate_Mandatory branch from 52698e8 to 1d11067 Compare December 28, 2020 22:30

robUx4 requested changes Jan 2, 2021

View reviewed changes

robUx4 added clarifications codec mapping spec_codecs Codec Matroska spec document target labels Jan 2, 2021

robUx4 requested changes Nov 28, 2021

View reviewed changes

robUx4 force-pushed the master branch from 5b4360d to 7c1fdea Compare December 29, 2022 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarification about when CodecPrivate is mandatory #446

Clarification about when CodecPrivate is mandatory #446

JeromeMartinez commented Dec 27, 2020

JeromeMartinez commented Dec 28, 2020

mbunkus left a comment

mbunkus Dec 28, 2020

mbunkus Dec 28, 2020

JeromeMartinez Dec 28, 2020

robUx4 left a comment

robUx4 Jan 2, 2021

robUx4 Oct 10, 2021

robUx4 Nov 28, 2021

robUx4 Jan 2, 2021

robUx4 Nov 28, 2021

robUx4 Nov 28, 2021

robUx4 Nov 28, 2021

robUx4 Nov 28, 2021

		For more information, see (#ssa-ass-subtitles) on SSA/ASS.

		Initialization: The `Private Data` contains the [Script Info] and [V4 Styles] sections.

Clarification about when CodecPrivate is mandatory #446

Are you sure you want to change the base?

Clarification about when CodecPrivate is mandatory #446

Conversation

JeromeMartinez commented Dec 27, 2020

JeromeMartinez commented Dec 28, 2020

mbunkus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robUx4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment