[vlc-devel] [PATCH 1/2] Add a trivial hevc packetizer

[Rafaël Carré]

Hello,

.git/rebase-apply/patch:278: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

On 02/11/14 01:00, Denis Charmet wrote:


> +/****************************************************************************
> + * Local prototypes
> + ****************************************************************************/

This is not a prototype

> +struct decoder_sys_t
> +{
> +    /* */
> +    packetizer_t packetizer;
> +
> +    bool     b_vcl;
> +    block_t *p_frame;
> +
> +};
> +
> +/* NAL types from https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-H.265-201304-I!!PDF-E&type=items */
> +enum nal_unit_type_e
> +{
> +    TRAIL_N    = 0,
> +    TRAIL_R    = 1,
> +    TSA_N      = 2,
> +    TSA_R      = 3,
> +    STSA_N     = 4,
> +    STSA_R     = 5,
> +    RADL_N     = 6,
> +    RADL_R     = 7,
> +    RASL_N     = 8,
> +    RASL_R     = 9,
> +    /* 10 to 15 reserved */
> +    /* Key frames */
> +    BLA_W_LP   = 16,
> +    BLA_W_RADL = 17,
> +    BLA_N_LP   = 18,
> +    IDR_W_RADL = 19,
> +    IDR_N_LP   = 20,
> +    CRA        = 21,
> +    /* 22 to 31 reserved */
> +    /* Non VCL NAL*/
> +    VPS        = 32,
> +    SPS        = 33,
> +    PPS        = 34,
> +    AUD        = 35, /* Access unit delimiter */
> +    EOS        = 36, /* End of sequence */
> +    EOB        = 37, /* End of bitstream */
> +    FD         = 38, /* Filler data*/
> +    PREF_SEI   = 39, /* Prefix SEI */
> +    SUFF_SEI   = 40, /* Suffix SEI */
> +    UNKNOWN_NAL
> +};

Do we need this enum?

Only VPS is ever used for < comparison.

> +static block_t *Packetize(decoder_t *, block_t **);
> +
> +static void PacketizeReset(void *p_private, bool b_broken);
> +static block_t *PacketizeParse(void *p_private, bool *pb_ts_used, block_t *);
> +static int PacketizeValidate(void *p_private, block_t *);
> +
> +static const uint8_t p_hevc_startcode[3] = {0x00, 0x00, 0x01};
> +
> +/*****************************************************************************
> + * Open
> + *****************************************************************************/
> +static int Open(vlc_object_t *p_this)
> +{
> +    decoder_t     *p_dec = (decoder_t*)p_this;
> +    decoder_sys_t *p_sys;
> +
> +    if (p_dec->fmt_in.i_codec != VLC_CODEC_HEVC)
> +        return VLC_EGENERIC;
> +
> +    if ((p_dec->p_sys = p_sys = calloc(1, sizeof(decoder_sys_t))) == NULL)
> +        return VLC_ENOMEM;

Personally I don't like assignement in comparison, especially if the
p_sys variable is
only used once (you could use p_dec->p_sys directly).

> +    packetizer_Init(&p_sys->packetizer,
> +                    p_hevc_startcode, sizeof(p_hevc_startcode),
> +                    NULL, 0, 0,
> +                    PacketizeReset, PacketizeParse, PacketizeValidate, p_dec);
> +
> +    /* Copy properties */
> +    es_format_Copy(&p_dec->fmt_out, &p_dec->fmt_in);
> +    p_dec->fmt_out.i_codec = p_dec->fmt_in.i_codec;

es_format_Copy already does this.

> +    /* Set callback */
> +    p_dec->pf_packetize = Packetize;
> +
> +    return VLC_SUCCESS;
> +
> +}


> +static block_t *PacketizeParse(void *p_private, bool *pb_ts_used, block_t *p_block)
> +{
> +    decoder_t *p_dec = p_private;
> +    decoder_sys_t *p_sys = p_dec->p_sys;
> +
> +    block_t * p_nal = NULL;
> +
> +    while (p_block->i_buffer > 5 && p_block->p_buffer[p_block->i_buffer-1] == 0x00 )
> +        p_block->i_buffer--;
> +
> +    bs_t bs;
> +    bs_init(&bs, p_block->p_buffer+3, p_block->i_buffer-3);
> +
> +    /* Get NALU type */
> +    uint32_t forbidden_zero_bit = bs_read1(&bs);
> +
> +    if (forbidden_zero_bit)
> +    {
> +        msg_Err(p_dec,"Forbidden zero bit not null, corrupted NAL");
> +        p_sys->p_frame = NULL;
> +        p_sys->b_vcl = false;
> +        return NULL;
> +    }
> +    uint32_t nalu_type = bs_read(&bs,6);
> +    bs_skip(&bs, 9);
> +
> +    if (nalu_type < VPS)
> +    {
> +        /* NAL is a VCL NAL */
> +        p_sys->b_vcl = true;
> +
> +        uint32_t first_slice_in_pic = bs_read1(&bs);
> +
> +        if (first_slice_in_pic && p_sys->p_frame)
> +        {
> +            p_nal = block_ChainGather(p_sys->p_frame);
> +            p_sys->p_frame = NULL;
> +        }
> +
> +        block_ChainAppend(&p_sys->p_frame, p_block);
> +    }
> +    else
> +    {
> +        if (p_sys->b_vcl)
> +        {
> +            p_nal = block_ChainGather(p_sys->p_frame);
> +            p_nal->p_next = p_block;
> +            p_sys->p_frame = NULL;
> +        }
> +        else
> +            p_nal = p_block;
> +
> +        p_sys->b_vcl =false;

Move inside if (p_sys->b_vcl)

> +
> +    }
> +
> +    *pb_ts_used = 0;

Please use false instead of 0

> +    return p_nal;
> +}