[FFmpeg-devel] [PATCH 12/12] avformat/av1: Avoid allocation + copying when filtering OBUs
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Mon Jan 27 03:45:07 EET 2020
On Sun, Jan 26, 2020 at 5:28 PM James Almer <jamrial at gmail.com> wrote:
> On 1/24/2020 7:48 PM, Andreas Rheinhardt wrote:
> > If it is desired, I can add a commit to switch ff_mov_write_packet() to
> > not use a pointer just for reformatted_data (that is of course
> > initialized to NULL), but to replace it by a data buffer that gets
> > initialized to pkt->data and modified so that data + offset always
> > points to the current data. (This is possible now that the av_freep()
> > have been removed from the reformatting functions.)
>
> Yes, this would be ideal if the speed gains above can also be done for
> movenc.
>
I think you missed the point of the comment above: It is not about
performance. Unless movenc uses a hint track for the av1 stream, it did not
need to copy the data to a freshly allocated buffer and so it did not have
to suffer the performance penalty for it. Ergo there is nothing to be
gained for it. And if a hint track is used, it already benefits from this
patch as-is (and as it has been applied).
> >
> > libavformat/av1.c | 50 ++++++++++++++++++++++++++++++++-------
> > libavformat/av1.h | 13 ++++++----
> > libavformat/matroskaenc.c | 2 +-
> > libavformat/movenc.c | 11 +++++----
> > 4 files changed, 58 insertions(+), 18 deletions(-)
> >
> > diff --git a/libavformat/av1.c b/libavformat/av1.c
> > index 07b399efcc..fef8e96f8d 100644
> > --- a/libavformat/av1.c
> > +++ b/libavformat/av1.c
> > @@ -29,13 +29,20 @@
> > #include "avio.h"
> > #include "avio_internal.h"
> >
> > -int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size)
> > +static int av1_filter_obus(AVIOContext *pb, const uint8_t *buf,
> > + int size, int *offset)
> > {
> > - const uint8_t *end = buf + size;
> > + const uint8_t *start = buf, *end = buf + size;
> > int64_t obu_size;
> > - int start_pos, type, temporal_id, spatial_id;
> > -
> > - size = 0;
> > + int off, start_pos, type, temporal_id, spatial_id;
> > + enum {
> > + START_NOT_FOUND,
> > + START_FOUND,
> > + END_FOUND,
> > + OFFSET_IMPOSSIBLE,
> > + } state = START_NOT_FOUND;
> > +
> > + off = size = 0;
> > while (buf < end) {
> > int len = parse_obu_header(buf, end - buf, &obu_size,
> &start_pos,
> > &type, &temporal_id, &spatial_id);
> > @@ -47,8 +54,16 @@ int ff_av1_filter_obus(AVIOContext *pb, const uint8_t
> *buf, int size)
> > case AV1_OBU_REDUNDANT_FRAME_HEADER:
> > case AV1_OBU_TILE_LIST:
> > case AV1_OBU_PADDING:
> > + if (state == START_FOUND)
> > + state = END_FOUND;
> > break;
> > default:
> > + if (state == START_NOT_FOUND) {
> > + off = buf - start;
> > + state = START_FOUND;
> > + } else if (state == END_FOUND) {
> > + state = OFFSET_IMPOSSIBLE;
> > + }
> > if (pb)
> > avio_write(pb, buf, len);
> > size += len;
> > @@ -57,19 +72,35 @@ int ff_av1_filter_obus(AVIOContext *pb, const
> uint8_t *buf, int size)
> > buf += len;
> > }
> >
> > + if (offset)
> > + *offset = state != OFFSET_IMPOSSIBLE ? off : -1;
> > +
> > return size;
> > }
> >
> > -int ff_av1_filter_obus_buf(const uint8_t *in, uint8_t **out, int *size)
> > +int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size)
> > +{
> > + return av1_filter_obus(pb, buf, size, NULL);
> > +}
> > +
> > +int ff_av1_filter_obus_buf(const uint8_t *in, uint8_t **out,
> > + int *size, int *offset)
> > {
> > AVIOContext pb;
> > uint8_t *buf;
> > - int len, ret;
> > + int len, off, ret;
> >
> > - len = ret = ff_av1_filter_obus(NULL, in, *size);
> > + len = ret = av1_filter_obus(NULL, in, *size, &off);
> > if (ret < 0) {
> > return ret;
> > }
> > + if (off >= 0) {
> > + *out = (uint8_t *)in;
> > + *size = len;
> > + *offset = off;
> > +
> > + return 0;
> > + }
> >
> > buf = av_malloc((size_t)len + AV_INPUT_BUFFER_PADDING_SIZE);
> > if (!buf)
> > @@ -77,13 +108,14 @@ int ff_av1_filter_obus_buf(const uint8_t *in,
> uint8_t **out, int *size)
> >
> > ffio_init_context(&pb, buf, len, 1, NULL, NULL, NULL, NULL);
> >
> > - ret = ff_av1_filter_obus(&pb, in, *size);
> > + ret = av1_filter_obus(&pb, in, *size, NULL);
> > av_assert1(ret == len);
> >
> > memset(buf + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> >
> > *out = buf;
> > *size = len;
> > + *offset = 0;
> >
> > return 0;
> > }
> > diff --git a/libavformat/av1.h b/libavformat/av1.h
> > index 6cc3fcaad2..dd5b47dc25 100644
> > --- a/libavformat/av1.h
> > +++ b/libavformat/av1.h
> > @@ -56,19 +56,24 @@ typedef struct AV1SequenceParameters {
> > int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size);
> >
> > /**
> > - * Filter out AV1 OBUs not meant to be present in ISOBMFF sample data
> and write
> > - * the resulting bitstream to a newly allocated data buffer.
> > + * Filter out AV1 OBUs not meant to be present in ISOBMFF sample data
> and return
> > + * the result in a data buffer, avoiding allocations and copies if
> possible.
> > *
> > * @param in input data buffer
> > - * @param out pointer to pointer that will hold the allocated data
> buffer
> > + * @param out pointer to pointer for the returned buffer. In case of
> success,
> > + * it is independently allocated if and only if `*out`
> differs from in.
> > * @param size size of the input data buffer. The size of the resulting
> output
> > * data buffer will be written here
> > + * @param offset offset of the returned data inside `*out`: It runs from
> > + * `*out + offset` (inclusive) to `*out + offset + size`
> > + * (exclusive); is zero if `*out` is independently
> allocated.
> > *
> > * @return 0 in case of success, a negative AVERROR code in case of
> failure.
> > * On failure, *out and *size are unchanged
> > * @note *out will be treated as unintialized on input and will not be
> freed.
> > */
> > -int ff_av1_filter_obus_buf(const uint8_t *in, uint8_t **out, int *size);
> > +int ff_av1_filter_obus_buf(const uint8_t *in, uint8_t **out,
> > + int *size, int *offset);
> >
> > /**
> > * Parses a Sequence Header from the the provided buffer.
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index 20bad95262..618f07c769 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -2112,7 +2112,7 @@ static int mkv_write_block(AVFormatContext *s,
> AVIOContext *pb,
> > /* extradata is Annex B, assume the bitstream is too and
> convert it */
> > err = ff_hevc_annexb2mp4_buf(pkt->data, &data, &size, 0, NULL);
> > } else if (par->codec_id == AV_CODEC_ID_AV1) {
> > - err = ff_av1_filter_obus_buf(pkt->data, &data, &size);
> > + err = ff_av1_filter_obus_buf(pkt->data, &data, &size, &offset);
> > } else if (par->codec_id == AV_CODEC_ID_WAVPACK) {
> > err = mkv_strip_wavpack(pkt->data, &data, &size);
> > } else
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index b5e06de3d5..f715f911f3 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -5374,7 +5374,7 @@ int ff_mov_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> > AVCodecParameters *par = trk->par;
> > AVProducerReferenceTime *prft;
> > unsigned int samples_in_chunk = 0;
> > - int size = pkt->size, ret = 0;
> > + int size = pkt->size, ret = 0, offset = 0;
> > int prft_size;
> > uint8_t *reformatted_data = NULL;
> >
> > @@ -5491,7 +5491,8 @@ int ff_mov_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> > }
> > } else if (par->codec_id == AV_CODEC_ID_AV1) {
> > if (trk->hint_track >= 0 && trk->hint_track < mov->nb_streams) {
> > - ret = ff_av1_filter_obus_buf(pkt->data, &reformatted_data,
> &size);
> > + ret = ff_av1_filter_obus_buf(pkt->data, &reformatted_data,
> > + &size, &offset);
> > if (ret < 0)
> > return ret;
> > avio_write(pb, reformatted_data, size);
> > @@ -5667,12 +5668,14 @@ int ff_mov_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> >
> > if (trk->hint_track >= 0 && trk->hint_track < mov->nb_streams)
> > ff_mov_add_hinted_packet(s, pkt, trk->hint_track, trk->entry,
> > - reformatted_data, size);
> > + reformatted_data ? reformatted_data +
> offset
> > + : NULL, size);
> >
> > end:
> > err:
> >
> > - av_free(reformatted_data);
> > + if (pkt->data != reformatted_data)
> > + av_free(reformatted_data);
> > return ret;
> > }
>
> Seems to work, so pushed alongside a new test to mux AV1 in Matroska
> that tests the offset path.
>
> I'll add another test with a sample using Padding OBUs in frames to test
> the path allocating a new buffer.
Good. I have tested it with redundant frame headers in the middle of
packets (you were right that libaom can create such files; thanks for the
suggestion) as well as temporal delimiters (of course) and padding at the
end (your sample).
Thanks for your efforts.
- Andreas
PS: I'll update my patch
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255149.html after
you have added the second new test.
More information about the ffmpeg-devel
mailing list