[FFmpeg-devel] [PATCH 4/5] avormat/av1dec: add low-overhead bitstream format
James Almer
jamrial at gmail.com
Fri Aug 7 06:05:16 EEST 2020
On 8/6/2020 12:34 PM, Xu, Guangxin wrote:
> Hi James,
> Comment inline.
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of James
>> Almer
>> Sent: Thursday, August 6, 2020 10:09 PM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 4/5] avormat/av1dec: add low-overhead
>> bitstream format
>>
>> On 8/6/2020 5:04 AM, Xu Guangxin wrote:
>>> It's defined in Section 5.2, used by netflix.
>>> see http://download.opencontent.netflix.com/?prefix=AV1/Chimera/
>>> ---
>>> libavformat/allformats.c | 1 +
>>> libavformat/av1dec.c | 193
>> +++++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 185 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c index
>>> fd9e46e233..8eead5619d 100644
>>> --- a/libavformat/allformats.c
>>> +++ b/libavformat/allformats.c
>>> @@ -75,6 +75,7 @@ extern AVOutputFormat ff_asf_stream_muxer; extern
>>> AVInputFormat ff_au_demuxer; extern AVOutputFormat ff_au_muxer;
>>> extern AVInputFormat ff_av1_demuxer;
>>> +extern AVInputFormat ff_av1_low_overhead_demuxer;
>>
>> How about ff_obu_demuxer instead?
> You have a good taste:).
> Yes the original name is ff_obu_demuxer, I change it to this before the I send to review.
> I will change it back. I guess you also suggest I change all low_overhead things in av1dec.c to obu. Right?
>
>>
>>> extern AVInputFormat ff_avi_demuxer; extern AVOutputFormat
>>> ff_avi_muxer; extern AVInputFormat ff_avisynth_demuxer; diff --git
>>> a/libavformat/av1dec.c b/libavformat/av1dec.c index
>>> ec66152e03..0c5d172a0f 100644
>>> --- a/libavformat/av1dec.c
>>> +++ b/libavformat/av1dec.c
>>> @@ -28,6 +28,9 @@
>>> #include "avio_internal.h"
>>> #include "internal.h"
>>>
>>> +//2 + max leb 128 size
>>> +#define MAX_HEAD_SIZE 10
>>
>> This value is also used in av1_parse.h, so you could put it there and reuse it. But
>> if you do, use a less generic name, like MAX_OBU_HEADER_SIZE.
>>
> Sure.
>
>>> +
>>> typedef struct AnnexBContext {
>>> const AVClass *class;
>>> AVBSFContext *bsf;
>>> @@ -139,9 +142,8 @@ static int annexb_probe(const AVProbeData *p)
>>> return 0;
>>> }
>>>
>>> -static int annexb_read_header(AVFormatContext *s)
>>> +static int read_header(AVFormatContext *s, AVRational framerate,
>>> +AVBSFContext **bsf, void *c)
>>
>> Since c is now a log context, rename it to logctx.
>>
> Sure.
>
>
>>> {
>>> - AnnexBContext *c = s->priv_data;
>>> const AVBitStreamFilter *filter =
>> av_bsf_get_by_name("av1_frame_merge");
>>> AVStream *st;
>>> int ret;
>>> @@ -160,25 +162,32 @@ static int annexb_read_header(AVFormatContext
>> *s)
>>> st->codecpar->codec_id = AV_CODEC_ID_AV1;
>>> st->need_parsing = AVSTREAM_PARSE_HEADERS;
>>>
>>> - st->internal->avctx->framerate = c->framerate;
>>> + st->internal->avctx->framerate = framerate;
>>> // taken from rawvideo demuxers
>>> avpriv_set_pts_info(st, 64, 1, 1200000);
>>>
>>> - ret = av_bsf_alloc(filter, &c->bsf);
>>> + ret = av_bsf_alloc(filter, bsf);
>>
>> If it requires this bsf, then it should be added as a depencency to configure.
>>
>>> if (ret < 0)
>>> return ret;
>>>
>>> - ret = avcodec_parameters_copy(c->bsf->par_in, st->codecpar);
>>> + ret = avcodec_parameters_copy((*bsf)->par_in, st->codecpar);
>>> if (ret < 0) {
>>> - av_bsf_free(&c->bsf);
>>> + av_bsf_free(bsf);
>>> return ret;
>>> }
>>>
>>> - ret = av_bsf_init(c->bsf);
>>> + ret = av_bsf_init(*bsf);
>>> if (ret < 0)
>>> - av_bsf_free(&c->bsf);
>>> + av_bsf_free(bsf);
>>>
>>> return ret;
>>> +
>>> +}
>>> +
>>> +static int annexb_read_header(AVFormatContext *s) {
>>> + AnnexBContext *c = s->priv_data;
>>> + return read_header(s, c->framerate, &c->bsf, c);
>>> }
>>>
>>> static int annexb_read_packet(AVFormatContext *s, AVPacket *pkt) @@
>>> -246,12 +255,158 @@ static int annexb_read_close(AVFormatContext *s)
>>> return 0;
>>> }
>>>
>>> -#define OFFSET(x) offsetof(AnnexBContext, x)
>>> +typedef struct LowOverheadContext {
>>> + const AVClass *class;
>>> + AVBSFContext *bsf;
>>> + AVRational framerate;
>>> + uint8_t prefetched[MAX_HEAD_SIZE];
>>> + //prefetched len
>>> + int len;
>>> +} LowOverheadContext;
>>> +
>>> +static int low_overhead_probe(const AVProbeData *p) {
>>> + AVIOContext pb;
>>> + int64_t obu_size;
>>> + int ret, type, cnt = 0, has_size_flag;
>>> +
>>> + ffio_init_context(&pb, p->buf, p->buf_size, 0,
>>> + NULL, NULL, NULL, NULL);
>>> +
>>> + // Check that the first OBU is a Temporal Delimiter.
>>> + ret = read_obu(p->buf + cnt, FFMIN(p->buf_size - cnt,
>>> + MAX_HEAD_SIZE), &obu_size, &type, &has_size_flag);
>>
>> cnt is 0 at this point, so might as well remove it.
> Accept.
>
>>
>>> + if (ret < 0 || type != AV1_OBU_TEMPORAL_DELIMITER || obu_size != 0
>> || !has_size_flag)
>>> + return 0;
>>> + cnt += ret;
>>> +
>>> + ret = read_obu(p->buf + cnt, FFMIN(p->buf_size - cnt, MAX_HEAD_SIZE),
>> &obu_size, &type, &has_size_flag);
>>> + if (ret < 0 || type != AV1_OBU_SEQUENCE_HEADER || !has_size_flag)
>>
>> Afaik, there's nothing in the spec that forbids a Metadata OBU to appear here
>> before a Sequence Header.
>>
> You are right.
>
>>> + return 0;
>>> + cnt += ret;
>>> +
>>> + ret = read_obu(p->buf + cnt, FFMIN(p->buf_size - cnt, MAX_HEAD_SIZE),
>> &obu_size, &type, &has_size_flag);
>>> + if (ret < 0 || (type != AV1_OBU_FRAME && type !=
>>> + AV1_OBU_FRAME_HEADER) || obu_size <= 0 || !has_size_flag)
>>
>> Similarly, the third OBU is not required to be a Frame or Frame Header.
>> It could be Metadata after the second one was a Sequence Header, or vice-versa.
>>
> Will change
>
>>> + return 0;
>>> + return AVPROBE_SCORE_EXTENSION + 1; }
>>> +
>>> +static int low_overhead_read_header(AVFormatContext *s) {
>>> + LowOverheadContext *c = s->priv_data;
>>> + return read_header(s, c->framerate, &c->bsf, c); }
>>> +
>>> +static int low_overhead_prefetch(AVFormatContext *s) {
>>> + LowOverheadContext *c = s->priv_data;
>>> + int ret;
>>> + int size = MAX_HEAD_SIZE - c->len;
>>> + if (size > 0 && !avio_feof(s->pb)) {
>>> + ret = avio_read(s->pb, &c->prefetched[c->len], size);
>>> + if (ret < 0)
>>> + return ret;
>>> + c->len += ret;
>>> + }
>>> + return c->len;
>>> +}
>>> +
>>> +static int low_overhead_read_data(AVFormatContext *s, AVPacket *pkt,
>>> +int size) {
>>> + int left;
>>> + LowOverheadContext *c = s->priv_data;
>>> + int ret = av_new_packet(pkt, size);
>>> + if (ret < 0) {
>>> + av_log(c, AV_LOG_ERROR, "Failed to allocate packet for obu\n");
>>> + return ret;
>>> + }
>>> + if (size <= c->len) {
>>> + memcpy(pkt->data, c->prefetched, size);
>>> +
>>> + left = c->len - size;
>>> + memcpy(c->prefetched, c->prefetched + size, left);
>>
>> This looks like it should be a memmove, as it can overlap.
> Memmove is same thing as memcpy if dest <= src
> See https://elixir.bootlin.com/linux/v4.3/source/lib/string.c#L734
https://man7.org/linux/man-pages/man3/memcpy.3.html strictly states
memory areas must not overlap. Even that implemention you linked says as
much, so we can't use memcpy() here.
A single implementation of a standard function isn't representative of
every other implementation.
>
>>
>>> + c->len = left;
>>> + } else {
>>> + memcpy(pkt->data, c->prefetched, c->len);
>>> + left = size - c->len;
>>> + ret = avio_read(s->pb, pkt->data + c->len, left);
>>> + if (ret != left) {
>>> + av_log(c, AV_LOG_ERROR, "Failed to read %d frome file\n", left);
>>> + return ret;
>>> + }
>>> + c->len = 0;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int low_overhead_get_packet(AVFormatContext *s, AVPacket *pkt)
>>> +{
>>> + int64_t obu_size;
>>> + int ret, type, has_size_flag;
>>> + LowOverheadContext *c = s->priv_data;
>>> + ret = low_overhead_prefetch(s);
>>> + if (ret < 0)
>>> + return ret;
>>> + if (ret) {
>>> + ret = read_obu(c->prefetched, c->len, &obu_size, &type,
>> &has_size_flag);
>>> + if (ret < 0 && !has_size_flag) {
>>
>> Shouldn't this be (ret < 0 || !has_size_flag)?
> Yes you are right, will change.
>
>
>>
>>> + av_log(c, AV_LOG_ERROR, "Failed to read obu\n");
>>> + return ret;
>>> + }
>>> + ret = low_overhead_read_data(s, pkt, ret);
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> +static int low_overhead_read_packet(AVFormatContext *s, AVPacket
>>> +*pkt) {
>>> + LowOverheadContext *c = s->priv_data;
>>> + int ret;
>>> +
>>> + while (1) {
>>> + ret = low_overhead_get_packet(s, pkt);
>>> + if (ret < 0)
>>> + return ret;
>>> + ret = av_bsf_send_packet(c->bsf, pkt);
>>> + if (ret < 0) {
>>> + av_log(s, AV_LOG_ERROR, "Failed to send packet to "
>>> + "av1_frame_merge filter\n");
>>> + return ret;
>>> + }
>>> + ret = av_bsf_receive_packet(c->bsf, pkt);
>>> + if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
>>> + av_log(s, AV_LOG_ERROR, "av1_frame_merge filter failed to "
>>> + "send output packet\n");
>>> +
>>> + if (ret != AVERROR(EAGAIN))
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int low_overhead_read_close(AVFormatContext *s) {
>>> + LowOverheadContext *c = s->priv_data;
>>> +
>>> + av_bsf_free(&c->bsf);
>>> + return 0;
>>> +}
>>> +
>>> #define DEC AV_OPT_FLAG_DECODING_PARAM
>>> +
>>> +#define OFFSET(x) offsetof(AnnexBContext, x)
>>> static const AVOption annexb_options[] = {
>>> { "framerate", "", OFFSET(framerate), AV_OPT_TYPE_VIDEO_RATE, {.str =
>> "25"}, 0, INT_MAX, DEC},
>>> { NULL },
>>> };
>>> +#undef OFFSET
>>> +
>>> +#define OFFSET(x) offsetof(LowOverheadContext, x) static const
>>> +AVOption low_overhead_options[] = {
>>> + { "framerate", "", OFFSET(framerate), AV_OPT_TYPE_VIDEO_RATE, {.str =
>> "25"}, 0, INT_MAX, DEC},
>>> + { NULL },
>>> +};
>>> +#undef OFFSET
>>>
>>> static const AVClass annexb_demuxer_class = {
>>> .class_name = "AV1 Annex B demuxer", @@ -272,3 +427,23 @@
>>> AVInputFormat ff_av1_demuxer = {
>>> .flags = AVFMT_GENERIC_INDEX,
>>> .priv_class = &annexb_demuxer_class,
>>> };
>>> +
>>> +static const AVClass low_overhead_demuxer_class = {
>>> + .class_name = "AV1 low overhead OBU demuxer",
>>> + .item_name = av_default_item_name,
>>> + .option = low_overhead_options,
>>> + .version = LIBAVUTIL_VERSION_INT,
>>> +};
>>> +
>>> +AVInputFormat ff_av1_low_overhead_demuxer = {
>>> + .name = "av1",
>>
>> This name is already used by the AnnexB demuxer, so change it to "obu".
> Sure.
>
>>
>>> + .long_name = NULL_IF_CONFIG_SMALL("AV1 low overhead OBU"),
>>> + .priv_data_size = sizeof(LowOverheadContext),
>>> + .read_probe = low_overhead_probe,
>>> + .read_header = low_overhead_read_header,
>>> + .read_packet = low_overhead_read_packet,
>>> + .read_close = low_overhead_read_close,
>>> + .extensions = "obu",
>>> + .flags = AVFMT_GENERIC_INDEX,
>>> + .priv_class = &low_overhead_demuxer_class,
>>> +};
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email ffmpeg-devel-request at ffmpeg.org
>> with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list