[2/2] matroskaenc: don't write private data for AAC streams

Message ID 20161030190822.7612-2-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Oct. 30, 2016, 7:08 p.m.
The spec says it should be void.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroskaenc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Hendrik Leppkes Oct. 30, 2016, 7:20 p.m. | #1
On Sun, Oct 30, 2016 at 8:08 PM, James Almer <jamrial@gmail.com> wrote:
> The spec says it should be void.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/matroskaenc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index aa9d381..c3a9702 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -584,6 +584,8 @@ static int mkv_write_native_codecprivate(AVFormatContext *s,
>              avio_write(dyn_cp, par->extradata + 12,
>                         par->extradata_size - 12);
>          break;
> +    case AV_CODEC_ID_AAC:
> +        break;
>      default:
>          if (par->extradata_size)
>          avio_write(dyn_cp, par->extradata, par->extradata_size);
> --
> 2.10.1

Thats the old spec, the new spec wants the 2-5 byte configuration
record with plain A_AAC (no weird sub-entries for profiles etc)

- Hendrik
James Almer Oct. 30, 2016, 7:45 p.m. | #2
On 10/30/2016 4:20 PM, Hendrik Leppkes wrote:
> On Sun, Oct 30, 2016 at 8:08 PM, James Almer <jamrial@gmail.com> wrote:
>> The spec says it should be void.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/matroskaenc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index aa9d381..c3a9702 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -584,6 +584,8 @@ static int mkv_write_native_codecprivate(AVFormatContext *s,
>>              avio_write(dyn_cp, par->extradata + 12,
>>                         par->extradata_size - 12);
>>          break;
>> +    case AV_CODEC_ID_AAC:
>> +        break;
>>      default:
>>          if (par->extradata_size)
>>          avio_write(dyn_cp, par->extradata, par->extradata_size);
>> --
>> 2.10.1
> 
> Thats the old spec, the new spec wants the 2-5 byte configuration
> record with plain A_AAC (no weird sub-entries for profiles etc)

Where does it says that? I checked https://matroska.org/technical/specs/codecid/index.html
and https://github.com/Matroska-Org/matroska-specification/blob/gh-pages/codec_specs.md
and for every possible AAC CodecID it says "The private data is void" in both.

Neither mention anything about plain A_AAC id, for that matter.

> 
> - Hendrik
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>
Hendrik Leppkes Oct. 30, 2016, 7:48 p.m. | #3
On Sun, Oct 30, 2016 at 8:45 PM, James Almer <jamrial@gmail.com> wrote:
> On 10/30/2016 4:20 PM, Hendrik Leppkes wrote:
>> On Sun, Oct 30, 2016 at 8:08 PM, James Almer <jamrial@gmail.com> wrote:
>>> The spec says it should be void.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavformat/matroskaenc.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index aa9d381..c3a9702 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -584,6 +584,8 @@ static int mkv_write_native_codecprivate(AVFormatContext *s,
>>>              avio_write(dyn_cp, par->extradata + 12,
>>>                         par->extradata_size - 12);
>>>          break;
>>> +    case AV_CODEC_ID_AAC:
>>> +        break;
>>>      default:
>>>          if (par->extradata_size)
>>>          avio_write(dyn_cp, par->extradata, par->extradata_size);
>>> --
>>> 2.10.1
>>
>> Thats the old spec, the new spec wants the 2-5 byte configuration
>> record with plain A_AAC (no weird sub-entries for profiles etc)
>
> Where does it says that? I checked https://matroska.org/technical/specs/codecid/index.html
> and https://github.com/Matroska-Org/matroska-specification/blob/gh-pages/codec_specs.md
> and for every possible AAC CodecID it says "The private data is void" in both.
>
> Neither mention anything about plain A_AAC id, for that matter.
>

Its listed in here:
http://haali.su/mkv/codecs.pdf

Codec IDs in matroska are the formats biggest mess right now, there is
no proper central registry, and the spec website links to the PDF as
"an additional resource".

- Hendrik
James Almer Oct. 30, 2016, 7:50 p.m. | #4
On 10/30/2016 4:48 PM, Hendrik Leppkes wrote:
> On Sun, Oct 30, 2016 at 8:45 PM, James Almer <jamrial@gmail.com> wrote:
>> On 10/30/2016 4:20 PM, Hendrik Leppkes wrote:
>>> On Sun, Oct 30, 2016 at 8:08 PM, James Almer <jamrial@gmail.com> wrote:
>>>> The spec says it should be void.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  libavformat/matroskaenc.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>> index aa9d381..c3a9702 100644
>>>> --- a/libavformat/matroskaenc.c
>>>> +++ b/libavformat/matroskaenc.c
>>>> @@ -584,6 +584,8 @@ static int mkv_write_native_codecprivate(AVFormatContext *s,
>>>>              avio_write(dyn_cp, par->extradata + 12,
>>>>                         par->extradata_size - 12);
>>>>          break;
>>>> +    case AV_CODEC_ID_AAC:
>>>> +        break;
>>>>      default:
>>>>          if (par->extradata_size)
>>>>          avio_write(dyn_cp, par->extradata, par->extradata_size);
>>>> --
>>>> 2.10.1
>>>
>>> Thats the old spec, the new spec wants the 2-5 byte configuration
>>> record with plain A_AAC (no weird sub-entries for profiles etc)
>>
>> Where does it says that? I checked https://matroska.org/technical/specs/codecid/index.html
>> and https://github.com/Matroska-Org/matroska-specification/blob/gh-pages/codec_specs.md
>> and for every possible AAC CodecID it says "The private data is void" in both.
>>
>> Neither mention anything about plain A_AAC id, for that matter.
>>
> 
> Its listed in here:
> http://haali.su/mkv/codecs.pdf
> 
> Codec IDs in matroska are the formats biggest mess right now, there is
> no proper central registry, and the spec website links to the PDF as
> "an additional resource".

Lovely. Anyway, consider this patch dropped then.

Thanks.
Dave Rice Oct. 31, 2016, 2:02 p.m. | #5
Hi all,

> On Oct 30, 2016, at 3:48 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> 
> On Sun, Oct 30, 2016 at 8:45 PM, James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> wrote:
>> On 10/30/2016 4:20 PM, Hendrik Leppkes wrote:
>>> On Sun, Oct 30, 2016 at 8:08 PM, James Almer <jamrial@gmail.com> wrote:
>>>> The spec says it should be void.
>>>> 
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> libavformat/matroskaenc.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>> 
>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>> index aa9d381..c3a9702 100644
>>>> --- a/libavformat/matroskaenc.c
>>>> +++ b/libavformat/matroskaenc.c
>>>> @@ -584,6 +584,8 @@ static int mkv_write_native_codecprivate(AVFormatContext *s,
>>>>             avio_write(dyn_cp, par->extradata + 12,
>>>>                        par->extradata_size - 12);
>>>>         break;
>>>> +    case AV_CODEC_ID_AAC:
>>>> +        break;
>>>>     default:
>>>>         if (par->extradata_size)
>>>>         avio_write(dyn_cp, par->extradata, par->extradata_size);
>>>> --
>>>> 2.10.1
>>> 
>>> Thats the old spec, the new spec wants the 2-5 byte configuration
>>> record with plain A_AAC (no weird sub-entries for profiles etc)
>> 
>> Where does it says that? I checked https://matroska.org/technical/specs/codecid/index.html
>> and https://github.com/Matroska-Org/matroska-specification/blob/gh-pages/codec_specs.md
>> and for every possible AAC CodecID it says "The private data is void" in both.
>> 
>> Neither mention anything about plain A_AAC id, for that matter.
>> 
> 
> Its listed in here:
> http://haali.su/mkv/codecs.pdf <http://haali.su/mkv/codecs.pdf>
> 
> Codec IDs in matroska are the formats biggest mess right now, there is
> no proper central registry, and the spec website links to the PDF as
> "an additional resource".

Agreed. Improving the specifications on the integration of codecs in Matroska is something we've started in the CELLAR working group but need to put more time into. Our initial conversation about this is at https://mailarchive.ietf.org/arch/search/?email_list=cellar&gbt=1&q=review+of+codec_specs.md+for+defining+use+of+encodings+in+Matroska. I think first we should document how an encoding in Matroska is described, identified, and registered and then re-write the definitions for existing encoding supports with attention to maintain reverse compatibility. Observations, comments, and suggestions are welcome, we're working on it at https://datatracker.ietf.org/wg/cellar/charter/.
Best Regards,
Dave Rice

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index aa9d381..c3a9702 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -584,6 +584,8 @@  static int mkv_write_native_codecprivate(AVFormatContext *s,
             avio_write(dyn_cp, par->extradata + 12,
                        par->extradata_size - 12);
         break;
+    case AV_CODEC_ID_AAC:
+        break;
     default:
         if (par->extradata_size)
         avio_write(dyn_cp, par->extradata, par->extradata_size);