[vlc-devel] [PATCH] Correct scroll effect and clearing in TV+Graphics decoder

[Jean-Baptiste Kempf]

Thanks a lot for your work.
First quick pass of review:

On Tue, Aug 02, 2011 at 08:55:41PM +0100, Kym Greenshields wrote :
>  /* The screen size */
> -#define CDG_SCREEN_WIDTH 300u
> -#define CDG_SCREEN_HEIGHT 216u
> +#define CDG_SCREEN_WIDTH 300
> +#define CDG_SCREEN_HEIGHT 216

Why this change?

>  /* The border of the screen size */
> -#define CDG_SCREEN_BORDER_WIDTH 6u
> -#define CDG_SCREEN_BORDER_HEIGHT 12u
> +#define CDG_SCREEN_BORDER_WIDTH 6
> +#define CDG_SCREEN_BORDER_HEIGHT 12

Idem?

> +#define CDG_TILES_WIDE		50
> +#define CDG_TILES_HIGH		18

No tabs, in source code.

>  struct decoder_sys_t
>  {
>      uint8_t  color[16][3];
> @@ -59,6 +62,7 @@ struct decoder_sys_t
>      uint8_t  *p_screen;
>  
>      int      i_packet;
> +	int		 i_border_color;

Idem.

> +	/* Only update the border if it differs from this one */
> +	if ( p_cdg->i_border_color != i_color ) {
> +		ScreenFill( p_cdg,   0,   0,
> +                CDG_SCREEN_WIDTH, CDG_SCREEN_BORDER_HEIGHT, p_cdg->i_border_color );
> +		ScreenFill( p_cdg,   0, CDG_SCREEN_HEIGHT-CDG_SCREEN_BORDER_HEIGHT,
> +                CDG_SCREEN_WIDTH, CDG_SCREEN_BORDER_HEIGHT, p_cdg->i_border_color );
> +		ScreenFill( p_cdg,   0, CDG_SCREEN_BORDER_HEIGHT,
> +                CDG_SCREEN_BORDER_WIDTH, CDG_SCREEN_HEIGHT-CDG_SCREEN_BORDER_HEIGHT, p_cdg->i_border_color );
> +		ScreenFill( p_cdg,   CDG_SCREEN_WIDTH-CDG_SCREEN_BORDER_WIDTH, CDG_SCREEN_BORDER_HEIGHT,
> +                CDG_SCREEN_BORDER_WIDTH, CDG_SCREEN_HEIGHT-CDG_SCREEN_BORDER_HEIGHT, p_cdg->i_border_color );
> +    }

Is this related to the main patch? Couldn't this change be in a separate
patch?

> -    sy = (p_data[2] & 0x1f)*12;
> -    sx = (p_data[3] & 0x3f)*6;
> +	sy = (p_data[2] & 0x1f);
> +	sx = (p_data[3] & 0x3f);
> +	
> +	if ( sy >= CDG_TILES_HIGH ) return 0;
> +	if ( sx >= CDG_TILES_WIDE ) return 0;
> +	
> +    sy *= CDG_SCREEN_BORDER_HEIGHT;
> +    sx *= CDG_SCREEN_BORDER_WIDTH;
>  
>      for( y = 0; y < 12; y++ )
>      {
> @@ -272,6 +294,7 @@ static int DecodeTileBlock( decoder_sys_t *p_cdg, const uint8_t *p_data, int doX
>  
>  static int DecodeScroll( decoder_sys_t *p_cdg, const uint8_t *p_data, int b_copy )
>  {
> +
Unrelated change.
>      /* Copy back */
> -    for( unsigned y = 0; y < CDG_SCREEN_HEIGHT; y++ )
> +    for( int y = 0; y < CDG_SCREEN_HEIGHT; y++ )
Why?

> -        for( unsigned x = 0; x < CDG_SCREEN_WIDTH; x++ )
> +        for( int x = 0; x < CDG_SCREEN_WIDTH; x++ )
idem.

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device