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 3C04E45582 for ; Tue, 7 Mar 2023 04:23:11 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0114368BCC3; Tue, 7 Mar 2023 06:23:07 +0200 (EET) Received: from w4.tutanota.de (w4.tutanota.de [81.3.6.165]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6348B68A630 for ; Tue, 7 Mar 2023 06:23:02 +0200 (EET) Received: from tutadb.w10.tutanota.de (unknown [192.168.1.10]) by w4.tutanota.de (Postfix) with ESMTP id B43AE1060159 for ; Tue, 7 Mar 2023 04:23:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1678162981; s=s1; d=lynne.ee; h=From:From:To:To:Subject:Subject:Content-Description:Content-ID:Content-Type:Content-Type:Content-Transfer-Encoding:Content-Transfer-Encoding:Cc:Date:Date:In-Reply-To:In-Reply-To:MIME-Version:MIME-Version:Message-ID:Message-ID:Reply-To:References:References:Sender; bh=dH1VjgHHLz3gRNTdI08EzZX4n1hdHwyX12EQGcgMt0k=; b=1A8jhLs0xdoiH4StXtrGNfog2a/kjiFngSWvUn7U+WGet3W0AQq2uQgcV2J3klJA j4g+OY1MMQfTtqwU9RaUYH8c0eeHLhp0EylyQZtkHwFbjk72q7Y5rpii2FfV4pHBl43 s0KJ8E6TEVepLyMozf3hVPRu/q2nVFpmBk1XOWLwm6NCTNwcxQYUqzujBfLYRXLhcKy xyOO6zbraNM7C/z019Fy2ascGazJMXN2oq/WJwFjOz6DU/sEElumtdJxGd2T4K3mqp/ hBKPhJPjXj4WGhDnFi50/82UYlwoWwnLnIk4wJ40/7DRkWU4js7ivWI9FZoDl3e7Z+3 HjDdgKyj5A== Date: Tue, 7 Mar 2023 05:23:01 +0100 (CET) From: Lynne To: FFmpeg development discussions and patches Message-ID: In-Reply-To: <20230102125312.1960-1-d.kozinski@samsung.com> References: <20230102125312.1960-1-d.kozinski@samsung.com> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH v16 2/9] avcodec/evc_parser: Added parser implementation 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="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Jan 2, 2023, 13:53 by d.kozinski@samsung.com: > - Added constants definitions for EVC parser > - Provided NAL units parsing following ISO_IEC_23094-1 > - EVC parser registration > > Signed-off-by: Dawid Kozinski > --- > libavcodec/Makefile | 1 + > libavcodec/evc.h | 155 +++++ > libavcodec/evc_parser.c | 1417 +++++++++++++++++++++++++++++++++++++++ > libavcodec/parsers.c | 1 + > 4 files changed, 1574 insertions(+) > create mode 100644 libavcodec/evc.h > create mode 100644 libavcodec/evc_parser.c > > > + > +// @see ISO_IEC_23094-1 (7.3.7 Reference picture list structure syntax) > +static int ref_pic_list_struct(GetBitContext* gb, RefPicListStruct* rpl) > +{ > + uint32_t delta_poc_st, strp_entry_sign_flag = 0; > + rpl->ref_pic_num = get_ue_golomb(gb); > + if (rpl->ref_pic_num > 0) > + { > + delta_poc_st = get_ue_golomb(gb); > + > + rpl->ref_pics[0] = delta_poc_st; > + if (rpl->ref_pics[0] != 0) > + { > + strp_entry_sign_flag = get_bits(gb, 1); > + > + rpl->ref_pics[0] *= 1 - (strp_entry_sign_flag << 1); > + } > + } > + > + for (int i = 1; i < rpl->ref_pic_num; ++i) > + { > + delta_poc_st = get_ue_golomb(gb); > + if (delta_poc_st != 0) { > + strp_entry_sign_flag = get_bits(gb, 1); > + } > + rpl->ref_pics[i] = rpl->ref_pics[i - 1] + delta_poc_st * (1 - (strp_entry_sign_flag << 1)); > + } > + > + return 0; > +} > Code style issues here, we don't put a newline on the bracket after an if or a for, and we don't use brackets at all for one-line if statements or if/else statements (as long as all statements are one-line). > +/** > + * @brief Reconstruct NAL Unit from incomplete data > + * > + * @param[in] s parser context > + * @param[in] data pointer to the buffer containg new data for current NALU prefix > + * @param[in] data_size amount of bytes to read from the input buffer > + * @param[out] nalu_prefix assembled NALU length > + * @param[in ] avctx codec context > + * > + * Assemble the NALU prefix storing NALU length if it has been split between 2 subsequent buffers (input chunks) incoming to the parser. > + * This is the case when the buffer size is not enough for the buffer to store the whole NAL unit prefix. > + * In this case, we have to get part of the prefix from the previous buffer and assemble it with the rest from the current buffer. > + * Then we'll be able to read NAL unit size. > + */ > +static int evc_assemble_nalu_prefix(AVCodecParserContext *s, const uint8_t *data, int data_size, > + uint8_t *nalu_prefix, AVCodecContext *avctx) > +{ > + EVCParserContext *ctx = s->priv_data; > + ParseContext *pc = &ctx->pc; > + > + // 1. pc->buffer contains previously read bytes of NALU prefix > + // 2. buf contains the rest of NAL unit prefix bytes > + // > + // ~~~~~~~ > + // EXAMPLE > + // ~~~~~~~ > + // > + // In the following example we assumed that the number of already read NAL Unit prefix bytes is equal 1 > + // > + // ---------- > + // pc->buffer -> conatins already read bytes > + // ---------- > + // __ pc->index == 1 > + // | > + // V > + // ------------------------------------------------------- > + // | 0 | 1 | 2 | 3 | 4 | ... | N | > + // ------------------------------------------------------- > + // | 0x00 | 0xXX | 0xXX | 0xXX | 0xXX | ... | 0xXX | > + // ------------------------------------------------------- > + // | PREF | | | | | ... | | > + // ------------------------------------------------------- > + // > + // ---------- > + // buf -> contains newly read bytes > + // ---------- > + // ------------------------------------------------------- > + // | 0 | 1 | 2 | 3 | 4 | ... | N | > + // ------------------------------------------------------- > + // | 0x00 | 0x00 | 0x3C | 0xXX | 0xXX | ... | 0xXX | > + // ------------------------------------------------------- > + // | PREF | PREF | PREF | | | ... | | > + // ------------------------------------------------------- > + // > + // ---------- > + // nalu_prefix > + // ---------- > + // --------------------------------- > + // | 0 | 1 | 2 | 3 | > + // --------------------------------- > + // | 0x00 | 0x00 | 0x00 | 0x3C | > + // --------------------------------- > + // | NALU LENGTH | > + // --------------------------------- > + // NAL Unit lenght = 60 (0x0000003C) > + // > The ascii art is neat, but it just makes this more complicated to read, and it doesn't even say all that much about it. just remove it. > + for (int i = 0; i < EVC_NALU_LENGTH_PREFIX_SIZE; i++) { > + if (i < pc->index) > + nalu_prefix[i] = pc->buffer[i]; > + else > + nalu_prefix[i] = data[i - pc->index]; > + } > + > + return 0; > +} > + > +/** > + * @brief Reconstruct NALU from incomplete data > + * Assemble NALU if it is split between multiple buffers > + * > + * This is the case when buffer size is not enough for the buffer to store NAL unit. > + * In this case, we have to get parts of the NALU from the previous buffers stored in pc->buffer and assemble it with the rest from the current buffer. > + * > + * @param[in] s parser context > + * @param[in] data pointer to the buffer containg new data for current NALU > + * @param[in] data_size amount of bytes to read from the input buffer > + * @param[out] nalu pointer to the memory block for storing assembled NALU > + * @param[in] nalu_size assembled NALU length > + * @param[in ] avctx codec context > + */ > You really don't need to put doxy on every single function, only for those that are not immediately self-explanatory, and you don't even need to use a doxy syntax for those, just a one or two-line comment on what it does, if it's not obvious. Rest of the code looks fine. _______________________________________________ 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".