[FFmpeg-devel] [PATCH V2 2/3] avcodec: add av1 hardware acceleration decoder

[James Almer]

On 9/2/2020 12:18 PM, James Almer wrote:
> On 9/2/2020 4:31 AM, Fei Wang wrote:
>> This av1 decoder is now only used for av1 hardware acceleration
>> decoder. Consider it can be extend to a local decoder like hevc
>> or vp9 in the future, so define its name as "av1" and put it into
>> external libraries codec list.
>>
>> Signed-off-by: Fei Wang <fei.w.wang at intel.com>
>> ---
>>  Changelog              |   1 +
>>  configure              |   1 +
>>  libavcodec/Makefile    |   1 +
>>  libavcodec/allcodecs.c |   1 +
>>  libavcodec/av1dec.c    | 728 +++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/av1dec.h    |  70 ++++
>>  libavcodec/version.h   |   4 +-
>>  7 files changed, 804 insertions(+), 2 deletions(-)
>>  create mode 100644 libavcodec/av1dec.c
>>  create mode 100644 libavcodec/av1dec.h

[...]

>> +static av_cold int av1_decode_init(AVCodecContext *avctx)
>> +{
>> +    AV1DecContext *s = avctx->priv_data;
>> +    int ret = 0;
>> +
>> +    for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) {
>> +        s->ref[i].tf.f = av_frame_alloc();
>> +        if (!s->ref[i].tf.f) {
>> +            av1_decode_free(avctx);
>> +            av_log(avctx, AV_LOG_ERROR,
>> +                   "Failed to allocate reference frame buffer %d.\n", i);
>> +            return AVERROR(ENOMEM);
>> +        }
>> +    }
>> +
>> +    s->cur_frame.tf.f = av_frame_alloc();
>> +    if (!s->cur_frame.tf.f) {
>> +        av1_decode_free(avctx);
>> +        av_log(avctx, AV_LOG_ERROR,
>> +               "Failed to allocate current frame buffer.\n");
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    s->avctx = avctx;
>> +    s->pix_fmt = AV_PIX_FMT_NONE;
>> +    s->seq_ref = NULL;
>> +    s->header_ref = NULL;
>> +
>> +    ret = ff_cbs_init(&s->cbc, AV_CODEC_ID_AV1, avctx);
>> +    if (ret < 0)
>> +        return ret;
> 
> You should parse avctx->extradata with ff_cbs_read(), if any, and
> initialize avctx with values taken from the filled AV1RawSequenceHeader.
> Profile, level, color info, timing info, the like.

For that matter, the same should be done in the loop in
av1_decode_frame() for all AV1_OBU_SEQUENCE_HEADER instances, since
extradata is not guaranteed to always be present.
So just add a function that takes an AV1RawSequenceHeader argument to
initialize avctx fields, and call it from here and av1_decode_frame() as
needed.

[...]

>> +            }
>> +            free_tile_data(s);
>> +            raw_tile_group = NULL;
>> +        }
>> +    }
> 
> You're duplicating a bit of code in the loop above. You could merge it
> even more. Frame header and Frame are both the same up to the point the
> Tile Group bits would start for the latter.
> 
> You could do something like:
> 
> /*****/
> switch (unit->type) {
> case AV1_OBU_REDUNDANT_FRAME_HEADER:
>     if (s->cur_frame_header)

This was meant to be s->raw_frame_header, here and below.

>         break; // In the middle of a frame, so ignore it
> // fall-through
> case AV1_OBU_FRAME:
> case AV1_OBU_FRAME_HEADER:
>     /* s->cur_frame_header_buf set here */
> 
>     if (unit->type == AV1_OBU_FRAME)
>         s->cur_frame_header = &obu->obu.frame.header;
>     else
>         s->cur_frame_header = &obu->obu.frame_header;
> 
>     /* Frame header handling code here */
> 
>     if (unit->type != AV1_OBU_FRAME)
>         break; // Keep going only for Frame OBUs
> // fall-through
> case AV1_OBU_TILE_GROUP:
>     if (!s->cur_frame_header) { // Sanity check
>         ret = AVERROR_INVALIDDATA;
>         goto end;
>     }
>     if (unit->type == AV1_OBU_FRAME)
>         raw_tile_group = &obu->obu.frame.tile_group;
>     else
>         raw_tile_group = &obu->obu.tile_group;
> 
>     /* Tile group handling code here */
> 
>     break;
> }
> 
> if (raw_tile_group && (s->tile_num == raw_tile_group->tg_end + 1)) {
>     /* Finish frame, output it if show_frame == 1 */
> 
>     s->cur_frame_header = NULL;
>     raw_tile_group = NULL;
> }
> /*****/
>