[FFmpeg-devel] [PATCH v2] avcodec: add an AV1 parser

[Derek Buitenhuis]

Hi,

Apologies if you've covered any of these comments before.

On 03/10/2018 02:44, James Almer wrote:
> Simple parser to set keyframes, frame type, structure, width, height, and pixel
> format, plus stream profile and level.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> Missing Changelog entry and version bump still.

[...]

> +    if (avctx->extradata_size && !s->parsed_extradata) {
> +        s->parsed_extradata = 1;

Shouldn't this be set at the bottom of this block (since parsing can fail)?


> +        }
> +
> +        avctx->profile = seq->seq_profile;
> +        avctx->level   = seq->seq_level_idx[0];
> +
> +        switch (frame_type) {
> +        case AV1_FRAME_KEY:
> +        case AV1_FRAME_INTRA_ONLY:
> +            ctx->pict_type = AV_PICTURE_TYPE_I;
> +            break;

Not strictly related to this patch, and not a blocker, but is it not
about time the API gains the ability to mark frames with more clarity
than just "I"? H.264, HEVC, etc. also have this issue (e.g. I vs IDR,
and HEVC's many times of RAPs). AV_PICTURE_TYPE_SI is kinda related,
I guess, but not exactly what I mean.

> +        case AV1_FRAME_INTER:
> +            ctx->pict_type = AV_PICTURE_TYPE_P;
> +            break;
> +        case AV1_FRAME_SWITCH:
> +            ctx->pict_type = AV_PICTURE_TYPE_SP;
> +            break;
> +        }

I assume we're not going to mark alt-refs in any special way since
they're combined in one packet with a visible frame?

> +        ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;

Any reason we just don't always set this at the top of the function?

> +        switch (av1->bit_depth) {
> +        case 8:
> +            ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY8
> +                                             : pix_fmts_8bit [color->subsampling_x][color->subsampling_y];
> +            break;
> +        case 10:
> +            ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY10
> +                                             : pix_fmts_10bit[color->subsampling_x][color->subsampling_y];
> +            break;
> +        case 12:
> +            ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY12
> +                                             : pix_fmts_12bit[color->subsampling_x][color->subsampling_y];
> +            break;
> +        }

Won't this break horribly on e.g. 4:4:0?

> +        av_assert2(ctx->format != AV_PIX_FMT_NONE);

I assume ctx->bit_depth will always be set to something valid.

- Derek