Re: [FFmpeg-devel] [PATCH v13 2/9] avcodec/evc_parser: Added parser implementaion for EVC format

[Anton Khirnov <anton@khirnov.net>]

Quoting Dawid Kozinski (2022-10-07 11:11:13)
> +
> +static int get_nalu_type(const uint8_t *bits, int bits_size, AVCodecContext *avctx)

You seem to be doing custom bitreading here and in
read_nal_unit_length(). You should use either the get_bits.h API for
bitreading or bytestream2 API for byte reading.

Also, avctx is unused (same in read_nal_unit_length()).

> +{
> +    int unit_type_plus1 = 0;
> +
> +    if (bits_size >= EVC_NAL_HEADER_SIZE) {
> +        unsigned char *p = (unsigned char *)bits;
> +        // forbidden_zero_bit
> +        if ((p[0] & 0x80) != 0)
> +            return -1;
> +
> +        // nal_unit_type
> +        unit_type_plus1 = (p[0] >> 1) & 0x3F;
> +    }
> +
> +    return unit_type_plus1 - 1;
> +}
> +
> +static uint32_t read_nal_unit_length(const uint8_t *bits, int bits_size, AVCodecContext *avctx)
> +{
> +    uint32_t nalu_len = 0;
> +
> +    if (bits_size >= EVC_NAL_UNIT_LENGTH_BYTE) {
> +
> +        int t = 0;
> +        unsigned char *p = (unsigned char *)bits;
> +
> +        for (int i = 0; i < EVC_NAL_UNIT_LENGTH_BYTE; i++)
> +            t = (t << 8) | p[i];
> +
> +        nalu_len = t;
> +        if (nalu_len == 0)
> +            return 0;
> +    }
> +
> +    return nalu_len;
> +}

this whole function looks very much like AV_RB32 or
bytestream2_get_be32.

> +static int parse_nal_units(AVCodecParserContext *s, const uint8_t *bs,
> +                           int bs_size, AVCodecContext *avctx)
> +{
> +    EVCParserContext *ev = s->priv_data;
> +    int nalu_type, nalu_size;
> +    unsigned char *bits = (unsigned char *)bs;
> +    int bits_size = bs_size;

First, casting away const is something you should almost never do.
Especially in a parser, which should never modify the input bitstream.

> +    avctx->codec_id = AV_CODEC_ID_EVC;

This seems unnecessary.

> +    s->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
> +    s->key_frame = -1;
> +
> +    nalu_size = read_nal_unit_length(bits, bits_size, avctx);
> +    if (nalu_size == 0) {

IIUC read_nal_unit_length() can return -1, which should be handled here.

> +        av_log(avctx, AV_LOG_ERROR, "Invalid NAL unit size: (%d)\n", nalu_size);
> +        return -1;
> +    }
> +
> +    bits += EVC_NAL_UNIT_LENGTH_BYTE;
> +    bits_size -= EVC_NAL_UNIT_LENGTH_BYTE;
> +
> +    nalu_type = get_nalu_type(bits, bits_size, avctx);

Invalid type should be handled here.
> +
> +    bits += EVC_NAL_HEADER_SIZE;
> +    bits_size -= EVC_NAL_HEADER_SIZE;
> +
> +    if (nalu_type == EVC_SPS_NUT) { // NAL Unit type: SPS (Sequence Parameter Set)

useless comment, the check is obvious

> +        EVCParserSPS *sps;
> +
> +        sps = parse_sps(bits, bits_size, ev);
> +        if (!sps) {
> +            av_log(avctx, AV_LOG_ERROR, "SPS parsing error\n");
> +            return -1;
> +        }
> +
> +        s->coded_width         = sps->pic_width_in_luma_samples;
> +        s->coded_height        = sps->pic_height_in_luma_samples;
> +        s->width               = sps->pic_width_in_luma_samples;
> +        s->height              = sps->pic_height_in_luma_samples;
> +
> +        if (sps->profile_idc == 1) avctx->profile = FF_PROFILE_EVC_MAIN;
> +        else avctx->profile = FF_PROFILE_EVC_BASELINE;
> +
> +        // Currently XEVD decoder supports ony YCBCR420_10LE chroma format for EVC stream

The parser is standalone, limitations of some specific decoder
implementation should not affect parsing.

> +        switch (sps->chroma_format_idc) {
> +        case 0: /* YCBCR400_10LE */
> +            av_log(avctx, AV_LOG_ERROR, "YCBCR400_10LE: Not supported chroma format\n");
> +            s->format = AV_PIX_FMT_GRAY10LE;
> +            return -1;
> +        case 1: /* YCBCR420_10LE */
> +            s->format = AV_PIX_FMT_YUV420P10LE;
> +            break;
> +        case 2: /* YCBCR422_10LE */
> +            av_log(avctx, AV_LOG_ERROR, "YCBCR422_10LE: Not supported chroma format\n");
> +            s->format = AV_PIX_FMT_YUV422P10LE;
> +            return -1;
> +        case 3: /* YCBCR444_10LE */
> +            av_log(avctx, AV_LOG_ERROR, "YCBCR444_10LE: Not supported chroma format\n");
> +            s->format = AV_PIX_FMT_YUV444P10LE;
> +            return -1;
> +        default:
> +            s->format = AV_PIX_FMT_NONE;
> +            av_log(avctx, AV_LOG_ERROR, "Unknown supported chroma format\n");
> +            return -1;
> +        }
> +
> +        // @note
> +        // The current implementation of parse_sps function doesn't handle VUI parameters parsing.
> +        // If it will be needed, parse_sps function could be extended to handle VUI parameters parsing
> +        // to initialize fields of the AVCodecContex i.e. color_primaries, color_trc,color_range
> +
> +    } else if (nalu_type == EVC_PPS_NUT) { // NAL Unit type: PPS (Video Parameter Set)
> +        EVCParserPPS *pps;
> +
> +        pps = parse_pps(bits, bits_size, ev);
> +        if (!pps) {
> +            av_log(avctx, AV_LOG_ERROR, "PPS parsing error\n");
> +            return -1;
> +        }

You're not doing anything with the PPS, why waste time parsing it at
all?

> +    } else if (nalu_type == EVC_SEI_NUT) // NAL unit type: SEI (Supplemental Enhancement Information)
> +        return 0;
> +    else if (nalu_type == EVC_IDR_NUT || nalu_type == EVC_NOIDR_NUT) { // NAL Unit type: Coded slice of a IDR or non-IDR picture
> +        EVCParserSliceHeader *sh;
> +
> +        sh = parse_slice_header(bits, bits_size, ev);
> +        if (!sh) {
> +            av_log(avctx, AV_LOG_ERROR, "Slice header parsing error\n");
> +            return -1;
> +        }
> +        switch (sh->slice_type) {
> +        case EVC_SLICE_TYPE_B: {
> +            s->pict_type =  AV_PICTURE_TYPE_B;
> +            break;
> +        }
> +        case EVC_SLICE_TYPE_P: {
> +            s->pict_type =  AV_PICTURE_TYPE_P;
> +            break;
> +        }
> +        case EVC_SLICE_TYPE_I: {
> +            s->pict_type =  AV_PICTURE_TYPE_I;
> +            break;
> +        }
> +        default: {
> +            s->pict_type =  AV_PICTURE_TYPE_NONE;
> +        }
> +        }
> +        s->key_frame = (nalu_type == EVC_IDR_NUT) ? 1 : 0;
> +    } else {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid NAL unit type: %d\n", nalu_type);
> +        return -1;

The message is false - the unit type is unhandled, not necessarily
invalid. And I see no reason why the parser should fail on unknown unit
types.

> +static int evc_parser_init(AVCodecParserContext *s)
> +{
> +    EVCParserContext *ev = s->priv_data;
> +
> +    ev->nal_length_size = EVC_NAL_UNIT_LENGTH_BYTE;

This seems to be write-only.

> +    ev->incomplete_nalu_prefix_read = 0;
> +
> +    return 0;
> +}
> +
> +static int evc_parse(AVCodecParserContext *s, AVCodecContext *avctx,
> +                     const uint8_t **poutbuf, int *poutbuf_size,
> +                     const uint8_t *buf, int buf_size)
> +{
> +    int next;
> +    EVCParserContext *ev = s->priv_data;
> +    ParseContext *pc = &ev->pc;
> +    int is_dummy_buf = !buf_size;
> +    const uint8_t *dummy_buf = buf;
> +
> +    if (s->flags & PARSER_FLAG_COMPLETE_FRAMES)
> +        next = buf_size;
> +    else {
> +        next = evc_find_frame_end(s, buf, buf_size, avctx);
> +        if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
> +            *poutbuf      = NULL;
> +            *poutbuf_size = 0;
> +            ev->to_read -= buf_size;
> +            return buf_size;
> +        }
> +    }
> +
> +    is_dummy_buf &= (dummy_buf == buf);
> +
> +    if (!is_dummy_buf)
> +        parse_nal_units(s, buf, buf_size, avctx);

parse_nal_units() should return meaningful error codes (like
AVERROR_INVALIDDATA) and they should be handled here.

> +
> +    // poutbuf contains just one NAL unit

The parser should not return individual NAL units, but complete frames
(access units in HEVC terminology, don't know if EVC defines something
similar).

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".