[FFmpeg-devel] [PATCH] avformat/oggenc: Avoid allocations and copying when writing page data

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Fri May 8 17:29:21 EEST 2020


Andreas Rheinhardt:
> When the Ogg muxer writes a page, it has to do three things: It needs to
> write a page header, then it has to actually copy the page data and then
> it has to calculate and write a CRC checksum of both header as well as
> data at a certain position in the page header.
> 
> To do this, the muxer used a dynamic buffer for both writing as well as
> calculating the checksum via an AVIOContext's feature to automatically
> calculate checksums on the data it writes. This entails an allocation of
> an AVIOContext, of the opaque specific to dynamic buffers and of the
> buffer itself (which may be reallocated multiple times) as well as
> memcopying the data (first into the AVIOContext's small write buffer,
> then into the dynamic buffer's big buffer).
> 
> This commit changes this: The page header is no longer written into a
> dynamic buffer any more; instead the (small) page header is written into
> a small buffer on the stack. The CRC is then calculated directly via
> av_crc() on both the page header as well as the page data. Then both the
> page header and the page data are written.
> 
> Finally, ogg_write_page() can now no longer fail, so it has been
> modified to return nothing; this also fixed a bug in the only caller of
> this function: It didn't check the return value.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavformat/oggenc.c | 63 ++++++++++++++++----------------------------
>  1 file changed, 22 insertions(+), 41 deletions(-)
> 
> diff --git a/libavformat/oggenc.c b/libavformat/oggenc.c
> index fbd14fedf9..f81907117e 100644
> --- a/libavformat/oggenc.c
> +++ b/libavformat/oggenc.c
> @@ -29,7 +29,6 @@
>  #include "libavcodec/bytestream.h"
>  #include "libavcodec/flac.h"
>  #include "avformat.h"
> -#include "avio_internal.h"
>  #include "internal.h"
>  #include "vorbiscomment.h"
>  
> @@ -99,50 +98,32 @@ static const AVClass flavor ## _muxer_class = {\
>      .version    = LIBAVUTIL_VERSION_INT,\
>  };
>  
> -static void ogg_update_checksum(AVFormatContext *s, AVIOContext *pb, int64_t crc_offset)
> -{
> -    int64_t pos = avio_tell(pb);
> -    uint32_t checksum = ffio_get_checksum(pb);
> -    avio_seek(pb, crc_offset, SEEK_SET);
> -    avio_wb32(pb, checksum);
> -    avio_seek(pb, pos, SEEK_SET);
> -}
> -
> -static int ogg_write_page(AVFormatContext *s, OGGPage *page, int extra_flags)
> +static void ogg_write_page(AVFormatContext *s, OGGPage *page, int extra_flags)
>  {
>      OGGStreamContext *oggstream = s->streams[page->stream_index]->priv_data;
> -    AVIOContext *pb;
> -    int64_t crc_offset;
> -    int ret, size;
> -    uint8_t *buf;
> -
> -    ret = avio_open_dyn_buf(&pb);
> -    if (ret < 0)
> -        return ret;
> -    ffio_init_checksum(pb, ff_crc04C11DB7_update, 0);
> -    ffio_wfourcc(pb, "OggS");
> -    avio_w8(pb, 0);
> -    avio_w8(pb, page->flags | extra_flags);
> -    avio_wl64(pb, page->granule);
> -    avio_wl32(pb, oggstream->serial_num);
> -    avio_wl32(pb, oggstream->page_counter++);
> -    crc_offset = avio_tell(pb);
> -    avio_wl32(pb, 0); // crc
> -    avio_w8(pb, page->segments_count);
> -    avio_write(pb, page->segments, page->segments_count);
> -    avio_write(pb, page->data, page->size);
> -
> -    ogg_update_checksum(s, pb, crc_offset);
> -
> -    size = avio_close_dyn_buf(pb, &buf);
> -    if (size < 0)
> -        return size;
> -
> -    avio_write(s->pb, buf, size);
> +    uint8_t buf[4 + 1 + 1 + 8 + 4 + 4 + 4 + 1 + 255], *ptr = buf, *crc_pos;
> +    const AVCRC *crc_table = av_crc_get_table(AV_CRC_32_IEEE);
> +    uint32_t crc;
> +
> +    bytestream_put_le32(&ptr, MKTAG('O', 'g', 'g', 'S'));
> +    bytestream_put_byte(&ptr, 0);
> +    bytestream_put_byte(&ptr, page->flags | extra_flags);
> +    bytestream_put_le64(&ptr, page->granule);
> +    bytestream_put_le32(&ptr, oggstream->serial_num);
> +    bytestream_put_le32(&ptr, oggstream->page_counter++);
> +    crc_pos = ptr;
> +    bytestream_put_le32(&ptr, 0);
> +    bytestream_put_byte(&ptr, page->segments_count);
> +    bytestream_put_buffer(&ptr, page->segments, page->segments_count);
> +
> +    crc = av_crc(crc_table, 0, buf, ptr - buf);
> +    crc = av_crc(crc_table, crc, page->data, page->size);
> +    bytestream_put_be32(&crc_pos, crc);
> +
> +    avio_write(s->pb, buf, ptr - buf);
> +    avio_write(s->pb, page->data, page->size);
>      avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT);
> -    av_free(buf);
>      oggstream->page_count--;
> -    return 0;
>  }
>  
>  static int ogg_key_granule(OGGStreamContext *oggstream, int64_t granule)
> 
Will apply this tomorrow unless there are objections.

- Andreas


More information about the ffmpeg-devel mailing list