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

Ronald S. Bultje rsbultje
Sat Feb 19 16:09:09 CET 2011


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

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

[..]
> @@ -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.

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

> @@ -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.

> ? ? ? ? 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_*()?

> 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... ?

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

> @@ -241,8 +241,8 @@ static void put_xiph_size(ByteIOContext *pb, int size)

That's a weird name for a function in matroskaenc, especially since
it's static, but disregard this comment for now, just one of those
"what the hell, that's weird" moments.

Note to self: these types of patches are hard-impossible to review
completely. What I think is a good way forward is to not worry too
much about most comments except the comment-indenting ones, fix it,
commit, and move forward from there. This patch improves stuff, it's
not complete, but I don't want to review this 10x. Others agree?
Anton, you OK with that? I do still hope you'll address the comments
(at least where relevant for the public API, I don't care much about
the private API stuff).

Ronald



More information about the ffmpeg-devel mailing list