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

[Andreas Rheinhardt]

Andreas Rheinhardt:
> 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
> 
Applied.

- Andreas