[vlc-devel] Patch for C64 SID suppport

[Jean-Baptiste Kempf]

On Thu, Dec 02, 2010 at 03:57:34PM -0600, Alan Fischer wrote :
> Hello,
> I've attached a patch that adds a demux module which uses libsidplay2 or
> libsidplay (looks for libsidplay2 first) to allow the playback of C64 SID
> files.

First, thanks a lot, and a lot more for your work.


>  dnl
> +dnl  SIDPlay plugin
> +dnl
> +AC_LANG_PUSH([C++])
> +  AC_ARG_ENABLE(sid,
> +    [  --enable-sid            C64 SID support (default auto)])
> +  AS_IF([test "${enable_sid}" != "no"], [
> +    AC_CHECK_HEADER([sidplay/sidplay2.h], [
> +      AC_DEFINE(SIDPLAY_VERSION,2,[Sidplay version, 1 or 2])
> +      VLC_ADD_LIBS([sid], [-lsidplay2 -lresid-builder])
> +      VLC_ADD_PLUGIN([sid])
> +    ], [
> +      AC_CHECK_HEADER([sidplay/player.h], [
> +        AC_DEFINE(SIDPLAY_VERSION,1,[Sidplay version, 1 or 2])
> +        VLC_ADD_LIBS([sid], [-lsidplay])
> +        VLC_ADD_PLUGIN([sid])
> +      ], [
> +        AS_IF([test "x${enable_sid}" != "x"], [
> +          AC_MSG_ERROR([Sidplay or Sidplay2 cannot be found. Please install the development files.])
> +        ])
> +      ])
> +    ])
> +  ])
> +AC_LANG_POP([C++])
Please use PKG_CONFIG. At least for sidplay2.
What is the point of supporting Sidplay1, btw?

Use AC_MSG_WARN when failing, instead of displaying nothing.

Tell if it is a Demuxer, a codec, etc... in the help "C64 SID" is too
cryptic.

> + * Copyright © 2010 Rémi Denis-Courmont
Wrong copyright.

> +#if SIDPLAY_VERSION==1

Why not using more classical HAVE_SIDPLAY_PLAYER_H and
HAVE_SIDPLAY_SIDPLAY2_H and define SID_VERSION_2 and SID_VERSION_1
and then #ifdef SID_VERSION_2 instead of those weird #if == 1?

> +vlc_module_begin ()
> +    set_shortname ("SID")
sid

> +#if SIDPLAY_VERSION==1
> +    set_description ("Sidplay")
> +#elif SIDPLAY_VERSION==2
> +    set_description ("Sidplay2") 
> +#endif
This isn't a description.
Missing translation too :D

> +static int Open (vlc_object_t *obj)


> +    if(data==NULL)
> +        return VLC_ENOMEM;
unlikely( )

> +    demux_sys_t *sys = (demux_sys_t*) malloc(sizeof(demux_sys_t));
> +    if (unlikely(sys == NULL)){
> +#if SIDPLAY_VERSION==2
> +        delete player;
> +#endif
> +        delete tune;
> +        return VLC_ENOMEM;
> +    }
> +    memset(sys,0,sizeof(demux_sys_t));

Use calloc there.

> +    player->getConfig(sys->config);
> +    sys->config.sampleFormat=SIDEMU_UNSIGNED_PCM;
> +    sys->config.bitsPerSample=16;
> +    sys->config.channels=2;
Some alignment wouldn't hurt :)

> +#elif SIDPLAY_VERSION==2
> +    sys->info=player->info();
> +    sys->config=player->config();
> +    {
> +        ReSIDBuilder *resid=new ReSIDBuilder("ReSID");
> +        if (unlikely(resid == NULL)){
> +#if SIDPLAY_VERSION==2
aren't you already in VERSION ==2?

> +            delete player;
> +#endif
> +            delete tune;
> +            free(sys);
> +            return VLC_ENOMEM;
> +        }
> +        resid->create(sys->info.maxsids);
> +        resid->sampling(sys->config.frequency);
> +        sys->config.sidEmulation=resid;
> +    }

Why those extra } ?

> +    es_format_Init (&sys->fmt, AUDIO_ES, VLC_CODEC_S16N);
> +
> +    sys->fmt.audio.i_rate = sys->config.frequency;
> +#if SIDPLAY_VERSION==1
> +    sys->fmt.audio.i_channels = sys->config.channels;
> +    sys->fmt.audio.i_bitspersample = sys->config.bitsPerSample;
> +#elif SIDPLAY_VERSION==2
> +    sys->fmt.audio.i_channels = sys->info.channels;
> +    sys->fmt.audio.i_bitspersample = sys->config.precision;
> +#endif
> +    sys->fmt.audio.i_bytes_per_frame = sys->fmt.audio.i_channels * sys->fmt.audio.i_bitspersample/8;
> +    sys->fmt.audio.i_frame_length = sys->fmt.audio.i_bytes_per_frame;
> +    sys->fmt.audio.i_blockalign = sys->fmt.audio.i_bytes_per_frame;
> +    sys->fmt.i_bitrate = sys->fmt.audio.i_rate * sys->fmt.audio.i_bytes_per_frame;
Some alignment would be cool too :D

> +    return VLC_SUCCESS;
> +}

Maybe some error_path could be factorized.


> +static int Control (demux_t *demux, int query, va_list args)
> +{

> +        case DEMUX_GET_META:{
> +            vlc_meta_t *p_meta = (vlc_meta_t *)va_arg( args, vlc_meta_t* );
> +            vlc_meta_SetTitle( p_meta, sys->tuneInfo.infoString[0] );
> +            vlc_meta_SetArtist( p_meta, sys->tuneInfo.infoString[1] );
> +            vlc_meta_SetCopyright( p_meta, sys->tuneInfo.infoString[2] );
> +            return VLC_SUCCESS;
Are those infoString[0..2] always defined?

In general, some code cosmetics would be nice. I know VLC old style is
too much whitespace, but here, if could gain a bit with some of it :D
(like around the = or after the ":" in the cases )

I hope to have helped


Best Regards,

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