[FFmpeg-devel] [PATCH] movenc: convert put_tag() into ffio_wfourcc().

[Baptiste Coudurier]

On 2/26/11 4:24 AM, Michael Niedermayer wrote:
> On Sat, Feb 26, 2011 at 06:42:55AM -0500, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Sat, Feb 26, 2011 at 5:03 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>> On Fri, Feb 25, 2011 at 04:10:26PM -0800, Baptiste Coudurier wrote:
>>>> Hi,
>>>>
>>>> On 02/25/2011 02:36 PM, Ronald S. Bultje wrote:
>>>>> ---
>>>>>   libavformat/movenc.c |    6 +++---
>>>>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>> index 93d6ce9..57a6d11 100644
>>>>> --- a/libavformat/movenc.c
>>>>> +++ b/libavformat/movenc.c
>>>>> @@ -1241,16 +1241,16 @@ static int mov_write_tapt_tag(ByteIOContext *pb, MOVTrack *track)
>>>>>       int64_t pos = url_ftell(pb);
>>>>>
>>>>>       avio_wb32(pb, 0); /* size */
>>>>> -    put_tag(pb, "tapt");
>>>>> +    ffio_wfourcc(pb, "tapt");
>>>>>
>>>>>       avio_wb32(pb, 20);
>>>>> -    put_tag(pb, "clef");
>>>>> +    ffio_wfourcc(pb, "clef");
>>>>>       avio_wb32(pb, 0);
>>>>>       avio_wb32(pb, width<<  16);
>>>>>       avio_wb32(pb, track->enc->height<<  16);
>>>>>
>>>>>       avio_wb32(pb, 20);
>>>>> -    put_tag(pb, "enof");
>>>>> +    ffio_wfourcc(pb, "enof");
>>>>>       avio_wb32(pb, 0);
>>>>>       avio_wb32(pb, track->enc->width<<  16);
>>>>>       avio_wb32(pb, track->enc->height<<  16);
>>>>
>>>> This name is extremely ugly.
>>>
>>> yes, the API is also worse
>>>
>>> put_tag() worked with things that had length different from 4
>>> sane thing would have been to make it ffio_put_tag() with put_tag() API
>>> I tried to participate in the renamings
>>
>> I'd like to see the part where you participated in the renaming of
>> put_tag(), I don't remember any such an occurance. You participated in
>> the renaming of another symbol, I think ByteIOContext -> AVIOContext.
>> Several people prefered AVIOContext, you wanted AVBIOContext, there
>> was a majority for AVIOContext so we went with that.
> 
> There are 2 IO APIs
> 
> previously
> ByteIOContext with url_fopen() and URLContext with url_open()
> 
> I suggested
> AVBIOContext avbio_open() and AVUIOContext with avuio_open()
> 
> which would be consistent, these could have been named
> AVHIOContext avhio_open() and AVLIOContext with avlio_open()
> for High/Low level IO too
> 
> we didnt come that far as you and  mans forced your choice
> through while i asked for this to be discussed on the ML.
> 
> to quote you guys:
> [13:20:55] <michaelni> BBB: on the ML yes, here is unfair
> [13:21:20] <BBB> takes too long
> [13:21:24] <BBB> elenril: your patch, you choose
> [13:21:29] <mru> just f*cking do it
> [13:21:30] <BBB> easiest solution
> 
> and now you have
> AVIOContext avio_open() and URLContext url_open()
> The right side still needs to be renamed and theres no good& consistent name
> left
> 
> about put_tag, try to read the whole mail you reply to, and try not to clip
> part of the message away
> "
>  Anyone remembers being asked? a vote?
>  no, well, thats why i gave up.
> "
> 
> That should clarify it i hope, but i can reword it, i gave up after seeing the
> total incompetence in how AVIOContext was handled. I value my time more than
> to throw it at a bunch of people who ask if they should vote and then dont
> vote when asked to do as they prefer to force their choice through anyway.
> 
> 
>>
>> Re put_tag, I opposed the presence of the function because it doesn't
>> do anything useful that avio_write() itself doesn't do.

Oh, one more nice dictatorship manifestation here.

>> The part where
>> I see it being used is as a convenient way to write fourccs, where
>> performance could be optimized because the amount of bytes is always
>> 4. We therefore made it a macro that calls avio_wl32() using MKTAG().
>> Once we implement AV_WL32() in that function, I/O performance may
>> actually improve a little. Other than that, avio_write() does the same
>> as put_tag(), so we use that now. If you opposed this concept, I
>> emailed this to the list several days before I applied that patch, and
>> nobody replied to it.
> 
> To show 3 actual hunks of the "improvment":
> 
> @@ -79,7 +79,7 @@ static int mmf_write_header(AVFormatContext *s)
>      avio_w8(pb, 0); /* code type */
>      avio_w8(pb, 0); /* status */
>      avio_w8(pb, 0); /* counts */
> -    put_tag(pb, "VN:libavcodec,"); /* metadata ("ST:songtitle,VN:version,...") */
> +    avio_write(pb, "VN:libavcodec,", sizeof("VN:libavcodec,") -1); /* metadata ("ST:songtitle,VN:version,...") */
>      end_tag_be(pb, pos);
> 

OMFG, are you guys CRAZY or is it an attempt to actually make FFmpeg
more difficult to work on ?

Please restore put_tag, and clean up this embarrassing mess.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org