[vlc-devel] [PATCH 1/3] Initial support for EIA-608 closed captions via V4L2 VBI devices

[Devin Heitmueller]

Hello Rémi,

Thanks for taking the time to provide feedback.  Your comments all
look pretty reasonable at first glance.  I'll go through them in more
detail tonight and rework the patches.

Below are a couple of quick comments (note I'm not indicating that the
other comments you made are invalid or won't be responded to).

> I gather there is no way to deduce the VBI node, if any, from the video node,
> is there? I fear that most users will fail to enable VBI manually :-(

Unfortunately, the answer is no (and this is something I've SCREAMED
about for years now, with the only proposed solution being the
eventual adoption of the "media controller" framework).  Same goes for
identifying the ALSA device associated with a video node, for cases
where you're using a raw capture device instead of using an MPEG
encoder.

I'm actually going to check with Hans as to whether you can actually
just open the /dev/videoX device in VBI mode (and don't use /dev/vbiX
at all).

>> +        /* Do a single read and throw away the results so that ZVBI calls
>> +           the STREAMON ioctl() */
>> +        GrabVBI( VLC_OBJECT(demux), sys->vbi_cap, sys->vbi_sliced,
>> +                 sys->vbi_sliced_size, sys->vbi_raw );
>
> I'm not sure I understand. You want to force zvbi to start streaming before
> poll()? Shouldn't that be done in Open()?

The issue here is that some drivers don't even start the streaming
until the first read() call occurs (and hence the poll() call *never*
returns the descriptor as having data for read).  I will see if there
is a better way to get the streaming started.

> So my first thought was, you don't need this #ifdef because any system with V4L
> has fcntl(). But then I realized the code would not quite work anyway.
>
> The VBI node needs to be open()ed with the O_CLOEXEC flag set. Otherwise, the
> file descriptor can be leaked into another forking thread, before the fcntl()
> system call. I had the same problem with ALSA-lib already; I sent a patch.
>
> An alternative is to open the file descriptor in VLC and pass the descriptor to
> the library, as with libv4l. I doubt libzvbi supports that though.

Agreed, being able to do the open with O_CLOEXEC would have been more
ideal, except as you noted the libzvbi doesn't support taking a file
descriptor as an input).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com