On 8/12/2020 9:53 PM, Xu, Guangxin wrote:
>
> Hi James,
> Thanks for the review.
> Comment in line.
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of James
>> Almer
>> Sent: Wednesday, August 12, 2020 9:06 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH V2 2/3] avormat/av1dec: add low-
>> overhead bitstream format
>>
>> On 8/10/2020 6:34 AM, Xu Guangxin wrote:
>>> It's defined in Section 5.2, used by netflix.
>>> see http://download.opencontent.netflix.com/?prefix=AV1/Chimera/
>>> ---
>>> configure | 1 +
>>> libavformat/allformats.c | 1 +
>>> libavformat/av1dec.c | 266 +++++++++++++++++++++++++++++++++++--
>> --
>>> 3 files changed, 245 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 8de1afcb99..d4a1fea9ce 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -3331,6 +3331,7 @@ mxf_d10_muxer_select="mxf_muxer"
>>> mxf_opatom_muxer_select="mxf_muxer"
>>> nut_muxer_select="riffenc"
>>> nuv_demuxer_select="riffdec"
>>> +obu_demuxer_select="av1_frame_merge_bsf av1_parser"
>>> oga_muxer_select="ogg_muxer"
>>> ogg_demuxer_select="dirac_parse"
>>> ogv_muxer_select="ogg_muxer"
>>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c index
>>> b7e59ae170..0aa9dd7198 100644
>>> --- a/libavformat/allformats.c
>>> +++ b/libavformat/allformats.c
>>> @@ -293,6 +293,7 @@ extern AVOutputFormat ff_null_muxer; extern
>>> AVInputFormat ff_nut_demuxer; extern AVOutputFormat ff_nut_muxer;
>>> extern AVInputFormat ff_nuv_demuxer;
>>> +extern AVInputFormat ff_obu_demuxer;
>>> extern AVOutputFormat ff_oga_muxer;
>>> extern AVInputFormat ff_ogg_demuxer; extern AVOutputFormat
>>> ff_ogg_muxer; diff --git a/libavformat/av1dec.c b/libavformat/av1dec.c
>>> index 1be2fac1c1..14efb8f1d4 100644
>>> --- a/libavformat/av1dec.c
>>> +++ b/libavformat/av1dec.c
>>> @@ -70,6 +70,24 @@ static int read_obu(const uint8_t *buf, int size, int64_t
>> *obu_size, int *type)
>>> return 0;
>>> }
>>>
>>> +//return < 0 if we need more data
>>> +static int get_score(int type, int *seq) {
>>> + switch (type) {
>>> + case AV1_OBU_SEQUENCE_HEADER:
>>> + *seq = 1;
>>> + return -1;
>>> + case AV1_OBU_FRAME:
>>> + case AV1_OBU_FRAME_HEADER:
>>> + return *seq ? AVPROBE_SCORE_EXTENSION + 1 : 0;
>>> + case AV1_OBU_METADATA:
>>> + return -1;
>>> + default:
>>> + break;
>>
>> Padding OBU should be ignored/skipped, same as Metadata. This function
>> should make the caller abort only for OBUs that are not meant to appear
>> before
>> a Frame/Frame Header (Tile list, Tile Group, Redundant Frame Header, another
>> Temporal Delimiter).
> Sure, I will add padding to allow list.
>
>>
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> static int annexb_probe(const AVProbeData *p) {
>>> AVIOContext pb;
>>> @@ -123,19 +141,9 @@ static int annexb_probe(const AVProbeData *p)
>>> return 0;
>>> cnt += obu_unit_size;
>>>
>>> - switch (type) {
>>> - case AV1_OBU_SEQUENCE_HEADER:
>>> - seq = 1;
>>> - break;
>>> - case AV1_OBU_FRAME:
>>> - case AV1_OBU_FRAME_HEADER:
>>> - return seq ? AVPROBE_SCORE_EXTENSION + 1 : 0;
>>> - case AV1_OBU_TILE_GROUP:
>>> - case AV1_OBU_TEMPORAL_DELIMITER:
>>> - return 0;
>>> - default:
>>> - break;
>>> - }
>>> + ret = get_score(type, &seq);
>>> + if (ret >= 0)
>>> + return ret;
>>>
>>> temporal_unit_size -= obu_unit_size + ret;
>>> frame_unit_size -= obu_unit_size + ret; @@ -144,15 +152,14 @@
>>> 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 *logctx)
>>
>> Nit: Maybe a pointer to framerate instead.
> I will change to
> const AVRational* framerate
>
>>
>>> {
>>> - AnnexBContext *c = s->priv_data;
>>> const AVBitStreamFilter *filter =
>> av_bsf_get_by_name("av1_frame_merge");
>>> AVStream *st;
>>> int ret;
>>>
>>> if (!filter) {
>>> - av_log(c, AV_LOG_ERROR, "av1_frame_merge bitstream filter "
>>> + av_log(logctx, AV_LOG_ERROR, "av1_frame_merge bitstream filter "
>>> "not found. This is a bug, please report it.\n");
>>> return AVERROR_BUG;
>>> }
>>> @@ -165,25 +172,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 (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) @@
>>> -251,12 +265,198 @@ static int annexb_read_close(AVFormatContext *s)
>>> return 0;
>>> }
>>>
>>> -#define OFFSET(x) offsetof(AnnexBContext, x)
>>> +typedef struct ObuContext {
>>> + const AVClass *class;
>>> + AVBSFContext *bsf;
>>> + AVRational framerate;
>>> + uint8_t prefetched[MAX_OBU_HEADER_SIZE];
>>> + //prefetched len
>>> + int len;
>>> +} ObuContext;
>>> +
>>> +//For low overhead obu, we can't foresee the obu size before we parsed the
>> header.
>>> +//So, we can't use parse_obu_header here, since it will check size <=
>>> +buf_size //see c27c7b49dc for more details static int
>>> +read_obu_with_size(const uint8_t *buf, int buf_size, int64_t
>>> +*obu_size, int *type) {
>>> + GetBitContext gb;
>>> + int ret, extension_flag, start_pos;
>>> + int64_t size;
>>> +
>>> + ret = init_get_bits8(&gb, buf, FFMIN(buf_size, MAX_OBU_HEADER_SIZE));
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (get_bits1(&gb) != 0) // obu_forbidden_bit
>>> + return AVERROR_INVALIDDATA;
>>> +
>>> + *type = get_bits(&gb, 4);
>>> + extension_flag = get_bits1(&gb);
>>> + if (!get_bits1(&gb)) // has_size_flag
>>> + return AVERROR_INVALIDDATA;
>>> + skip_bits1(&gb); // obu_reserved_1bit
>>> +
>>> + if (extension_flag) {
>>> + get_bits(&gb, 3); // temporal_id
>>> + get_bits(&gb, 2); // spatial_id
>>> + skip_bits(&gb, 3); // extension_header_reserved_3bits
>>> + }
>>> +
>>> + *obu_size = leb128(&gb);
>>> + if (*obu_size > INT_MAX)
>>> + return AVERROR_INVALIDDATA;
>>> +
>>> + if (get_bits_left(&gb) < 0)
>>> + return AVERROR_INVALIDDATA;
>>> +
>>> + start_pos = get_bits_count(&gb) / 8;
>>> +
>>> + size = *obu_size + start_pos;
>>> + if (size > INT_MAX)
>>> + return AVERROR_INVALIDDATA;
>>> + return size;
>>> +}
>>> +
>>> +static int obu_probe(const AVProbeData *p) {
>>> + int64_t obu_size;
>>> + int seq = 0;
>>> + int ret, type, cnt;
>>> +
>>> + // Check that the first OBU is a Temporal Delimiter.
>>> + cnt = read_obu_with_size(p->buf, p->buf_size, &obu_size, &type);
>>> + if (cnt < 0 || type != AV1_OBU_TEMPORAL_DELIMITER || obu_size != 0)
>>> + return 0;
>>> +
>>> + while (1) {
>>> + ret = read_obu_with_size(p->buf + cnt, p->buf_size - cnt,
>>> + &obu_size, &type);
>>
>> You need to ensure p->buf_size is bigger than cnt to avoid overreads when you
> I think about this before. It may not needed.
> If cnt>p->buf_size, init_get_bits_xe will report an error for init_get_bits8.
> See
> https://github.com/FFmpeg/FFmpeg/blob/c455a28a9e99d41d070be887228aa8609543b9a8/libavcodec/get_bits.h#L631
Yeah, did not consider the fact that init_get_bits() will fail on < 0
size arguments.
>
>> attempt to skip to the next OBU, so use read_obu() here since you're passing
>> the
>> entire buffer length and not just up to 10 bytes like in obu_get_packet()
>> below.
> If we use read_obu, it will skip the check for has_size_flag. We may got a
> false positive.
>
>> The size <= buf_size check is needed here.
> If size>buf_size happens before AV1_OBU_FRAME/ AV1_OBU_FRAME_HEADER, we do
> not need check it. the function will return 0 since we can't find the frame
> obu.
> If it happens in frame obu, we only have part of frame in probe size. it's
> not big problem too, right?
>
>>
>>> + if (ret < 0 || obu_size <= 0)
>>> + return 0;
>>> + cnt += ret;
>>> +
>>> + ret = get_score(type, &seq);
>>> + if (ret >= 0)
>>> + return ret;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int obu_read_header(AVFormatContext *s) {
>>> + ObuContext *c = s->priv_data;
>>> + return read_header(s, c->framerate, &c->bsf, c); }
>>> +
>>> +static int obu_prefetch(AVFormatContext *s) {
>>> + ObuContext *c = s->priv_data;
>>> + int ret;
>>> + int size = MAX_OBU_HEADER_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 obu_read_data(AVFormatContext *s, AVPacket *pkt, int size)
>>> +{
>>> + int left;
>>> + ObuContext *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;
>>> + memmove(c->prefetched, c->prefetched + size, left);
>>
>> You can probably use av_fifo_* functions for the prefetch buffer. See
>> libavutil/fifo.h
> Could you explain more about this?
> It may save 1 or 2 lines of code here, but it's not benefit total code size
> or performance.
It may save a few lines of code, it communicates the intent more clearly
to anyone reading the code, and avoids reimplementing a FIFO in this
demuxer when there's a public API available specifically for that purpose.
You can even pass a custom function to av_fifo_generic_write() to read
the data directly from s->pb.
>
>>
>>> + 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 obu_get_packet(AVFormatContext *s, AVPacket *pkt) {
>>> + int64_t obu_size;
>>> + int ret, type;
>>> + ObuContext *c = s->priv_data;
>>> + ret = obu_prefetch(s);
>>> + if (ret < 0)
>>> + return ret;
>>> + if (ret) {
>>> + ret = read_obu_with_size(c->prefetched, c->len, &obu_size, &type);
>>> + if (ret < 0) {
>>> + av_log(c, AV_LOG_ERROR, "Failed to read obu\n");
>>> + return ret;
>>> + }
>>> + ret = obu_read_data(s, pkt, ret);
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> +static int obu_read_packet(AVFormatContext *s, AVPacket *pkt) {
>>> + ObuContext *c = s->priv_data;
>>> + int ret;
>>> +
>>> + while (1) {
>>> + ret = obu_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 obu_read_close(AVFormatContext *s) {
>>> + ObuContext *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(ObuContext, x) static const AVOption
>>> +obu_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", @@ -277,3 +477,23 @@
>>> AVInputFormat ff_av1_demuxer = {
>>> .flags = AVFMT_GENERIC_INDEX,
>>> .priv_class = &annexb_demuxer_class,
>>> };
>>> +
>>> +static const AVClass obu_demuxer_class = {
>>> + .class_name = "AV1 low overhead OBU demuxer",
>>> + .item_name = av_default_item_name,
>>> + .option = obu_options,
>>> + .version = LIBAVUTIL_VERSION_INT,
>>> +};
>>> +
>>> +AVInputFormat ff_obu_demuxer = {
>>> + .name = "obu",
>>> + .long_name = NULL_IF_CONFIG_SMALL("AV1 low overhead OBU"),
>>> + .priv_data_size = sizeof(ObuContext),
>>> + .read_probe = obu_probe,
>>> + .read_header = obu_read_header,
>>> + .read_packet = obu_read_packet,
>>> + .read_close = obu_read_close,
>>> + .extensions = "obu",
>>> + .flags = AVFMT_GENERIC_INDEX,
>>> + .priv_class = &obu_demuxer_class,
>>> +};
>>>
>>
>> _______________________________________________
>> 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".
>
_______________________________________________
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".
