I apologize if this email arrives twice, I initially sent it from the wrong account, which is not subscribed to the list.<br><br><div class="gmail_quote">Thank you for all the feedback,</div><div class="gmail_quote">I have attached a new patch which only uses sidplay2, since sidplay1 seemed to be too problematic to support, and should take into account the rest of the comments I received.</div>
<div class="gmail_quote"><br>The only thing I was unsure about was the alignment issue. I am setting sys->fmt.audio.i_blockalign, is there additional alignment I should be adding somewhere?</div><div class="gmail_quote">
<br></div><div class="gmail_quote">Thanks again,</div><div class="gmail_quote">Alan Fischer<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br><div class="gmail_quote">
2010/12/3 Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net" target="_blank">remi@remlab.net</a>></span><div><div class="h5"><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hello,<br>
<br>
A few comments inline...<br>
<br>
diff --git a/modules/demux/sid.cpp b/modules/demux/sid.cpp<br>
new file mode 100644<br>
index 0000000..e765909<br>
--- /dev/null<br>
+++ b/modules/demux/sid.cpp<br>
@@ -0,0 +1,337 @@<br>
+/**<br>
+ * @file sid.cpp<br>
+ * @brief Sidplay demux module for VLC media player<br>
+ */<br>
+/*****************************************************************************<br>
<div>+ * Copyright © 2010 Rémi Denis-Courmont<br>
<br>
</div>Copied from the GME demux? You should _add_ your copyright then.<br>
<br>
+ *<br>
+ * This library is free software; you can redistribute it and/or<br>
+ * modify it under the terms of the GNU Lesser General Public License<br>
+ * as published by the Free Software Foundation; either version 2.1<br>
+ * of the License, or (at your option) any later version.<br>
+ *<br>
+ * This library is distributed in the hope that it will be useful,<br>
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
+ * GNU General Public License for more details.<br>
+ *<br>
+ * You should have received a copy of the GNU Lesser General Public<br>
+ * License along with this library; if not, write to the Free Software<br>
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301<br>
USA<br>
+<br>
****************************************************************************/<br>
+<br>
+#ifdef HAVE_CONFIG_H<br>
+# include <config.h><br>
+#endif<br>
<div>+<br>
+#include <stdarg.h><br>
+#include <limits.h><br>
</div>+#include <assert.h><br>
<br>
I don't see any assert()'ion. Why do you include this file?<br>
<br>
+<br>
+#include <vlc_common.h><br>
+#include <vlc_input.h><br>
+#include <vlc_demux.h><br>
+#include <vlc_plugin.h><br>
+<br>
+#if SIDPLAY_VERSION==1<br>
+# include <sidplay/player.h><br>
+#elif SIDPLAY_VERSION==2<br>
+# include <sidplay/sidplay2.h><br>
+# include <sidplay/builders/resid.h><br>
+#endif<br>
+<br>
+#if SIDPLAY_VERSION==1<br>
+ // libsidplay's engine can not be re-used once deallocated, so<br>
allocate it statically<br>
+ static emuEngine engine;<br>
+#endif<br>
<br>
This is not going to work. First, VLC plugins must be re-entrant, and I<br>
don't suppose that the "engine" object is thread-safe? Second, the plug-in<br>
may be unloaded or reloaded, in which case, the memory leak will still<br>
occur. I would advise simply dropping support for sidplay 1. If you think<br>
sidplay 1 is really worth supporting, then you probable need to add a<br>
static mutex and mark the plug-in as not-to-be-unloaded.<br>
<br>
+static int Open (vlc_object_t *);<br>
+static void Close (vlc_object_t *);<br>
<div>+<br>
+vlc_module_begin ()<br>
+ set_shortname ("SID")<br>
</div><div>+#if SIDPLAY_VERSION==1<br>
+ set_description ("Sidplay")<br>
+#elif SIDPLAY_VERSION==2<br>
+ set_description ("Sidplay2")<br>
+#endif<br>
</div>+ set_category (CAT_INPUT)<br>
+ set_subcategory (SUBCAT_INPUT_DEMUX)<br>
+ set_capability ("demux", 10)<br>
<br>
Is that high enough? IIRC, priority 10 is used by some rather "promiscuous"<br>
demux plug-ins such as the MPEG-PS one.<br>
<br>
+ set_callbacks (Open, Close)<br>
+vlc_module_end ()<br>
+<br>
+struct demux_sys_t<br>
+{<br>
+#if SIDPLAY_VERSION==1<br>
+ emuEngine *player;<br>
+ emuConfig config;<br>
+ sidTune *tune;<br>
+ sidTuneInfo tuneInfo;<br>
+#elif SIDPLAY_VERSION==2<br>
+ sidplay2 *player;<br>
+ sid2_config_t config;<br>
+ sid2_info_t info;<br>
+ SidTune *tune;<br>
+ SidTuneInfo tuneInfo;<br>
+#endif<br>
+<br>
+ es_format_t fmt;<br>
+ es_out_id_t *es;<br>
+ date_t pts;<br>
+};<br>
+<br>
+<br>
+static int Demux (demux_t *);<br>
+static int Control (demux_t *, int, va_list);<br>
<div>+<br>
+static int Open (vlc_object_t *obj)<br>
</div>+{<br>
+ demux_t *demux = (demux_t *)obj;<br>
+<br>
+ int64_t size = stream_Size (demux->s);<br>
+ if (size < 4 || size > LONG_MAX) /* We need to load the whole file for<br>
sidplay */<br>
+ return VLC_EGENERIC;<br>
+<br>
+ uint8_t *data=(uint8_t*)malloc(size);<br>
<div>+ if(data==NULL)<br>
+ return VLC_ENOMEM;<br>
</div>+<br>
+ if(stream_Read(demux->s,data,size)<size){<br>
+ free(data);<br>
+ return VLC_EGENERIC;<br>
+ }<br>
<br>
You cannot do this here. You need to probe the file type first. Otherwise<br>
you will break any plug-in with lower priority.<br>
<br>
+#if SIDPLAY_VERSION==1<br>
+ sidTune *tune=new sidTune(0);<br>
+#elif SIDPLAY_VERSION==2<br>
+ SidTune *tune=new SidTune(0);<br>
+#endif<br>
+ if(unlikely(tune==NULL))<br>
+ return VLC_ENOMEM;<br>
<br>
Memory leak: free(data)<br>
<br>
+#if SIDPLAY_VERSION==1<br>
+ bool result=tune->load(data,size);<br>
+#elif SIDPLAY_VERSION==2<br>
+ bool result=tune->read(data,size);<br>
+#endif<br>
+ free(data);<br>
+ if(result==false){<br>
+ delete tune;<br>
+ return VLC_EGENERIC;<br>
+ }<br>
+<br>
+#if SIDPLAY_VERSION==1<br>
+ emuEngine *player=&engine;<br>
+#elif SIDPLAY_VERSION==2<br>
+ sidplay2 *player=new sidplay2();<br>
+#endif<br>
+ if(unlikely(player==NULL)){<br>
<div>+ delete tune;<br>
+ return VLC_ENOMEM;<br>
+ }<br>
+<br>
</div><div>+ demux_sys_t *sys = (demux_sys_t*) malloc(sizeof(demux_sys_t));<br>
</div>+ if (unlikely(sys == NULL)){<br>
+#if SIDPLAY_VERSION==2<br>
<div>+ delete player;<br>
+#endif<br>
+ delete tune;<br>
</div><div>+ return VLC_ENOMEM;<br>
+ }<br>
+ memset(sys,0,sizeof(demux_sys_t));<br>
</div>+<br>
+ sys->player=player;<br>
+ sys->tune=tune;<br>
+<br>
+ tune->getInfo(sys->tuneInfo);<br>
+<br>
+#if SIDPLAY_VERSION==1<br>
<div>+ player->getConfig(sys->config);<br>
+ sys->config.sampleFormat=SIDEMU_UNSIGNED_PCM;<br>
<br>
</div>Unsigned? Then why do you use the S16N (signed 16-bits native endianess)<br>
codec?<br>
<div><br>
+ sys->config.bitsPerSample=16;<br>
+ sys->config.channels=2;<br>
</div>+ player->setConfig(sys->config);<br>
<div>+#elif SIDPLAY_VERSION==2<br>
+ sys->info=player->info();<br>
+ sys->config=player->config();<br>
+ {<br>
+ ReSIDBuilder *resid=new ReSIDBuilder("ReSID");<br>
+ if (unlikely(resid == NULL)){<br>
+#if SIDPLAY_VERSION==2<br>
</div><div>+ delete player;<br>
+#endif<br>
+ delete tune;<br>
+ free(sys);<br>
+ return VLC_ENOMEM;<br>
+ }<br>
+ resid->create(sys->info.maxsids);<br>
+ resid->sampling(sys->config.frequency);<br>
+ sys->config.sidEmulation=resid;<br>
+ }<br>
</div>+ sys->config.precision=16;<br>
+ sys->config.playback=sys->info.channels==2?sid2_stereo:sid2_mono;<br>
+ player->config(sys->config);<br>
+#endif<br>
<div>+<br>
+ es_format_Init (&sys->fmt, AUDIO_ES, VLC_CODEC_S16N);<br>
+<br>
+ sys->fmt.audio.i_rate = sys->config.frequency;<br>
+#if SIDPLAY_VERSION==1<br>
+ sys->fmt.audio.i_channels = sys->config.channels;<br>
+ sys->fmt.audio.i_bitspersample = sys->config.bitsPerSample;<br>
</div>+#elif SIDPLAY_VERSION==2<br>
<div>+ sys->fmt.audio.i_channels = sys->info.channels;<br>
+ sys->fmt.audio.i_bitspersample = sys->config.precision;<br>
+#endif<br>
+ sys->fmt.audio.i_bytes_per_frame = sys->fmt.audio.i_channels *<br>
sys->fmt.audio.i_bitspersample/8;<br>
+ sys->fmt.audio.i_frame_length = sys->fmt.audio.i_bytes_per_frame;<br>
+ sys->fmt.audio.i_blockalign = sys->fmt.audio.i_bytes_per_frame;<br>
+ sys->fmt.i_bitrate = sys->fmt.audio.i_rate *<br>
sys->fmt.audio.i_bytes_per_frame;<br>
</div>+<br>
+ sys->es = es_out_Add (demux->out, &sys->fmt);<br>
+ date_Init (&sys->pts, sys->fmt.audio.i_rate, 1);<br>
+ date_Set (&sys->pts, 0);<br>
+<br>
+#if SIDPLAY_VERSION==1<br>
+ result=sidEmuInitializeSong(*sys->player,*sys->tune,0);<br>
+#elif SIDPLAY_VERSION==2<br>
+ sys->tune->selectSong(0);<br>
+ result=(sys->player->load(sys->tune)>=0);<br>
+ sys->player->fastForward(100);<br>
+#endif<br>
+ if(result==false){<br>
+#if SIDPLAY_VERSION==2<br>
+ delete sys->player;<br>
+ delete sys->config.sidEmulation;<br>
+#endif<br>
+ delete sys->tune;<br>
+ free(sys);<br>
<br>
Could be factored to Close(obj);<br>
<br>
+ return VLC_ENOMEM;<br>
+ }<br>
+<br>
+ /* Callbacks */<br>
+ demux->pf_demux = Demux;<br>
+ demux->pf_control = Control;<br>
+ demux->p_sys = sys;<br>
+<br>
+ return VLC_SUCCESS;<br>
+}<br>
+<br>
+<br>
+static void Close (vlc_object_t *obj)<br>
+{<br>
+ demux_t *demux = (demux_t *)obj;<br>
+ demux_sys_t *sys = demux->p_sys;<br>
+<br>
+#if SIDPLAY_VERSION==2<br>
+ delete sys->player;<br>
+ delete sys->config.sidEmulation;<br>
+#endif<br>
+ delete sys->tune;<br>
+ free (sys);<br>
+}<br>
+<br>
+static int Demux (demux_t *demux)<br>
+{<br>
+ demux_sys_t *sys = demux->p_sys;<br>
+ int i_bk = ( sys->fmt.audio.i_bitspersample / 8 ) *<br>
sys->fmt.audio.i_channels;<br>
+ block_t *block = block_New( p_demux, sys->fmt.audio.i_rate / 10 * i_bk<br>
);<br>
+ if (unlikely(block == NULL))<br>
+ return 0;<br>
+<br>
+ if(sys->tune->getStatus()==false)<br>
+ return 0;<br>
+<br>
+ int i_read=block->i_buffer;<br>
+#if SIDPLAY_VERSION==1<br>
+<br>
sidEmuFillBuffer(*sys->player,*sys->tune,(void*)block->p_buffer,block->i_buffer);<br>
+#elif SIDPLAY_VERSION==2<br>
+ i_read=sys->player->play((void*)block->p_buffer,block->i_buffer);<br>
+#endif<br>
+ if(i_read<=0){<br>
+ block_Release (block);<br>
+ return 0;<br>
+ }<br>
+ block->i_buffer=i_read;<br>
+ block->i_pts = block->i_dts = VLC_TS_0 + date_Get (&sys->pts);<br>
+<br>
+ es_out_Control (demux->out, ES_OUT_SET_PCR, block->i_pts);<br>
+<br>
+ es_out_Send (demux->out, sys->es, block);<br>
+<br>
+ date_Increment (&sys->pts, i_read/i_bk);<br>
+<br>
+ return 1;<br>
+}<br>
+<br>
+<br>
<div>+static int Control (demux_t *demux, int query, va_list args)<br>
+{<br>
</div>+ demux_sys_t *sys = demux->p_sys;<br>
+<br>
+ switch (query)<br>
+ {<br>
+ case DEMUX_GET_TIME:{<br>
+ int64_t *v=va_arg(args,int64_t*);<br>
+ *v=0;<br>
<br>
This statement has no effects.<br>
<br>
+#if SIDPLAY_VERSION==1 && defined(SIDEMU_TIME_COUNT)<br>
+ *v=sys->player->getSecondsThisSong()*1000;<br>
<br>
Use CLOCK_FREQ.<br>
<br>
+#elif SIDPLAY_VERSION==2<br>
+ *v=sys->player->time()*sys->player->timebase()*10;<br>
+#endif<br>
+ *v=*v * 1000;<br>
+ return VLC_SUCCESS;<br>
+ }<br>
<div>+<br>
+ case DEMUX_GET_META:{<br>
+ vlc_meta_t *p_meta = (vlc_meta_t *)va_arg( args, vlc_meta_t*<br>
);<br>
+ vlc_meta_SetTitle( p_meta, sys->tuneInfo.infoString[0] );<br>
+ vlc_meta_SetArtist( p_meta, sys->tuneInfo.infoString[1] );<br>
+ vlc_meta_SetCopyright( p_meta, sys->tuneInfo.infoString[2] );<br>
+ return VLC_SUCCESS;<br>
</div>+ }<br>
+<br>
+ case DEMUX_GET_TITLE_INFO:<br>
+ if( sys->tuneInfo.songs > 1 )<br>
+ {<br>
+ input_title_t ***ppp_title = (input_title_t***)va_arg(<br>
args, input_title_t*** );<br>
+ int *pi_int = (int*)va_arg( args, int* );<br>
+<br>
+ *pi_int = sys->tuneInfo.songs;<br>
+ *ppp_title = (input_title_t**)xmalloc( sizeof(<br>
input_title_t**) * sys->tuneInfo.songs );<br>
+<br>
+ for( int i = 0; i < sys->tuneInfo.songs; i++ )<br>
+ {<br>
+ char psz_temp[16];<br>
+ snprintf(psz_temp, 15, "Song %i", i);<br>
+ (*ppp_title)[i] = vlc_input_title_New();<br>
+ (*ppp_title)[i]->psz_name = strdup(psz_temp);<br>
+ }<br>
+<br>
+ return VLC_SUCCESS;<br>
+ }<br>
+ return VLC_EGENERIC;<br>
+<br>
+ case DEMUX_SET_TITLE:{<br>
+ int i_idx = (int)va_arg( args, int );<br>
+ bool result=false;<br>
+#if SIDPLAY_VERSION==1<br>
+ result=sidEmuInitializeSong(*sys->player,*sys->tune,i_idx+1);<br>
+#elif SIDPLAY_VERSION==2<br>
+ sys->tune->selectSong(i_idx+1);<br>
+ result=(sys->player->load(sys->tune)>=0);<br>
+#endif<br>
+ if(result==false){<br>
+ return VLC_EGENERIC;<br>
+ }<br>
+ demux->info.i_title = i_idx;<br>
+ demux->info.i_update = INPUT_UPDATE_TITLE;<br>
+ msg_Dbg( demux, "set song %i", i_idx);<br>
+ return VLC_SUCCESS;<br>
+ }<br>
+<br>
+ }<br>
+<br>
+ return VLC_EGENERIC;<br>
+}<br>
<div><div><br>
<br>
--<br>
Rémi Denis-Courmont<br>
<a href="http://www.remlab.net" target="_blank">http://www.remlab.net</a><br>
<a href="http://fi.linkedin.com/in/remidenis" target="_blank">http://fi.linkedin.com/in/remidenis</a><br>
<br>
_______________________________________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="http://mailman.videolan.org/listinfo/vlc-devel" target="_blank">http://mailman.videolan.org/listinfo/vlc-devel</a><br>
</div></div></blockquote></div></div></div><br>
</blockquote></div><br>