[vlc-devel] Symbian Aout

[Jean-Baptiste Kempf]

Hello,

On Sun, Feb 13, 2011 at 02:41:26AM +0530, Pankaj yadav wrote :
>  * Copyright © 2010 Pankaj Yadav

2010-2011

> #define bool int
Why?

> #define FRAME_SIZE 12442
Why this number?

> typedef struct AudioSpec
Why this name?

>     set_shortname( "SYMBIANAUDIO" )
Please, no. This needs a better name.

>     add_shortcut( "sym" )
No. Could be symlink.

> public:
>     int v_freq,v_channels,closed;

Closed shouldn't be public.
Why isn't closed in p_sys ?

I think you should do a setter for v_freq.

>     void sclose();
I don't see the use. Or make it a correct setter.

>     void FillBuffer(aout_instance_t *v_p_aout,TInt16* buffer, TUint len);
Missing spaces.

> void MdaSound::sclose()
> {
>     closed = 1;
> }
See above comment.

>     iStream->Open( &iSettings );
Can Open fail?

> void MdaSound::MaoscOpenComplete(TInt aError)
> {
> 
> 
unnecessary whitespace.

>     desired.format     = 1;
Why 1?

  From here:
>     if((desired.freq < 8000) || (desired.freq > 96000))
>     {
>         return VLC_EGENERIC;
>     }
>     else if((desired.freq >= 8000) && (desired.freq < 11025))
>     {
>         SymbianSound.v_freq = 0x10;
>     }
>     else if((desired.freq >= 11025) && (desired.freq < 12000))
>     {
>         SymbianSound.v_freq = 0x40;
>     }
>     else if((desired.freq >= 12000) && (desired.freq < 16000))
>     {
>         SymbianSound.v_freq = 0x80;
>     }
>     else if((desired.freq >= 16000) && (desired.freq < 22050))
>     {
>         SymbianSound.v_freq = 0x100;
>     }
>     else if((desired.freq >= 22050) && (desired.freq < 24000))
>     {
>         SymbianSound.v_freq = 0x400;
>     }
>     else if((desired.freq >= 24000) && (desired.freq < 32000))
>     {
>         SymbianSound.v_freq = 0x800;
>     }
>     else if((desired.freq >= 32000) && (desired.freq < 44100))
>     {
>         SymbianSound.v_freq = 0x1000;
>     }
>     else if((desired.freq >= 44100) && (desired.freq < 48000))
>     {
>         SymbianSound.v_freq = 0x4000;
>     }
>     else if((desired.freq >= 48000) && (desired.freq<64000))
>     {
>         SymbianSound.v_freq = 0x10000;
>     }
>     else if((desired.freq >= 64000) && (desired.freq<96000))
>     {
>         SymbianSound.v_freq = 0x40000;
>     }
>     else
>     {
>         SymbianSound.v_freq=0x20000;
>     }
to there, it should be  SymbianSound function, probabaly.

>     if(pthread_create(&tid,NULL,createdevice,NULL))
Why pthread directly?

>         if ( var_Type( p_aout, "audio-device" ) == 0 )
>         {
>             var_Create( p_aout, "audio-device",
>                         VLC_VAR_INTEGER | VLC_VAR_HASCHOICE );
>             text.psz_string = _("Audio device");
>             var_Change( p_aout, "audio-device", VLC_VAR_SETTEXT, &text, NULL );
> 
>             val.i_int = (obtained.channels == 2) ? AOUT_VAR_STEREO :
>                         AOUT_VAR_MONO;
>             text.psz_string = (obtained.channels == 2) ? (char*)N_("Stereo") :(char*)
>                               N_("Mono");
>             var_Change( p_aout, "audio-device",
>                         VLC_VAR_ADDCHOICE, &val, &text );
>             var_AddCallback( p_aout, "audio-device", aout_ChannelsRestart,
>                              NULL );
>         }
>     }
>     else if ( var_Type( p_aout, "audio-device" ) == 0 )
>     {
>         var_Create( p_aout, "audio-device",
>                     VLC_VAR_INTEGER | VLC_VAR_HASCHOICE );
>         text.psz_string = _("Audio device");
>         var_Change( p_aout, "audio-device", VLC_VAR_SETTEXT, &text, NULL );
> 
>         val.i_int = AOUT_VAR_STEREO;
>         text.psz_string = N_("Stereo");
>         var_Change( p_aout, "audio-device", VLC_VAR_ADDCHOICE, &val, &text );
>         val.i_int = AOUT_VAR_MONO;
>         text.psz_string = N_("Mono");
>         var_Change( p_aout, "audio-device", VLC_VAR_ADDCHOICE, &val, &text );
>         if ( i_nb_channels == 2 )
>         {
>             val.i_int = AOUT_VAR_STEREO;
>         }
>         else
>         {
>             val.i_int = AOUT_VAR_MONO;
>         }
>         var_Change( p_aout, "audio-device", VLC_VAR_SETDEFAULT, &val, NULL );
>         var_AddCallback( p_aout, "audio-device", aout_ChannelsRestart, NULL );
>     }

You should update all that to more modern Variable functions.

>     val.b_bool = true;
>     var_Set( p_aout, "intf-change", val );
idem.

>     if ( p_buffer != NULL )
>     {
>         vlc_memcpy( p_stream, p_buffer->p_buffer, 2*i_len );
>         aout_BufferFree( p_buffer );
>     }
>     else
>     {
>         vlc_memset( p_stream, 0,2*i_len );
>     }

This part deserves comments

> void *createdevice(void * arg)
Maybe a name better consistent?

Best Regards,

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