From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 82C3544045 for ; Mon, 24 Oct 2022 10:16:46 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9A0D768BB95; Mon, 24 Oct 2022 13:16:42 +0300 (EEST) Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3D5C868BB17 for ; Mon, 24 Oct 2022 13:16:35 +0300 (EEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20221024101634euoutp017254a03c57618df7b4e7cb41880304b1~g_d4mcmkx1838218382euoutp01a for ; Mon, 24 Oct 2022 10:16:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20221024101634euoutp017254a03c57618df7b4e7cb41880304b1~g_d4mcmkx1838218382euoutp01a DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1666606594; bh=zdwPzKteEu5YyXSIN5gmDj4HRTjMqwz+x4R9UC8VsPo=; h=From:To:In-Reply-To:Subject:Date:References:From; b=Bd+p+ixABfmutj/8SKWDByIIZsja6YJBuhOpBbZi2Jvq2cy83FwmhuUC/KQZMTKgA B7poYB3Nu6jVsrRGsTGjsaKMdhZ/Dm28y9+p5tlZiWbFAQC3ZrcMo9jNBXJPCUlG3G XJn00jkrtlNaYbPlwea4aPV9cv4jnZuNmjnFEpDw= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20221024101634eucas1p211a4b717f7bfc27f41a5ba04f6d38a79~g_d4Zv3dz1795017950eucas1p2i for ; Mon, 24 Oct 2022 10:16:34 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id 72.31.07817.20666536; Mon, 24 Oct 2022 11:16:34 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20221024101633eucas1p1a29f0a343d8162c26aff6cb60204bceb~g_d34js0w2066320663eucas1p1u for ; Mon, 24 Oct 2022 10:16:33 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20221024101633eusmtrp1355b97285b417ceaa7da799bcb427444~g_d337V981711917119eusmtrp1E for ; Mon, 24 Oct 2022 10:16:33 +0000 (GMT) X-AuditID: cbfec7f4-893ff70000011e89-8d-6356660253b4 Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 4B.5F.07473.10666536; Mon, 24 Oct 2022 11:16:33 +0100 (BST) Received: from AMDN5164 (unknown [106.210.132.171]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20221024101633eusmtip168eace0de12c20517ca2728bc8119215~g_d3mR1Tf0077000770eusmtip1q for ; Mon, 24 Oct 2022 10:16:33 +0000 (GMT) From: "Dawid Kozinski/Multimedia \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics" To: "'FFmpeg development discussions and patches'" In-Reply-To: <166591768194.12287.5804009794696497982@lain.khirnov.net> Date: Mon, 24 Oct 2022 12:16:33 +0200 Message-ID: <004e01d8e791$b12c6320$13852960$@samsung.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHVxiWn6r+6sDNZAPDEmIkW2p6M0wIHU5bpAjnOYEEByX5bTq3zLyEQ Content-Language: pl X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrJIsWRmVeSWpSXmKPExsWy7djPc7pMaWHJBiteClp8+3SG2YHR48+i zSwBjFFcNimpOZllqUX6dglcGXderGMp+NHIWHFmjkkDY1ddFyMnh4SAicTjiauZuhi5OIQE VjBK7N97ggXCmcQkceXRQUYIZyKTxPeO6+wwLYs3TGaGSCxnlLj44x2U08YksWjHHbAqNoE8 icef1zKD2CICPhLd69ezgticAm4Svxr7wGqEBbIlLi3ZzQhiswioSpzdtRLoEA4OXgFLiSvv 7EHCvAKCEidnPmEBsZkF9CSenZoFZWtLLFv4mhniIAWJn0+XsUKscpO4cuA9VI2IxI1HLWAf SAhM5JCY8XQ2G8h8CQEXiePTvSF6hSVeHd8C9ZiMxOnJPSwQJcUSh/odIMwaiUM/0iEqrCXe Nh5nhLAdJbb8v8QOUcInceOtIMRSPolJ26YzQ4R5JTrahCBMFYm+TjGIRimJp8vmME9gVJqF 5MNZSD6cheTDWUg+WcDIsopRPLW0ODc9tdgoL7Vcrzgxt7g0L10vOT93EyMwNZz+d/zLDsbl rz7qHWJk4mA8xCjBwawkwsvyJDhZiDclsbIqtSg/vqg0J7X4EKM0B4uSOC/bDK1kIYH0xJLU 7NTUgtQimCwTB6dUA1OO7pX3y49t3h7SP63/7O+CWMOlQfc9Qvkmir11nDh/5Yt7N/utBa4q qKZNv/dUX1baND3tnfcun8mJhmdKDiiczpbt8liYUhSUrl5/v8NYpmia7CnejV7HFx4PWDaX sXpHt/5uU/MDBgXTvPY0Pp/PwKSUULfWeckk5g/3jn7t2C9kurxDeZlW5Rkl5d12kRJO7huy vI9JWy/1+j5r4ZYO76Oxu4pqg6Ncl3x8nHQgRXpp/o36R6Uhx3c9XOHyMEx+ipLjtL33/0ya /Tij1lV8bfzRFOnpby9tT+TncVhwYePFc2afHzt7SC1cs9Pho82dydEf9qa3zJ2qlvuq4vDq A5d1jFiEfA9K2wR7dn9SYinOSDTUYi4qTgQA46zUzXwDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrHLMWRmVeSWpSXmKPExsVy+t/xu7qMaWHJBsv36Ft8+3SG2YHR48+i zSwBjFF6NkX5pSWpChn5xSW2StGGFkZ6hpYWekYmlnqGxuaxVkamSvp2NimpOZllqUX6dgl6 GS9fZRecDaho7jnI2sC40KqLkZNDQsBEYvGGycxdjFwcQgJLGSV+zL/ODpGQkli6dBEjhC0s 8edaFxtEUQuTxKIbC9hAEmwCORJrZ09kArFFBHwkutevZ4UomsYk8fPBHrBuTgE3iV+NfWBT hQUyJY5POwzWzCKgKnF210qgZg4OXgFLiSvv7EHCvAKCEidnPmEBsZkFDCSWLPzFBGFrSyxb +JoZ4iAFiZ9Pl7FC7HWTuHLgPVS9iMSNRy2MExiFZiEZNQvJqFlIRs1C0rKAkWUVo0hqaXFu em6xoV5xYm5xaV66XnJ+7iZGYERsO/Zz8w7Gea8+6h1iZOJgPMQowcGsJMLL8iQ4WYg3JbGy KrUoP76oNCe1+BCjKdBrE5mlRJPzgTGZVxJvaGZgamhiZmlgamlmrCTO61nQkSgkkJ5Ykpqd mlqQWgTTx8TBKdXA1Hhtwc+b0QGfPVeLPHox4+4J5vDTc0K8qs4Jro/LO2fmJWJRv0Lf+VR7 xuKlmRb2u5l+b31Xr6pZ97dlH+/+CHXX6taQzCP9kxTFAhyDiu4ePMhw6mQIr7D3s4KDjn+u BE+8urbHeLt84JbnPCGfW/5vXaxtsK44d6fpvqsuqr1MtY5+vLfSo+/bsu1Ssa3v+mk0u+fU Rdlit9/LlA6YP3dPK2Qs1lttlhn365jzbL6LTL6T3XYumGHH/SHZL2pr/QffIKe1f68XPZvG KsQWzHBHVruN65PFui+Jpx99e/hm2qt5Odz72B5M1Z1c9st4FccByw0tp388F/cVuRudw5fp FnlY83vg/L/mm18mKbEUZyQaajEXFScCALu2LMkRAwAA X-CMS-MailID: 20221024101633eucas1p1a29f0a343d8162c26aff6cb60204bceb X-Msg-Generator: CA X-RootMTR: 20221007091126eucas1p16499c4717a1c8dc52e10a58c6f5d67af X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20221007091126eucas1p16499c4717a1c8dc52e10a58c6f5d67af References: <20221007091113.27-1-d.kozinski@samsung.com> <166591768194.12287.5804009794696497982@lain.khirnov.net> Subject: Re: [FFmpeg-devel] [PATCH v13 2/9] avcodec/evc_parser: Added parser implementaion for EVC format X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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=3D7794) 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 On Behalf Of Anton Khirnov Sent: niedziela, 16 pa=BCdziernika 2022 12:55 To: d.frankiewic@samsung.com; FFmpeg development discussions and patches Cc: Dawid Kozinski 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 =3D 0; > + > + if (bits_size >=3D EVC_NAL_HEADER_SIZE) { > + unsigned char *p =3D (unsigned char *)bits; > + // forbidden_zero_bit > + if ((p[0] & 0x80) !=3D 0) > + return -1; > + > + // nal_unit_type > + unit_type_plus1 =3D (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 =3D 0; > + > + if (bits_size >=3D EVC_NAL_UNIT_LENGTH_BYTE) { > + > + int t =3D 0; > + unsigned char *p =3D (unsigned char *)bits; > + > + for (int i =3D 0; i < EVC_NAL_UNIT_LENGTH_BYTE; i++) > + t =3D (t << 8) | p[i]; > + > + nalu_len =3D t; > + if (nalu_len =3D=3D 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 =3D s->priv_data; > + int nalu_type, nalu_size; > + unsigned char *bits =3D (unsigned char *)bs; > + int bits_size =3D 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 =3D AV_CODEC_ID_EVC; This seems unnecessary. [DONE] I've removed it (It has been fixed in patchset v14) > + s->picture_structure =3D AV_PICTURE_STRUCTURE_FRAME; > + s->key_frame =3D -1; > + > + nalu_size =3D read_nal_unit_length(bits, bits_size, avctx); > + if (nalu_size =3D=3D 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 +=3D EVC_NAL_UNIT_LENGTH_BYTE; > + bits_size -=3D EVC_NAL_UNIT_LENGTH_BYTE; > + > + nalu_type =3D get_nalu_type(bits, bits_size, avctx); Invalid type should be handled here. [DONE] It has been fixed in patchset v14 > + > + bits +=3D EVC_NAL_HEADER_SIZE; > + bits_size -=3D EVC_NAL_HEADER_SIZE; > + > + if (nalu_type =3D=3D 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 =3D parse_sps(bits, bits_size, ev); > + if (!sps) { > + av_log(avctx, AV_LOG_ERROR, "SPS parsing error\n"); > + return -1; > + } > + > + s->coded_width =3D sps->pic_width_in_luma_samples; > + s->coded_height =3D sps->pic_height_in_luma_samples; > + s->width =3D sps->pic_width_in_luma_samples; > + s->height =3D sps->pic_height_in_luma_samples; > + > + if (sps->profile_idc =3D=3D 1) avctx->profile =3D FF_PROFILE_EVC= _MAIN; > + else avctx->profile =3D 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 =3D AV_PIX_FMT_GRAY10LE; > + return -1; > + case 1: /* YCBCR420_10LE */ > + s->format =3D AV_PIX_FMT_YUV420P10LE; > + break; > + case 2: /* YCBCR422_10LE */ > + av_log(avctx, AV_LOG_ERROR, "YCBCR422_10LE: Not supported chroma format\n"); > + s->format =3D 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 =3D AV_PIX_FMT_YUV444P10LE; > + return -1; > + default: > + s->format =3D 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 =3D=3D EVC_PPS_NUT) { // NAL Unit type: PPS (Vi= deo Parameter Set) > + EVCParserPPS *pps; > + > + pps =3D 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 =3D=3D EVC_SEI_NUT) // NAL unit type: SEI (Supplemental Enhancement Information) > + return 0; > + else if (nalu_type =3D=3D EVC_IDR_NUT || nalu_type =3D=3D EVC_NOIDR_= NUT) { // NAL Unit type: Coded slice of a IDR or non-IDR picture > + EVCParserSliceHeader *sh; > + > + sh =3D 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 =3D AV_PICTURE_TYPE_B; > + break; > + } > + case EVC_SLICE_TYPE_P: { > + s->pict_type =3D AV_PICTURE_TYPE_P; > + break; > + } > + case EVC_SLICE_TYPE_I: { > + s->pict_type =3D AV_PICTURE_TYPE_I; > + break; > + } > + default: { > + s->pict_type =3D AV_PICTURE_TYPE_NONE; > + } > + } > + s->key_frame =3D (nalu_type =3D=3D 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 =3D s->priv_data; > + > + ev->nal_length_size =3D EVC_NAL_UNIT_LENGTH_BYTE; This seems to be write-only. > + ev->incomplete_nalu_prefix_read =3D 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 =3D s->priv_data; > + ParseContext *pc =3D &ev->pc; > + int is_dummy_buf =3D !buf_size; > + const uint8_t *dummy_buf =3D buf; > + > + if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) > + next =3D buf_size; > + else { > + next =3D evc_find_frame_end(s, buf, buf_size, avctx); > + if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) { > + *poutbuf =3D NULL; > + *poutbuf_size =3D 0; > + ev->to_read -=3D buf_size; > + return buf_size; > + } > + } > + > + is_dummy_buf &=3D (dummy_buf =3D=3D 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-request@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-request@ffmpeg.org with subject "unsubscribe".