[FFmpeg-devel] [PATCH 01/10] avio: add avio_ prefix to put_* functions

Anton Khirnov anton
Sat Feb 19 19:37:53 CET 2011


On Sat, Feb 19, 2011 at 10:09:09AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> this is a bitch to review. :-).
> 
> On Sat, Feb 19, 2011 at 6:42 AM, Anton Khirnov <anton at khirnov.net> wrote:
> > diff --git a/libavformat/adtsenc.c b/libavformat/adtsenc.c
> [..]
> > @@ -125,13 +125,13 @@ static int adts_write_packet(AVFormatContext *s, AVPacket *pkt)
> [..]
> > - ? ?put_buffer(pb, pkt->data, pkt->size);
> > + ? ?avio_put_buffer(pb, pkt->data, pkt->size);
> > ? ? put_flush_packet(pb);
> 
> Please don't forget this put_*() function also. If we rename them
> systematically, they should all be renamed. (We discussed this on IRC,
> fine in a separate patch if you want to rename it, just don't forget.)

Sure, will be done.

> 
> > diff --git a/libavformat/aiffenc.c b/libavformat/aiffenc.c
> [..]
> > @@ -43,10 +43,10 @@ static int aiff_write_header(AVFormatContext *s)
> > - ? ?put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* file length */
> > - ? ?put_tag(pb, aifc ? "AIFC" : "AIFF");
> > + ? ?avio_put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* file length */
> > + ? ?avio_put_tag(pb, aifc ? "AIFC" : "AIFF");
> 
> This comment is misaligned both before and after, please fix.
> 

Not very sure with _what_ it's supposed to be aligned, but hopefully
fixed.
> [..]
> > @@ -77,22 +77,22 @@ static int aiff_write_header(AVFormatContext *s)
> [..]
> > ? ? aiff->ssnd = url_ftell(pb); ? ? ? ? /* Sound chunk size */
> > - ? ?put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* Sound samples data size */
> > - ? ?put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* Data offset */
> > - ? ?put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* Block-size (block align) */
> > + ? ?avio_put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* Sound samples data size */
> > + ? ?avio_put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* Data offset */
> > + ? ?avio_put_be32(pb, 0); ? ? ? ? ? ? ? ? ? ?/* Block-size (block align) */
> 
> This misaligns comments.

fixed

> 
> > diff --git a/libavformat/asfenc.c b/libavformat/asfenc.c
> [..]
> > @@ -272,11 +272,11 @@ static void put_chunk(AVFormatContext *s, int type, int payload_length, int flag
> [..]
> > - ? ?put_le16(pb, type);
> > - ? ?put_le16(pb, length); ? ?//size
> > - ? ?put_le32(pb, asf->seqno);//sequence number
> > - ? ?put_le16(pb, flags); /* unknown bytes */
> > - ? ?put_le16(pb, length); ? ?//size_confirm
> > + ? ?avio_put_le16(pb, type);
> > + ? ?avio_put_le16(pb, length); ? ?//size
> > + ? ?avio_put_le32(pb, asf->seqno);//sequence number
> > + ? ?avio_put_le16(pb, flags); /* unknown bytes */
> > + ? ?avio_put_le16(pb, length); ? ?//size_confirm
> 
> Misaligned comment (although before also, so feel free to ignore this for now).
> 

fixed

> > @@ -423,19 +423,19 @@ static int asf_write_header1(AVFormatContext *s, int64_t file_size, int64_t data
> [..]
> > - ? ? ? ?put_le64(pb, 0); /* ??? */
> > + ? ? ? ?avio_put_le64(pb, 0); /* ??? */
> > ? ? ? ? es_pos = url_ftell(pb);
> 
> So here you see another nice inconsistency that you should probably
> consider fixing: the {URL,ByteIO}Context confusion thing. If you're
> gonna rename ByteIOContext to AVIOContext and have avio_get/put_*(),
> then these url_f*() functions should be renamed avio_*(), not
> avurl_f*(). This is for url_ftell(), url_fseek(), and some more.
> 

Ok, will do.

> > ? ? ? ? if (enc->codec_type == AVMEDIA_TYPE_AUDIO) {
> > ? ? ? ? ? ? /* WAVEFORMATEX header */
> > ? ? ? ? ? ? int wavsize = ff_put_wav_header(pb, enc);
> 
> And what with those "internal" functions with the wrong prefix? Leave?
> Rename to ffio_*()?
> 

I'd leave them for now, we can fix them whenever if needed.

> > diff --git a/libavformat/avienc.c b/libavformat/avienc.c
> [..]
> > @@ -77,9 +77,9 @@ static int64_t avi_start_new_riff(AVFormatContext *s, ByteIOContext *pb,
> [..]
> > ? ? avi->riff_start = ff_start_tag(pb, "RIFF");
> > - ? ?put_tag(pb, riff_tag);
> > + ? ?avio_put_tag(pb, riff_tag);
> > ? ? loff = ff_start_tag(pb, "LIST");
> > - ? ?put_tag(pb, list_tag);
> > + ? ?avio_put_tag(pb, list_tag);
> 
> ff_start_tag() is another one of those internal functions that may
> still need a prefix. Same for ff_end_tag().
> 
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> [..]
> > @@ -293,9 +321,9 @@ int avio_put_str16le(ByteIOContext *s, const char *str)
> [..]
> > ? ? ? ? GET_UTF8(ch, *q++, break;)
> > - ? ? ? ?PUT_UTF16(ch, tmp, put_le16(s, tmp);ret += 2;)
> > + ? ? ? ?PUT_UTF16(ch, tmp, avio_put_le16(s, tmp);ret += 2;)
> 
> An these PUT_*() (and same for GET_*() for the get_*() avio prefix
> patch) ones are not prefixed because... ?
> 

Because they're macros from lavu ;)

> > diff --git a/libavformat/ffmetaenc.c b/libavformat/ffmetaenc.c
> [..]
> > @@ -64,15 +64,15 @@ static int write_trailer(AVFormatContext *s)
> [..]
> > - ? ? ? ?put_tag(s->pb, ID_CHAPTER);
> > - ? ? ? ?put_byte(s->pb, '\n');
> > + ? ? ? ?avio_put_tag(s->pb, ID_CHAPTER);
> > + ? ? ? ?avio_put_byte(s->pb, '\n');
> > ? ? ? ? url_fprintf(s->pb, "TIMEBASE=%d/%d\n", ch->time_base.num, ch->time_base.den);
> > ? ? ? ? url_fprintf(s->pb, "START=%"PRId64"\n", ch->start);
> > ? ? ? ? url_fprintf(s->pb, "END=%"PRId64"\n", ? ch->end);
> 
> More url_f*() functions that should get a prefix (avio_*(), not avurl_f*()).
> 
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> [..]
> > @@ -123,9 +123,9 @@ static void put_ebml_id(ByteIOContext *pb, unsigned int id)
> > ?static void put_ebml_size_unknown(ByteIOContext *pb, int bytes)
> 
> I suppose we want to leave the static put_*() functions alone for now?
> It looks weird, e.g.:
> 
> > ?static void put_ebml_float(ByteIOContext *pb, unsigned int elementid, double val)
> > ?{
> > ? ? put_ebml_id(pb, elementid);
> > ? ? put_ebml_num(pb, 8, 0);
> > - ? ?put_be64(pb, av_dbl2int(val));
> > + ? ?avio_put_be64(pb, av_dbl2int(val));
> > ?}
> 
> The old code looked nicer.
> 

OTOH it's clearly visible which are "local" functions defined in the
same file and which are global.

-- 
Anton Khirnov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avio-add-avio_-prefix-to-put_-functions.patch
Type: text/x-diff
Size: 299987 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110219/e98ec707/attachment.patch>
-------------- 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/20110219/e98ec707/attachment.pgp>



More information about the ffmpeg-devel mailing list