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

Michael Niedermayer michaelni
Sat Feb 26 13:24:53 CET 2011


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. 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);

     avio_write(pb, "ATR\x00", 4);
@@ -279,10 +280,12 @@ static int gxf_write_material_data_section(AVFormatContext *s)
         filename++;
     else
         filename = s->filename;
+    len = strlen(filename);
+
     avio_w8(pb, MAT_NAME);
-    avio_w8(pb, strlen(SERVER_PATH) + strlen(filename) + 1);
-    put_tag(pb, SERVER_PATH);
-    put_tag(pb, filename);
+    avio_w8(pb, strlen(SERVER_PATH) + len + 1);
+    avio_write(pb, SERVER_PATH, sizeof(SERVER_PATH) - 1);
+    avio_write(pb, filename, len);
     avio_w8(pb, 0);

     /* first field */
@@ -55,13 +56,13 @@ static int aiff_write_header(AVFormatContext *s)
             return -1;
         }
         /* Version chunk */
-        put_tag(pb, "FVER");
+        ffio_wfourcc(pb, "FVER");
         avio_wb32(pb, 4);
         avio_wb32(pb, 0xA2805140);
     }

     /* Common chunk */
-    put_tag(pb, "COMM");
+    ffio_wfourcc(pb, "COMM");
     avio_wb32(pb, aifc ? 24 : 18); /* size */
     avio_wb16(pb, enc->channels);  /* Number of channels */


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110226/5774c2ed/attachment.pgp>



More information about the ffmpeg-devel mailing list