[vlc-devel] [PATCH 2/2] V4L2: Add support for radio devices.

[Thierry Reding]

* Rémi Denis-Courmont wrote:
>    Hello,
> 
> Do I get this right that this is meant to control analog audio tuners but
> the data is fetched with an ALSA input slave?

Correct yes. Basically the only thing required for radio support is opening
the device, setting the frequency and keeping the device open to keep the
driver sending data to the ALSA device.

> 
> Comments inline...
> 
> On Thu,  4 Aug 2011 09:53:52 +0200, Thierry Reding
> <thierry.reding at avionic-design.de> wrote:
> > ---
> >  modules/access/v4l2.c |  130
> >  ++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 files changed, 122 insertions(+), 8 deletions(-)
> > 
> > diff --git a/modules/access/v4l2.c b/modules/access/v4l2.c
> > index f074a9c..eec8f09 100644
> > --- a/modules/access/v4l2.c
> > +++ b/modules/access/v4l2.c
> > @@ -416,8 +416,10 @@ static bool IsPixelFormatSupported( demux_t
> *p_demux,
> >                                            unsigned int i_pixelformat );
> >  
> >  static int InitVideoDev( vlc_object_t *, demux_sys_t *, bool );
> > -static int ProbeVideoDev( vlc_object_t *, demux_sys_t *,
> > +static int InitRadioDev( vlc_object_t *, demux_sys_t *, bool );
> > +static int ProbeDev( vlc_object_t *, demux_sys_t *,
> >                                   const char *psz_device );
> > +static bool IsRadioDev( demux_sys_t *p_sys );
> >  
> >  static int ControlList( vlc_object_t *, demux_sys_t *, int , bool, bool
> );
> >  static int Control( vlc_object_t *, demux_sys_t *, int i_fd,
> > @@ -645,11 +647,19 @@ static int FindMainDevice( vlc_object_t *p_this,
> > demux_sys_t *p_sys,
> >      /* TODO: if using default device, loop through all /dev/video*
> until
> >       * one works */
> >      msg_Dbg( p_this, "opening device '%s'", p_sys->psz_device );
> > -    p_sys->i_fd = ProbeVideoDev( p_this, p_sys, p_sys->psz_device );
> > +    p_sys->i_fd = ProbeDev( p_this, p_sys, p_sys->psz_device );
> >      if( p_sys->i_fd >= 0 )
> >      {
> > -        msg_Dbg( p_this, "'%s' is a video device", p_sys->psz_device );
> > -        InitVideoDev( p_this, p_sys, b_demux );
> > +        if( IsRadioDev( p_sys ) )
> > +        {
> > +            msg_Dbg( p_this, "'%s' is a radio device",
> p_sys->psz_device
> > );
> > +            InitRadioDev( p_this, p_sys, b_demux );
> > +        }
> > +        else
> > +        {
> > +            msg_Dbg( p_this, "'%s' is a video device",
> p_sys->psz_device
> > );
> > +            InitVideoDev( p_this, p_sys, b_demux );
> > +        }
> >      }
> >  
> >      if( p_sys->i_fd < 0 ) return VLC_EGENERIC;
> > @@ -1035,6 +1045,9 @@ static void DemuxClose( vlc_object_t *p_this )
> >      demux_t     *p_demux = (demux_t *)p_this;
> >      demux_sys_t *p_sys   = p_demux->p_sys;
> >  
> > +    if( IsRadioDev( p_sys ) )
> > +        goto common;
> > +
> 
> IMHO, a separate "access_demux" radio sub-module with dedicated Open and
> Close functions would be cleaner. In fact, it seems like there is not that
> much common code, so you might even consider creating a completely new
> plug-in. What do you think?

Most code in the V4L2 module is not needed for radio support, but I thought
this was easiest because it allows sharing the probing code. If you think
it's cleaner to have a separate module or submodule, I can take a look. It
will take some time though because I'll need to dig in some more to figure
out how to implement this best.

> 
> >      /* Stop video capture */
> >      if( p_sys->i_fd >= 0 )
> >      {
> > @@ -1099,6 +1112,7 @@ static void DemuxClose( vlc_object_t *p_this )
> >          free( p_sys->p_buffers );
> >      }
> >  
> > +common:
> >      CommonClose( p_this, p_sys );
> >  }
> >  
> > @@ -1268,7 +1282,10 @@ static block_t *AccessRead( access_t * p_access )
> >  
> >      /* Wait for data */
> >      if( poll( &fd, 1, 500 ) > 0 ) /* Timeout after 0.5 seconds since I
> >      don't know if pf_demux can be blocking. */
> > -        return GrabVideo( VLC_OBJECT(p_access), p_sys );
> > +    {
> > +        if( !IsRadioDev( p_sys ) )
> > +            return GrabVideo( VLC_OBJECT(p_access), p_sys );
> > +    }
> >  
> >      return NULL;
> >  }
> 
> If there is never going to be any data, then AccessOpen() should fail. So
> AccessRead() should never be reached to begin with.

I thought that failing AccessOpen() would perhaps lead to VLC failing to open
the stream. Perhaps I should dig through some more code since I'm obviously
missing some pieces.

> 
> > @@ -1335,7 +1352,7 @@ static int Demux( demux_t *p_demux )
> >              return -1;
> >          }
> >  
> > -    if( fd.revents )
> > +    if( fd.revents && !IsRadioDev( p_sys ) )
> >      {
> >           block_t *p_block = GrabVideo( VLC_OBJECT(p_demux), p_sys );
> >           if( p_block )
> 
> poll() should be used if events are not dealt with. I guess you can just
> set pf_demux to NULL, as in access/screen/xcb.c.
[...]

Okay, I'll keep that in mind for the submodule rewrite.

Thanks for the review,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110804/c37c9478/attachment.sig>