[FFmpeg-devel] [PATCH v13 2/9] avcodec/evc_parser: Added parser implementaion for EVC format
Anton Khirnov
anton at khirnov.net
Sun Oct 16 13:54:41 EEST 2022
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
More information about the ffmpeg-devel
mailing list