[FFmpeg-devel] [PATCH 2/2] lavd: add v4l2 outdev.

[Clément Bœsch]

On Mon, May 20, 2013 at 12:37:20AM +0200, Stefano Sabatini wrote:
[...]
> > +struct video_data {
> > +    int fd;
> > +};
> 
> Silly name? V4L2Context seems more sensible.
> 

I was just making it consistent with v4l2.c. Changed to V4L2Context since
I don't care much.

> > +
> > +static av_cold int write_header(AVFormatContext *s1)
> > +{
> > +    int res = 0, flags = O_RDWR;
> > +    struct v4l2_format fmt = {
> > +        .type = V4L2_BUF_TYPE_VIDEO_OUTPUT
> > +    };
> > +    struct video_data *s = s1->priv_data;
> 
> > +    AVCodecContext *enc;
> 
> nit++: enc_ctx?
> 

Sure, whatever, renamed.

> > +    uint32_t v4l2_pixfmt;
> > +
> > +    if (s1->flags & AVFMT_FLAG_NONBLOCK)
> > +        flags |= O_NONBLOCK;
> > +
> > +    s->fd = open(s1->filename, flags);
> > +    if (s->fd < 0) {
> 
> > +        av_log(s1, AV_LOG_ERROR, "Unable to open V4L2 device '%s'.\n", s1->filename);
> 
> Nit: no trailing "."
> 

Removed. Again, that was for consistency with some other messages.

> Also:
> 
>     if (s->fd < 0) {
>        ret = AVERROR(errno); 
>        av_log(s1, AV_LOG_ERROR, "Unable to open V4L2 device '%s');
>        return ret;
>     }
> 
> since av_log might change errno.
> 

Good point, fixed.

[...]
> > +AVOutputFormat ff_v4l2_muxer = {
> > +    .name           = "v4l2",
> 
> > +    .long_name      = NULL_IF_CONFIG_SMALL("Video4Linux2 device push"),
> 
> "push"?
> 

Input device uses "grab", I just flipped that word. Changed to
"Video4Linux2 output device".

> Some docs in outdevs.texi with an example may be usefull as well.
> 

Well I'm not sure about the documentation; there is really not a single
option yet. Though, doc/general.texi needed some update, which I did fix.

Maybe we can add an example with v4l2loopback eventually. But actually, it
might be relevant to make that project reference v4l2 pushing with FFmpeg
first.

> LGTM otherwise, thanks.

Applied, thanks for the fast review,

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130520/23292fba/attachment.asc>