[FFmpeg-devel] [PATCH v24 1/9] avcodec/evc_parser: Added parser implementation for EVC format

[James Almer]

On 6/12/2023 9:28 AM, Dawid Kozinski wrote:
> +int ff_evc_parse_nal_units(EVCParserContext *ctx, const uint8_t *buf, int buf_size, void *logctx)
> +{
> +    const uint8_t *data = buf;
> +    int data_size = buf_size;
> +    int bytes_read = 0;
> +    int nalu_size = 0;
> +
> +    while (data_size > 0) {
> +
> +        // Buffer size is not enough for buffer to store NAL unit 4-bytes prefix (length)
> +        if (data_size < EVC_NALU_LENGTH_PREFIX_SIZE)
> +            return END_NOT_FOUND;

END_NOT_FOUND is a return value from the packet merging functionality of 
AVParser, which you're not using anymore. Given the parser now expects 
complete NALUs, this should be AVERROR_INVALIDDATA.

Also, ff_evc_parse_nal_units() seems to be used only by this parser, so 
why is it shared? It's ff_evc_parse_nal_unit() that is also used by the 
merge bsf.

> +
> +        nalu_size = ff_evc_read_nal_unit_length(data, data_size, logctx);
> +
> +        bytes_read += EVC_NALU_LENGTH_PREFIX_SIZE;
> +
> +        data += EVC_NALU_LENGTH_PREFIX_SIZE;
> +        data_size -= EVC_NALU_LENGTH_PREFIX_SIZE;
> +
> +        if (data_size < nalu_size)
> +            return END_NOT_FOUND;

Same.

> +
> +        if (ff_evc_parse_nal_unit(ctx, data, nalu_size, logctx) != 0) {
> +            av_log(logctx, AV_LOG_ERROR, "Parsing of NAL unit failed\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        data += nalu_size;
> +        data_size -= nalu_size;
> +    }
> +    return 0;
> +}

[...]

> +/**
> + * Parse NAL units of found picture and decode some basic information.
> + *
> + * @param s parser context.
> + * @param avctx codec context.
> + * @param buf buffer with field/frame data.
> + * @param buf_size size of the buffer.
> + */
> +static int parse_nal_units(AVCodecParserContext *s, AVCodecContext *avctx, const uint8_t *buf, int buf_size)
> +{
> +    EVCParserContext *p = s->priv_data;
> +
> +    int res = ff_evc_parse_nal_units(p, buf, buf_size, avctx);
> +    if(!res)
> +        return res;

ff_evc_parse_nal_units() returns 0 on success, so the check should be 
ret < 0. Otherwise all the code below will never be used.

> +
> +    s->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
> +
> +    if(p->nalu_type == EVC_SPS_NUT) {

Wont p->nalu_type here be the value of the last NAL parsed in the AU? If 
so, I'd expect it to be a picture and not a sequence header, so this 
code will rarely if ever be covered.

Since ff_evc_parse_nal_units() doesn't need to be shared, it can reside 
locally here and you can pass it the AVCodecParserContext pointer, 
setting all these values as soon as they are found while parsing the 
respective NALUs.

> +
> +        s->coded_width         = p->coded_width;
> +        s->coded_height        = p->coded_height;
> +        s->width               = p->width;
> +        s->height              = p->height;
> +
> +        s->format              = p->format;
> +
> +        avctx->coded_width     = s->coded_width;
> +        avctx->coded_height    = s->coded_height;
> +        avctx->width           = s->width;
> +        avctx->height          = s->height;

Remove these four. They are set generically by the AVParser code.

> +
> +        avctx->gop_size        = p->gop_size;
> +        avctx->delay           = p->delay;
> +        avctx->profile         = p->profile;
> +
> +        avctx->framerate.den   = p->framerate.den;
> +        avctx->framerate.num   = p->framerate.num;
> +
> +    } else if(p->nalu_type == EVC_NOIDR_NUT) {
> +        s->pict_type = p->pict_type;
> +        s->key_frame = p->key_frame;
> +        s->output_picture_number = p->output_picture_number;

Why export these only for non IDR pictures?

> +    }
> +
> +    return 0;
> +}