[vlc-devel] [PATCH v2] Add support for reading EIA-608 closed captions via V4L2 VBI devices

[Rémi Denis-Courmont]

Mostly fine but I think there is a small bug left...

On Sun, 28 Oct 2012 22:34:09 -0400, Devin Heitmueller
<dheitmueller at kernellabs.com> wrote:
> @@ -88,12 +99,32 @@ int DemuxOpen( vlc_object_t *obj )
>          goto error;
>      sys->fd = fd;
>  
> -    if (InitVideo (demux, fd, caps))
> +    v4l2_std_id std;
> +    if (InitVideo (demux, fd, caps, &std, &entry))
>      {
>          v4l2_close (fd);
>          goto error;
>      }
>  
> +#ifdef ZVBI_COMPILED
> +    char *vbi_path = var_InheritString (obj, CFG_PREFIX"vbidev");
> +    if (vbi_path != NULL && (std & V4L2_STD_NTSC_M))
> +    {    
> +        sys->vbi_cap = OpenVBIDev (obj, vbi_path);
> +        if (sys->vbi_cap)
> +            InitVBI(demux);
> +        free(vbi_path);
> +    }
> +#endif
> +
> +    /* Now that everything is setup, start the reading thread */
> +    if (vlc_clone (&sys->thread, entry, demux,
VLC_THREAD_PRIORITY_INPUT))
> +    {
> +        if (sys->bufv != NULL)
> +            StopMmap (sys->fd, sys->bufv, sys->bufc);
> +        goto error;

That leaks, I think.
It might be simpler to call InitVBI() from InitVideo().

> +    }
> +    
>      sys->controls = ControlsInit (VLC_OBJECT(demux), fd);
>      sys->start = mdate ();
>      demux->pf_demux = NULL;
> @@ -439,14 +470,33 @@ static int InitVideo (demux_t *demux, int fd,
> uint32_t caps)
>          return -1;
>      }
>  
> -    if (vlc_clone (&sys->thread, entry, demux,
VLC_THREAD_PRIORITY_INPUT))
> +    return 0;
> +}
> +
> +#ifdef ZVBI_COMPILED
> +static int InitVBI (demux_t *demux)
> +{
> +    demux_sys_t *sys = demux->p_sys;
> +
> +    for (int i = 0; i < VBI_NUM_CC_STREAMS; i++)
>      {
> -        if (sys->bufv != NULL)
> -            StopMmap (sys->fd, sys->bufv, sys->bufc);
> -        return -1;
> +        es_format_t fmt;
> +
> +        es_format_Init( &fmt, SPU_ES, VLC_FOURCC('c','c',0x31 + i,' ')
);

Cosmetic but I'd use '1' rather than 0x31.

> +        if (asprintf(&fmt.psz_description, "CC%d", i + 1) >= 0)
> +        {
> +            msg_Dbg( demux, "new spu es %4.4s", (char*)&fmt.i_codec );
> +            sys->p_es_subt[i] = es_out_Add( demux->out, &fmt );
> +        }
>      }
> +    
> +    /* Do a single read and throw away the results so that ZVBI calls
> +       the STREAMON ioctl() */
> +    GrabVBI(demux, sys->vbi_cap, sys->p_es_subt, VBI_NUM_CC_STREAMS);
> +
>      return 0;
>  }
> +#endif
>  
>  void DemuxClose( vlc_object_t *obj )
>  {
> @@ -459,6 +509,15 @@ void DemuxClose( vlc_object_t *obj )
>          StopMmap (sys->fd, sys->bufv, sys->bufc);
>      ControlsDeinit( obj, sys->controls );
>      v4l2_close (sys->fd);
> +
> +#ifdef ZVBI_COMPILED
> +    if( sys->vbi_cap )
> +    {
> +        close(vbi_capture_fd(sys->vbi_cap));

Oh? zvbi does not close the descriptor in that case?

> +        vbi_capture_delete( sys->vbi_cap );
> +    }
> +#endif
> +
>      free( sys );
>  }
>  
> @@ -503,11 +562,22 @@ static void *UserPtrThread (void *data)
>      demux_t *demux = data;
>      demux_sys_t *sys = demux->p_sys;
>      int fd = sys->fd;
> -    struct pollfd ufd[1];
> -
> +    struct pollfd ufd[2];
> +    nfds_t numfds = 1;
> +    
>      ufd[0].fd = fd;
>      ufd[0].events = POLLIN;
>  
> +#ifdef ZVBI_COMPILED
> +    if ( sys->vbi_cap )
> +    {
> +        ufd[1].fd = vbi_capture_fd(sys->vbi_cap);
> +        ufd[1].events = POLLIN;
> +        ufd[1].revents = 0;

Strictly speaking, there is no need to set revents.

> +        numfds++;
> +    }
> +#endif
> +    
>      int canc = vlc_savecancel ();
>      for (;;)
>      {

-- 
Rémi Denis-Courmont
Sent from my collocated server