[vlc-devel] [PATCH 1/1] [rawvideo] Fix timestamps & input options

[Laurent Aimar]

Hi,

On Wed, Nov 05, 2008, davidf+nntp at woaf.net wrote:
Packetizer:
-----------
> +    date_t m_pts;
 I don't think m_ is used anywhere else (you can just drop it).

> +    date_Init( &p_sys->m_pts, p_dec->fmt_out.video.i_frame_rate,
> +               p_dec->fmt_out.video.i_frame_rate_base);
 Becarefull that it may not be valid (ie i_frame_rate or i_frame_rate_base == 0 ).
You will have to use a default value (like 25fps).

> +    if( !p_block->i_pts && !date_Get( &p_sys->m_pts ) )
> +    if( p_block->i_pts )
You MUST use dts if pts is not provided. Some demuxer will only set
dts (like avi, or asf).


Demuxer:
--------
> +    date_t m_pcr;
 Same here.
> +        for( int i = 0; p_presets[i].psz_ext ; i++ )
> +            if( !strcasecmp( psz_ext, p_presets[i].psz_ext ) )
>              {
> +                p_preset = &p_presets[i];
>                  b_valid = true;
>                  break;
>              }
 I would prefer adding {}. It is too easy to do something wrong later.

> +    psz_tmp = var_CreateGetString( p_demux, "rawvid-chroma" );
> +    if( psz_tmp && strlen( psz_tmp ) >= 4 )
>      {
 It would probably be better to check for == 4 to detect error no ?

> +    psz_tmp = var_CreateGetString( p_demux, "rawvid-fps" );
> +    if( psz_tmp ) {
 Could you fix the { position ?

> +        char *p_ptr;
> +        /* fps can either be n/d or q.f
> +         * for accuracy, avoid representation in float */
> +        u_fps_num = strtol( psz_tmp, &p_ptr, 10 );
> +        if( '/' == *p_ptr ) {
 Idem.
> +            p_ptr++;
> +            u_fps_den = strtol( p_ptr, NULL, 10 );
>          }
> -        if( !*psz_chroma )
> -        {
> -            free( psz_chroma );
> -            psz_chroma = strdup( psz_chroma );
> +        else if( '.' == *p_ptr ) {
 Idem.
> +            char *p_end;
> +            p_ptr++;
> +            int i_frac =  strtol( p_ptr, &p_end, 10 );
> +            u_fps_den = (p_end - p_ptr) * 10;
> +            if( !u_fps_den )
> +                u_fps_den = 1;
> +            u_fps_num = u_fps_num * u_fps_den + i_frac;
>          }
> +        else if( '\0' == *p_ptr ) {
 Idem.
> +            u_fps_den = 1;
> +        }
> +        free( psz_tmp );
>      }
 And you have a weird way to express test (value == variable iof
variable == value like we do everwhere else in vlc).

> -    if( i_width <= 0 || i_height <= 0 )
> +    psz_tmp = var_CreateGetString( p_demux, "rawvid-aspect-ratio" );
> +    if( psz_tmp )
>      {
> -        msg_Err( p_demux, "width and height must be strictly positive." );
> -        free( psz_aspect_ratio );
> -        free( psz_chroma );
> -        free( p_sys );
> -        return VLC_EGENERIC;
> +        char *psz_denominator = strchr( psz_tmp, ':' );
> +        if( psz_denominator ) {
 { position.

> @@ -394,8 +420,10 @@ static int Control( demux_t *p_demux, int i_query, va_list args )
>  {
>      demux_sys_t *p_sys  = p_demux->p_sys;
>  
> +    int i_bps = 8 * p_sys->frame_size * p_sys->m_pcr.i_divider_num
> +              / p_sys->m_pcr.i_divider_den;
 You should do the calcul in 64 bits as it may overflow (with
1920x1080 YUY2 at 60000/10001 fps.

 The result can overflow with a too high resolution/frame rate but that's
a problem with demux_vaControlHelper that can be fixed later.

-- 
fenrir