[vlc-devel] [PATCH 1/1] codec/dirac: Rewrite libdirac encoding support; Remove decoding support

[Laurent Aimar]

On Mon, Nov 10, 2008, davidf+nntp at woaf.net wrote:
> +#define ENC_BUFSIZE 1024*1024*10
If it is the size of the output buffer, and the librairie cannot to the right
allocation by itself, I think it would be better to compute it base on raw
video size (assuming that default case will not expand).

> +#define PTS_TLB_SIZE 32
 Is it a encoding picture reordering (for timestamp), if so doesn't the librairie
outputs/defines the max value ?

 +    encoder_sys_t *p_sys = p_enc->p_sys;
> +    for( int i=0; i<PTS_TLB_SIZE; i++) {
> +        p_sys->pts_tlb[i].i_empty = 1;
 Isn't i_empty used as a bool and its types changed to it ?

> -        case STATE_INVALID:
> -            msg_Dbg( p_dec, "STATE_INVALID" );
> -            break;
> +            p_sys->pts_tlb[i].u_pnum = u_pnum;
> +            p_sys->pts_tlb[i].i_pts = i_pts;
> +            p_sys->pts_tlb[i].i_empty = 0;
 Could you add the return statement ? It prevents future errors.
> +    /* guess the video format based upon number of lines and picture height */
> +    int i = 0;
> +    VideoFormat guessed_video_fmt = VIDEO_FORMAT_CUSTOM;
> +    do {
> +        if( dirac_format_guess[i].i_height != p_enc->fmt_in.video.i_height )
> +            continue;
> +        int src_fps = p_enc->fmt_in.video.i_frame_rate / p_enc->fmt_in.video.i_frame_rate_base;
> +        int delta_fps = abs( dirac_format_guess[i].i_approx_fps - src_fps );
> +        if( delta_fps > 2 )
 Wouldn't a search minimizing the error be better ?
> +    p_sys->ctx.src_params.frame_rate.numerator = p_enc->fmt_in.video.i_frame_rate;
> +    p_sys->ctx.src_params.frame_rate.denominator = p_enc->fmt_in.video.i_frame_rate_base;
> +    vlc_ureduce( (unsigned*) &p_sys->ctx.src_params.pix_asr.numerator,
> +                 (unsigned*) &p_sys->ctx.src_params.pix_asr.denominator,
 Those casts seem ugly, using temporary variable would be better.

> +    if( val.psz_string )
> +        free( val.psz_string );
 Useless check (for all of them).

> +    var_Get( p_enc, ENC_CFG_PREFIX ENC_QUALITY_FACTOR, &val );
> +    p_sys->ctx.enc_params.qf = val.f_float;
> +
> +    /* use bitrate from sout-transcode-vb in kbps */
> +    p_sys->ctx.enc_params.trate = p_enc->fmt_out.i_bitrate / 1000;
> +    var_Get( p_enc, ENC_CFG_PREFIX ENC_TARGETRATE, &val );
> +    if( val.i_int > -1 )
> +        p_sys->ctx.enc_params.trate = val.i_int;
 Could you use the var_GetFloat/Integer/... variants ?
(for all of them)

> +/* Attempt to find dirac picture number in an encapsulation unit */
> +static int ReadDiracPictureNumber( uint32_t *p_picnum, block_t *p_block )
> +{
> +    uint32_t u_pos = 4;
> +    /* protect against falling off the edge */
> +    while ( u_pos + 13 < p_block->i_buffer ) {
> +        /* find the picture startcode */
> +        if ( p_block->p_buffer[u_pos] & 0x08 ) {
> +            *p_picnum = GetDWBE( p_block->p_buffer + u_pos + 9 );
> +            return 1;
> +        }
> +        /* skip to the next dirac data unit */
> +        uint32_t u_npo = GetDWBE( p_block->p_buffer + u_pos + 1 );
> +        if (u_npo == 0)
> +            u_npo = 13;
> +        u_pos += u_npo;
 As it can overflow, I think you may have an infinite loop with corrupted
stream.

> +    if( !p_sys->p_dirac )
> +    {
> +        date_t date;
> +        /* Initialise the encoder with the encoder context */
 Why not doing it inside the encoder Open function.

> +        if( 1 == p_sys->ctx.enc_params.picture_coding_mode ) {
 Isn't there a define instead of magic value ? (if it is a bool then do not
check against a specific value).

> +    /* store dts in a queue, so that they appear in order in
> +     * coded order */
> +    p_block = block_New( p_enc, 1 );
 Using block here seems a bit overkill, but is acceptable.

> +             /* store all extracted blocks in a chain and gather up when an
> +              * entire encapsulation unit is avaliable (ends with a picture) */
> +             block_ChainAppend( &p_sys->p_chain, p_block );
 It is way faster to use block_ChainAppendLast.

-- 
fenrir