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

[Lynne <dev@lynne.ee>]

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 <d.kozinski@samsung.com>
> ---
>  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".