[FFmpeg-devel] [PATCH] lavf: Make id3v1_create_tag a lavf internal function

[James Almer]

On 13/07/13 5:23 PM, Paul B Mahol wrote:
> On 7/13/13, James Almer <jamrial at gmail.com> wrote:
>> On 13/07/13 4:29 PM, Paul B Mahol wrote:
>>> On 7/13/13, James Almer <jamrial at gmail.com> wrote:
>>>> There are more containers than just mp3 with support for id3v1,
>>>> and we currently have muxer for at least one of them
>>>>
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>>  libavformat/Makefile   |  3 ++-
>>>>  libavformat/id3v1.h    |  6 +++++
>>>>  libavformat/id3v1enc.c | 73
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  libavformat/mp3enc.c   | 52 ++++-------------------------------
>>>>  4 files changed, 86 insertions(+), 48 deletions(-)
>>>>  create mode 100644 libavformat/id3v1enc.c
>>>>
>>>
>>> What other muxer(s) are going to use this?
>>
>> ADTS is one. I was planning to add an avoption for it after the APE tag one
>> is
>> committed, or i can send an updated patch adding both at the same time
>> instead.
> 
> I see no reason to support both matadata tags, pick best one.
> 
>>
>> Then there's apparently tta, musepack and wavpack that also support it.
> 
> There is no musepack muxer for good reason.
> 
> Wavpack muxer have ape tags and its enough.
> 
> I really see no reason to support id3v1 for cases where better option exist.
> 
> So I really do not see valid reason why this should be commited.

I personally thought having the option was a good idea. I wouldn't be surprised if some 
players can't read Ape tags but can read id3v1 instead.
To me, having this code exclusive for the mp3 muxer when it could easily be shared and 
used by other muxers seemed like a waste.

About adts, if we have to choose one then the patch i sent with Ape tags is the best 
option.
I'm ok with dropping this patch if it's considered superfluous.

Regards.