| Message ID | 20200124224833.17579-6-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avformat/matroskaenc: Check for reformatting errors | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On 1/24/2020 7:48 PM, Andreas Rheinhardt wrote: > Certain types of OBUs are stripped away before muxing into Matroska and > ISOBMFF; there are two functions to do this: One that outputs by > directly writing in an AVIOContext and one that returns a freshly > allocated buffer with the units not stripped away copied into it. > > The latter option is bad for performance, especially when the input > does already not contain any of the units intended to be stripped away > (this covers typical remuxing scenarios). Therefore this commit changes > this by avoiding allocating and copying when possible; it is possible if > the OBUs to be retained are consecutively in the input buffer (without > an OBU to be discarded between them). In this case, the caller receives > the offset as well as the length of the part of the buffer that contains > the units to be kept. This also avoids copying when e.g. the only unit > to be discarded is a temporal delimiter at the front. > > For a 22.7mb/s file with average framesize 113 kB this improved the time > for the calls to ff_av1_filter_obus_buf() when writing Matroska from > 313319 decicycles to 2368 decicycles; for another file with 1.5mb/s > (average framesize 7.3 kB) it improved from 34539 decicycles to 1922 > decicyles. For these files the only units that needed to be stripped > away were temporal unit delimiters at the front. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > 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. > > 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.
On Sun, Jan 26, 2020 at 5:28 PM James Almer <jamrial@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.
On 1/26/2020 10:45 PM, Andreas Rheinhardt wrote: > On Sun, Jan 26, 2020 at 5:28 PM James Almer <jamrial@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). Right, for some reason i thought the ff_av1_filter_obus() call from movenc was using a dynamically allocated AVIOContext, where it's simply writing directly to the output muxer's AVIOContext instead. Nevermind then, sorry for the confusion. > > >>> >>> 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. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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; }
Certain types of OBUs are stripped away before muxing into Matroska and ISOBMFF; there are two functions to do this: One that outputs by directly writing in an AVIOContext and one that returns a freshly allocated buffer with the units not stripped away copied into it. The latter option is bad for performance, especially when the input does already not contain any of the units intended to be stripped away (this covers typical remuxing scenarios). Therefore this commit changes this by avoiding allocating and copying when possible; it is possible if the OBUs to be retained are consecutively in the input buffer (without an OBU to be discarded between them). In this case, the caller receives the offset as well as the length of the part of the buffer that contains the units to be kept. This also avoids copying when e.g. the only unit to be discarded is a temporal delimiter at the front. For a 22.7mb/s file with average framesize 113 kB this improved the time for the calls to ff_av1_filter_obus_buf() when writing Matroska from 313319 decicycles to 2368 decicycles; for another file with 1.5mb/s (average framesize 7.3 kB) it improved from 34539 decicycles to 1922 decicyles. For these files the only units that needed to be stripped away were temporal unit delimiters at the front. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- 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.) libavformat/av1.c | 50 ++++++++++++++++++++++++++++++++------- libavformat/av1.h | 13 ++++++---- libavformat/matroskaenc.c | 2 +- libavformat/movenc.c | 11 +++++---- 4 files changed, 58 insertions(+), 18 deletions(-)