Hi,
Anthon, thank you for your review.
I've just submitted to the patchwork a new patchset (v14
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=7794) containing
changes following your review. If something still needs to be changed
please let me know.
I will be grateful for your feedback.
Best regards
Dawid
-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Anton
Khirnov
Sent: niedziela, 16 października 2022 12:55
To: d.frankie...@samsung.com; FFmpeg development discussions and patches
<ffmpeg-devel@ffmpeg.org>
Cc: Dawid Kozinski <d.kozin...@samsung.com>
Subject: Re: [FFmpeg-devel] [PATCH v13 2/9] avcodec/evc_parser: Added parser
implementaion for EVC format
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.
[REPLY]
I only read 2 bytes of the header here to get NALU type. The function is
small and clear. That's the reason I decided not to use any ffmpeg API here.
If I had to parse something big i.e VUI would definitely use the GetBits
API.
However, If you still insist I will change it ofcourse.
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.
[DONE] Yes, you are absolutely right. That's a severe flaw. I've just fixed
it.
> + avctx->codec_id = AV_CODEC_ID_EVC;
This seems unnecessary.
[DONE] I've removed it (It has been fixed in patchset v14)
> + 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.
[DONE] It has been fixed in patchset v14
> + 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.
[DONE] It has been fixed in patchset v14
> +
> + 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
[DONE] It has been fixed in patchset v14 (useless comment has been removed)
> + 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.
[DONE] It has been fixed in patchset v14 (Fixed)
> + 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?
[REPLY] The information from parsed PPS is used by parse_slice_header()
function
> + } 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.
[DONE] It has been fixed in patchset v14
> +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.
[DONE] It has been fixed in patchset v14
> +
> + // 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).
[REPLY] Current EVC decoder implementation needs individual NAL units.
--
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-requ...@ffmpeg.org
with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
