[FFmpeg-devel] [PATCH] lavf/adtsenc: Add support for APE tags

[James Almer]

On 12/07/13 8:27 AM, Paul B Mahol wrote:
> On 7/12/13, Thierry Foucu <tfoucu at gmail.com> wrote:
>> On Thu, Jul 11, 2013 at 1:16 PM, Paul B Mahol <onemda at gmail.com> wrote:
>>
>>> On 7/11/13, Thierry Foucu <tfoucu at gmail.com> wrote:
>>>> On Thu, Jul 11, 2013 at 11:08 AM, James Almer <jamrial at gmail.com>
>>>> wrote:
>>>>
>>>>> Should fix ticket #2269
>>>>>
>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>> ---
>>>>>  libavformat/adtsenc.c | 9 +++++++++
>>>>>  1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/libavformat/adtsenc.c b/libavformat/adtsenc.c
>>>>> index 60d7b07..7118105 100644
>>>>> --- a/libavformat/adtsenc.c
>>>>> +++ b/libavformat/adtsenc.c
>>>>> @@ -25,6 +25,7 @@
>>>>>  #include "libavcodec/avcodec.h"
>>>>>  #include "libavcodec/mpeg4audio.h"
>>>>>  #include "avformat.h"
>>>>> +#include "apetag.h"
>>>>>
>>>>>  #define ADTS_HEADER_SIZE 7
>>>>>
>>>>> @@ -162,6 +163,13 @@ static int adts_write_packet(AVFormatContext *s,
>>>>> AVPacket *pkt)
>>>>>      return 0;
>>>>>  }
>>>>>
>>>>> +static int adts_write_trailer(AVFormatContext *s)
>>>>> +{
>>>>> +    ff_ape_write_tag(s);
>>>>>
>>>>
>>>> I think this should be an option of the adts muxer as not all the adts
>>>> decoder will know what to do with this and some will crash.
>>>> and if some people use libav as a library and want to create adts chunk
>>>> (HLS) they may be calling trailer at the end of each chunk and this
>>>> will
>>> be
>>>> adding the ape metadata.
>>>>
>>>> What do you all think?
>>>
>>> You sure this code is used for HLS?
>>>
>>
>> in HLS, audio only could be ADTS audio. And apple is using the size of the
>> chunk and the duration of it to compute its bitrate. So, we should not have
>> extra metadata for each chunk.
> 
> Yes, but: is this code used by HLS? and metadata is not set after each
> chunk, just at
> end.

Ping.

What version is preferred in the end? Forced, or enabled with an AVOption? 
If the latter, I'd prefer the AVOption to be "write_apetag" instead of "apetag" 
as i originally named it in alternative patch i submitted. That way it will be 
consistent with similar AVOptions from other muxers.