[vlc-devel] [PATCH] Add option to disable DVD subtitle transparency

[Kaarlo Räihä]

Rémi Denis-Courmont kirjoitti 17.01.2010 kello 13:42:
> Le dimanche 17 janvier 2010 13:33:49 Kaarlo Räihä, vous avez écrit :
> > As someone pointed out in forums:
> > http://forum.videolan.org/viewtopic.php?f=14&t=70201&p=233859
> > 
> > Some DVD subtitles have "bad" transparency values. I created a patch to
> >  solve this. It adds option that makes subtitle parsing skip
> >  transparency/alpha values given in stream and instead use default values
> >  which solid ones (no transparency at all).
> 
> diff --git a/modules/codec/spudec/parse.c b/modules/codec/spudec/parse.c
> index 5c2bc30..6fc4a4d 100644
> --- a/modules/codec/spudec/parse.c
> +++ b/modules/codec/spudec/parse.c
> @@ -293,11 +293,14 @@ static int ParseControlSeq( decoder_t *p_dec, 
> subpicture_t *p_spu,
>                  return VLC_EGENERIC;
>              }
>  
> -            b_cmd_alpha = true;
> -            spu_data_cmd.pi_alpha[3] =
>  (p_sys->buffer[i_index+1]>>4)&0x0f;
> -            spu_data_cmd.pi_alpha[2] = (p_sys->buffer[i_index+1])&0x0f;
> -            spu_data_cmd.pi_alpha[1] =
>  (p_sys->buffer[i_index+2]>>4)&0x0f;
> -            spu_data_cmd.pi_alpha[0] = (p_sys->buffer[i_index+2])&0x0f;
> +            if(!p_sys->b_disabletrans)
> +            { /* If we want to use original transparency values */
> +                b_cmd_alpha = true;
> +                spu_data_cmd.pi_alpha[3] = (p_sys-
> >buffer[i_index+1]>>4)&0x0f;
> +                spu_data_cmd.pi_alpha[2] =
>  (p_sys->buffer[i_index+1])&0x0f;
> +                spu_data_cmd.pi_alpha[1] = (p_sys-
> >buffer[i_index+2]>>4)&0x0f;
> +                spu_data_cmd.pi_alpha[0] =
>  (p_sys->buffer[i_index+2])&0x0f;
> +            }
> 
> Are those values initialized somewhere?

Yes, http://git.videolan.org/?p=vlc.git;a=blob;f=modules/codec/spudec/parse.c;h=5c2bc30015ce4e16d27593030df9a5ece926a882;hb=HEAD and lines 188-191

>  
> @@ -49,6 +53,9 @@ vlc_module_begin ()
>      set_subcategory( SUBCAT_INPUT_SCODEC )
>      set_callbacks( DecoderOpen, Close )
>  
> +    add_bool( "dvdsub-transparency", false, NULL,
> +    		  DVDSUBTRANS_DISABLE_TEXT, DVDSUBTRANS_DISABLE_LONGTEXT, true )
> +    change_need_restart ()
> 
> Why does this need a restart??

I am living under assumption that if there isn't a callback to care about runtime changes, restart is needed. If this assumption is wrong, I can change the code.

> 
> @@ -79,6 +86,9 @@ static int DecoderOpen( vlc_object_t *p_this )
>      p_dec->p_sys = p_sys = malloc( sizeof( decoder_sys_t ) );
>  
>      p_sys->b_packetizer = false;
> +    p_sys->b_disabletrans = false;
> +    if( var_CreateGetBool( p_dec, "dvdsub-transparency" ) )
> +        p_sys->b_disabletrans = true;
> 
> Why do you create a new object variable? This seems totally
>  contradictory
> with 
> requiring a restart.

I should use var_Create( p_dec, "dvdsub-transparency", VLC_VAR_BOOL | VLC_VAR_DOINHERIT ); instead or ?


> diff --git a/modules/codec/spudec/spudec.h
>  b/modules/codec/spudec/spudec.h
> index 9e5b5fb..df2fa27 100644
> --- a/modules/codec/spudec/spudec.h
> +++ b/modules/codec/spudec/spudec.h
> @@ -26,6 +26,7 @@
>  struct decoder_sys_t
>  {
>      int b_packetizer;
> +    int b_disabletrans;
> 
> Why is this not a boolean?

I just followed existing coding style (b_packetizer is an int). It could be bool. 

> 
> -- 
> Rémi Denis-Courmont
> http://www.remlab.net/
> http://fi.linkedin.com/in/remidenis
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel


....................................................................
Luukku Plus -paketilla pääset eroon tila- ja turvallisuusongelmista.
Hanki Luukku Plus ja helpotat elämääsi. http://www.mtv3.fi/luukku