[vlc-devel] [PATCH 2/2] codec/dirac: Rewrite libdirac(-research) encoding support

[Laurent Aimar]

Hi,

 It seems to be missing my latest remarks, I will not repeat them
and will simply wait for a new version.

 There are additionnal comments:

> +static mtime_t GetPicturePTS( encoder_t *p_enc, uint32_t u_pnum )
> +{
> +    encoder_sys_t *p_sys = p_enc->p_sys;
> +
> +    for( int i=0; i<PTS_TLB_SIZE; i++ ) {
> +        if( (!p_sys->pts_tlb[i].i_empty) &&
> +            (p_sys->pts_tlb[i].u_pnum == u_pnum)) {
 Useless parenthesis.

> +/*****************************************************************************
>   * OpenEncoder: probe the encoder and return score
>   *****************************************************************************/
>  static int OpenEncoder( vlc_object_t *p_this )
> @@ -95,7 +428,6 @@ static int OpenEncoder( vlc_object_t *p_this )
>      encoder_t *p_enc = (encoder_t *)p_this;
>      encoder_sys_t *p_sys = p_enc->p_sys;
>      vlc_value_t val;
> -    float f_quality;
>  
>      if( p_enc->fmt_out.i_codec != VLC_FOURCC('d','r','a','c') &&
>          !p_enc->b_force )
> @@ -107,58 +439,319 @@ static int OpenEncoder( vlc_object_t *p_this )
>      if( ( p_sys = (encoder_sys_t *)malloc(sizeof(encoder_sys_t)) ) == NULL )
 Useless cast in C. Using sizeof(*p_sys) may be safer.
>      memset( p_sys, 0, sizeof(encoder_sys_t) );
 ... missing * or use *p_sys or use calloc iof the above malloc.

> -    dirac_encoder_context_init( &p_sys->ctx, VIDEO_FORMAT_CUSTOM );
> -    /* */
> +    /* 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;
 You MUST check the frame rate validity (x/0 is allowed when the frame rate
information is not availaible or N/A).
 Same a bit later for date_Init.

> +    if ( !val.psz_string ) {}
> +    else if ( !strcmp( val.psz_string, "420" ) ) {
> +        p_enc->fmt_in.i_codec = VLC_FOURCC('I','4','2','0');
> +        p_enc->fmt_in.video.i_bits_per_pixel = 12;
> +        p_sys->ctx.src_params.chroma = format420;
> +        p_sys->i_buffer_in = p_enc->fmt_in.video.i_width * p_enc->fmt_in.video.i_height * 3 / 2;
> +    }
> +    else if ( !strcmp( val.psz_string, "422" ) ) {
> +        p_enc->fmt_in.i_codec = VLC_FOURCC('I','4','2','2');
> +        p_enc->fmt_in.video.i_bits_per_pixel = 16;
> +        p_sys->ctx.src_params.chroma = format422;
> +        p_sys->i_buffer_in = p_enc->fmt_in.video.i_width * p_enc->fmt_in.video.i_height * 2;
> +    }
> +    else if ( !strcmp( val.psz_string, "444" ) ) {
> +        p_enc->fmt_in.i_codec = VLC_FOURCC('I','4','4','4');
> +        p_enc->fmt_in.video.i_bits_per_pixel = 24;
> +        p_sys->ctx.src_params.chroma = format444;
> +        p_sys->i_buffer_in = p_enc->fmt_in.video.i_width * p_enc->fmt_in.video.i_height * 3;
> +    }
> +    if( val.psz_string )
> +        free( val.psz_string );
 Some parameters seem not initialied to a valid value if !val.psz_string or
val.psz_string is not a valid value.
(Either handle this case as I420 or as a fatal error).

It seems to be the case for other string variables.

>  /****************************************************************************
>   * Encode: the whole thing
>   ****************************************************************************
> - * This function spits out ogg packets.
> + * This function spits out encapsulation units.
>   ****************************************************************************/
>  static block_t *Encode( encoder_t *p_enc, picture_t *p_pic )
>  {
>      encoder_sys_t *p_sys = p_enc->p_sys;
> -    block_t *p_block, *p_chain = NULL;
> +    block_t *p_block, *p_output_chain = NULL;
>      int i_plane, i_line, i_width, i_src_stride;
>      uint8_t *p_dst;
>  
> -    /* Copy input picture in encoder input buffer (stride by stride) */
> +    /* we only know if the sequence is interlaced when the first
> +     * picture arrives, so final setup is done here */
> +    /* XXX todo, detect change of interlace */
> +    p_sys->ctx.src_params.topfieldfirst = p_pic->b_top_field_first;
> +    p_sys->ctx.src_params.source_sampling = !p_pic->b_progressive;
> +
> +    if( p_sys->b_auto_field_coding )
> +        p_sys->ctx.enc_params.picture_coding_mode = !p_pic->b_progressive;
> +
> +    if( !p_sys->p_dirac )
> +    {
> +        date_t date;
> +        /* Initialise the encoder with the encoder context */
> +        p_sys->p_dirac = dirac_encoder_init( &p_sys->ctx, 0 );
> +        if( !p_sys->p_dirac )
> +        {
> +            msg_Err( p_enc, "Failed to initialize dirac encoder\n" );
> +            p_enc->b_error = 1;
I think it will segfault after that as p_dirac is NULL.

> +    /* store pts in a lookaside buffer, so that the same pts may
> +     * be used for the picture in coded order */
> +    StorePicturePTS( p_enc, p_sys->i_input_picnum, p_pic->date );
> +    p_sys->i_input_picnum++;
> +
> +    /* store dts in a queue, so that they appear in order in
> +     * coded order */
> +    p_block = block_New( p_enc, 1 );
> +    p_block->i_dts = p_pic->date - p_sys->i_pts_offset;
 p_block MUST be checked.

> +    block_FifoPut( p_sys->p_dts_fifo, p_block );
> +    p_block = NULL;
> +
> +    /* for field coding mode, insert an extra value into both the
> +     * pts lookaside buffer and dts queue, offset to correspond
> +     * to a one field delay. */
> +    if( 1 == p_sys->ctx.enc_params.picture_coding_mode ) {
> +        StorePicturePTS( p_enc, p_sys->i_input_picnum, p_pic->date + p_sys->i_field_time );
> +        p_sys->i_input_picnum++;
> +
> +        p_block = block_New( p_enc, 1 );
> +        p_block->i_dts = p_pic->date - p_sys->i_pts_offset + p_sys->i_field_time;
 Same here.
> +        block_FifoPut( p_sys->p_dts_fifo, p_block );
> +        p_block = NULL;
> +    }
> +    dirac_encoder_state_t state;
> +    /* Retrieve encoded frames from encoder */
> +    do
> +        p_sys->p_dirac->enc_buf.buffer = p_sys->p_buffer_out;
> +        p_sys->p_dirac->enc_buf.size = ENC_BUFSIZE;
> +        state = dirac_encoder_output( p_sys->p_dirac );
> +        switch( state )
> +        {
> +        case ENC_STATE_AVAIL: {
> +             uint32_t pic_num;
> +
> +             /* extract data from encoder temporary buffer. */
> +             p_block = block_New( p_enc, p_sys->p_dirac->enc_buf.size );
> +             memcpy( p_block->p_buffer, p_sys->p_dirac->enc_buf.buffer,
> +                     p_sys->p_dirac->enc_buf.size );
 Same here.

Regards,

-- 
fenrir