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

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@gmail.com>
---
 libavformat/oggenc.c | 63 ++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

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

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