[vlc-devel] [PATCH] [WIP] New mpg123 decoder

[Ludovic Fauvet]

On Mon, Nov 24, 2014, at 14:00, Jean-Baptiste Kempf wrote:
> On 24 Nov, Ludovic Fauvet wrote :
> > +libmpg123_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(codecdir)'
> 
> Do you need that?

I do not but libtool maintainers recommend to use it.
 
> > +static unsigned int mpg123_refcount = 0;
> > +static vlc_mutex_t mpg123_mutex = VLC_STATIC_MUTEX;
> 
> Can you avoid this?

I'm afraid I can't since mpg123_init and mpg123_exit are not reentrant
and they need to be protected in some way.

> > +struct decoder_sys_t
> > +{
> > +    mpg123_handle * p_handle;
> > +    date_t          end_date;
> > +    int             i_rate;
> 
> You don't seem to use p_sys->i_rate.
> 
> > +    set_shortname( N_("mpg123") )
> 
> Does this need translation?
> 
> > + ****************************************************************************/
> > +static block_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> > +{
> > +    int i_err;
> > +	block_t *p_block = *pp_block;
> > +	decoder_sys_t *p_sys = p_dec->p_sys;
> > +
> > +	if( !pp_block || !p_block )
> 
> Careful about tabs.
> 
> > +    if( p_dec->fmt_in.i_codec != VLC_CODEC_MPGA && p_dec->fmt_in.i_codec != VLC_CODEC_MP3 &&
> > +        p_dec->fmt_in.i_codec != VLC_FOURCC('m','p','g','3') )
> 
> Does this one actually happen?
> 
> With my kindest regards,
> 
> -- 
> Jean-Baptiste Kempf
> http://www.jbkempf.com/ - +33 672 704 734
> Sent from my Electronic Device

All other concerns addressed in the upcoming patch.
Thanks for reviewing my patch.

-- 
Ludovic Fauvet
www.videolan.org