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

[Kym Greenshields]

Ok, some quick responses - obviously I'll change whatever is needed to
match the existing semantics. 

I'll try and be verbose without leading off into tedium :)

On Wed, 2011-08-03 at 15:32 +0200, Jean-Baptiste Kempf wrote:
> 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?
> 
The changes above to the unsigned appear to have been introduced after
the release of 1.1.11 stable. I personally prefer that they remain
signed as this serves to re-inforce that the screen is being rotated +/-
offsets.
> > +#define CDG_TILES_WIDE		50
> > +#define CDG_TILES_HIGH		18
> 
> No tabs, in source code.

Ok - missed that on the 20 minute git seminar! :)

> >  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?
> 
This actually is the main patch!

To clarify, there appears to have been some misunderstanding on all
sides when the specification was implemented. Some manufacturers decided
that a BorderPreset should be issued before writing a tile in the border
area. 

This is fine when no scrolling or offsetting occurs subsequently - but
complete erases the tiles on all four sides for those discs where they
interpreted the spec as above - so best to only update on a
MemoryPreset, which in turn resets all offset too!

> > -    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;
> >

The above ensures that we don't stomp all over memory where there's been
a misread of a subcode packet. Currently, VLC will happily run off to
your neighbours' screen, if for example the payload was > 12 or > 50.

>   
> >      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.
> 

Again - personal preference.

> Best Regards,
> 

Let me know what to change and I'll get it done ASAP.
Kym.