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

[James Almer]

On 10/3/2018 9:53 AM, Derek Buitenhuis wrote:
> 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)?

If extradata parsing fails and we bail out without setting this first,
no packet will ever be parsed since this runs first thing every time.

A better API would allow us to check it during init(), like in the
decoder and bitstream filter ones, but that's not the case here.

> 
> 
>> +        }
>> +
>> +        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?

Exactly. They are handled in the following packets, when the visible
frame is a show_existing_frame one pointing to them.

> 
>> +        ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
> 
> Any reason we just don't always set this at the top of the function?

Because the parsing can fail, and i figured it was nicer to keep it as
the default AV_PICTURE_STRUCTURE_UNKNOWN in those cases.

> 
>> +        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?

Fortunately, AV1 got rid of 4:4:0 :p

> 
>> +        av_assert2(ctx->format != AV_PIX_FMT_NONE);
> 
> I assume ctx->bit_depth will always be set to something valid.

If you look at how i set ctx->format, it depends on the values of
color->subsampling_x and color->subsampling_y. If for whatever reason
cbs_av1 wrongly sets the former as 0 and the latter as 1 (which is
invalid), the lookup table will return AV_PIX_FMT_NONE. This assert is
to make sure that doesn't happen, as it'd be an internal bug.

I can remove it if you prefer, but by being av_assert2 it will not run
outside of builds purposely enabling level 2 asserts.

> 
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>