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

[Denis Charmet]

Hi,

Le mardi 11 février 2014 à 08:17:47, Rafaël Carré a écrit :
> Hello,
> 
> .git/rebase-apply/patch:278: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
> 
You should treat those whitelineophobia.

> This is not a prototype
> 
'K

> > +/* 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.
>
Not yet but it shows that VCL NAL are before VPS and will be useful when
the packetizer evolves (and it was long enough to type when I first
wrote this to prevent me from deleting it when it ended up useless).

> > +    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).
> 

I told you it's a rip_off :)

> > +    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.
> 
same excuse :)
> > +        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)
> 
ok

> > +
> > +    }
> > +
> > +    *pb_ts_used = 0;
> 
> Please use false instead of 0
> 
ok

Regards,

-- 
Denis Charmet - TypX
Le mauvais esprit est un art de vivre