[vlc-devel] [PATCH] Added IVTC deinterlacer (NTSC film mode)

[Jean-Baptiste Kempf]

Hello,

Thanks a lot for the work.

Here are rough remarks on a first analysis.

On Fri, Dec 31, 2010 at 03:41:39AM +0200, Juha Jeronen wrote :
> +#define DEINTERLACE_FILM    9
Here and all the following, you use "FILM" as a name, isn't that too
generic for the user to understand?
And in the code, maybe it should be IVTC?

> +    /* Film. This one needs a lot of state. */
What about having a sub-structure or a separate one?

> -    return fc < 1 ? false : true;
> +    /* Changed for Film deinterlacer - return the raw fc value.
> +       RenderX() treats the result as a logical, so it won't break. */
> +/*    return fc < 1 ? false : true; */
> +    return fc;

Maybe this should be in a separate patch

>  #ifdef CAN_COMPILE_MMXEXT
>  static inline int XDeint8x8DetectMMXEXT( uint8_t *src, int i_src )
>  {
> -
>      int y, x;
>      int32_t ff, fr;
>      int fc;
> @@ -1182,7 +1213,6 @@ static inline int XDeintNxNDetect( uint8_t *src,  
> int i_src,
>      int ff, fr;
>      int fc;
>
> -
>      /* Detect interlacing */
>      /* FIXME way too simple, need to be more like XDeint8x8Detect */
>      ff = fr = 0;
> @@ -1199,7 +1229,10 @@ static inline int XDeintNxNDetect( uint8_t *src,  
> int i_src,
>              fc++;
>      }

Cosmetics should not be in this patch

> -    return fc < 2 ? false : true;
> +    /* Changed for Film deinterlacer - return the raw fc value.
> +       RenderX() treats the result as a logical, so it won't break. */
> +/*    return fc < 2 ? false : true; */
> +    return fc < 2 ? 0 : fc;
>  }

as above?

> +static void ComposeFrame( filter_t *p_filter, picture_t *p_outpic,  
> picture_t *p_inpic_top, picture_t *p_inpic_bottom )
> +{
> +    assert( p_filter != NULL );
> +    assert( p_outpic != NULL );
> +    assert( p_inpic_top != NULL );
> +    assert( p_inpic_bottom != NULL );
> +
> +    int i_plane;
> +    for( i_plane = 0 ; i_plane < p_inpic_top->i_planes ; i_plane++ )
You can defined int directly in the for, because vlc is C99

> +    for( i_plane = 0 ; i_plane < p_pic->i_planes ; i_plane++ )
same here

> +        for( y = 1; y < i_lasty; y+=2 )
and there

> +                for( x = 0; x < w; ++x )
> +                {
> +                    long C = *src_c;
> +                    long P = *src_p;
> +                    long N = *src_n;
Are you sure long is the right type? I kind of doubt it, seeing how long
behaves differently in LP64 and LLP64.

> +#define FIELD_PAIR_TPBP 0
> +#define FIELD_PAIR_TPBC 1
> +#define FIELD_PAIR_TCBP 2
> +#define FIELD_PAIR_TCBC 3
> +#define FIELD_PAIR_TCBN 4
> +#define FIELD_PAIR_TNBC 5
> +#define FIELD_PAIR_TNBN 6
enum maybe?

> +#define CADENCE_POS_PROGRESSIVE 0
> +#define CADENCE_POS_TFF_ABC 1
> +#define CADENCE_POS_TFF_BCD 2
> +#define CADENCE_POS_TFF_CDE 3
> +#define CADENCE_POS_TFF_EAB 4
> +#define CADENCE_POS_BFF_ABC 5
> +#define CADENCE_POS_BFF_BCD 6
> +#define CADENCE_POS_BFF_CDE 7
> +#define CADENCE_POS_BFF_EAB 8
idem

> +/* Operations for film frame reconstruction.
> +   Indices: [TFD][i_cadence_pos] */
> +#define IVTC_OP_DROP_FRAME 0
> +#define IVTC_OP_COPY_N 1
> +#define IVTC_OP_COPY_C 2
> +#define IVTC_OP_COMPOSE_TNBC 3
> +#define IVTC_OP_COMPOSE_TCBN 4
idem

> +            /* allocate new */
> +            p_sys->pp_ivtc_working_area[i] = picture_NewFromFormat(  
> &p_next->format );
> +
> +            assert( p_sys->pp_ivtc_working_area[i] != NULL );
Are you sure you have to assert here? If mem is full, wouldn't
picture_NewFromFormat return NULL ? I would quit, nicely...

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/
+33 672 704 734