On 1/26/2020 10:45 PM, Andreas Rheinhardt wrote:
> On Sun, Jan 26, 2020 at 5:28 PM James Almer <jamr...@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-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
