[FFmpeg-devel] Fwd: framebuffer device demuxer

[Stefano Sabatini]

On date Friday 2011-01-28 01:10:46 +0000, M?ns Rullg?rd encoded:
> Sorry to chip in so late, but here are some comments on this, mostly
> minor.
> 
> Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
> 
> > From 9059a39f16131efc768d59784bd0179a2dfce69d Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > Date: Tue, 25 Jan 2011 19:40:29 +0100
> > Subject: [PATCH] Add Linux framebuffer device.
> >
> > Based on a patch by Giliard B. de Freitas /com/gmail/giliarde.
> >
> > See thread:
> > Subject: [FFmpeg-devel] Fwd: framebuffer device demuxer
> > Date: Sat, 23 May 2009 09:32:13 -0300
> > ---
> >  Changelog                 |    1 +
> >  configure                 |    2 +
> >  doc/indevs.texi           |   18 ++++
> >  libavdevice/Makefile      |    1 +
> >  libavdevice/alldevices.c  |    1 +
> >  libavdevice/framebuffer.c |  216 +++++++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 239 insertions(+), 0 deletions(-)
> >  create mode 100644 libavdevice/framebuffer.c
[...]
> > + at section framebuffer
> > +
> > +Linux framebuffer input device.
> > +
> > +The name of the framebuffer device to read is a file device node,
> > +usually @file{/dev/fb0}.
> > +
> > +For example, to record from the framebuffer device /dev/fb0 with
> > + at file{ffmpeg}:
> > + at example
> > +ffmpeg -f framebuffer -r 10 -i /dev/fb0 out.avi
> > + at end example
> > +
> > +You can take a single screenshot image with the command:
> > + at example
> > +ffmpeg -f framebuffer -vframes 1 -r 1 -i /dev/fb0 screenshot.jpeg
> > + at end example
> > +
> >  @section jack
> >  
> >  JACK input device.
> 
> I think "framebuffer" is a bit too generic a name.  After all,
> anything with a display of any kind has a framebuffer.  The Linux
> framebuffer device generally goes by the name "fbdev", so perhaps that
> would be a better name.

Replaced with linuxfb, this should be more explicative of the fact
that it is a linux thing, this should prevent having users ask "I'm on
windows, why can't I find the fbdev device?".

[...]
> > +/**
> > + * Initialize the framebuffer input device.
> > + */
> > +av_cold static int framebuffer_read_header(AVFormatContext *avctx, AVFormatParameters *ap)
> > +{
> > +    FrameBufferContext *fb = avctx->priv_data;
> > +    struct fb_var_screeninfo fb_varinfo;
> > +    struct fb_fix_screeninfo fb_fixinfo;
> > +    AVStream *st = NULL;
> > +    enum PixelFormat pix_fmt;
> > +    int ret, bits_per_pixel, flags = O_RDONLY;
> > +
> > +    if (!(st = av_new_stream(avctx, 0)))
> > +        return AVERROR(ENOMEM);
> > +
> > +    if (avctx->flags & AVFMT_FLAG_NONBLOCK)
> > +        flags |= O_NONBLOCK;
> > +
> > +    if ((fb->fd = open(avctx->filename, flags)) == -1) {
> > +        ret = AVERROR(errno);
> > +        av_log(avctx, AV_LOG_ERROR, "Could not open framebuffer device '%s'.\n", avctx->filename);
> > +        goto fail;
> 
> You could return directly here and drop the check before close().

Done.

[...]
> > +    fb->width  = fb_varinfo.xres;
> > +    fb->height = fb_varinfo.yres;
> > +    bits_per_pixel = fb_varinfo.bits_per_pixel;
> > +
> > +    switch (bits_per_pixel) {
> > +    case  8: pix_fmt = PIX_FMT_PAL8  ; break;
> > +    case 15: pix_fmt = PIX_FMT_BGR555; break;
> > +    case 16: pix_fmt = PIX_FMT_RGB565; break;
> > +    case 24: pix_fmt = PIX_FMT_RGB24 ; break;
> > +    case 32: pix_fmt = PIX_FMT_RGB32 ; break;
> > +    default:
> > +        ret = AVERROR(EINVAL);
> > +        av_log(avctx, AV_LOG_ERROR, "Image with bits per pixel depth %d not supported.\n",
> > +               bits_per_pixel);
> > +        goto fail;
> > +    }
> 
> These are not the only formats possible.  You cannot use bits per
> pixel exclusively to determine the format.

Yes indeed. I'm using a far more robust solution now, yet incomplete,
also I have to see how to handle pal formats.

[...]
> > +static int framebuffer_read_packet(AVFormatContext *avctx, AVPacket *pkt)
> > +{
> > +    FrameBufferContext *fb = avctx->priv_data;
> > +    int64_t curtime, delay;
> > +    struct timespec ts;
> > +    int i, ret;
> > +    uint8_t *pin, *pout;
> > +
> > +    /* wait based on the frame rate */
> > +    while (1) {
> > +        curtime = av_gettime();
> > +        delay = curtime - fb->time_frame * av_q2d(fb->time_base);
> > +        if (delay >= 0) {
> > +            fb->time_frame += INT64_C(1000000) * av_q2d(fb->time_base);
> > +            break;
> > +        }
> > +        if (avctx->flags & AVFMT_FLAG_NONBLOCK)
> > +            return AVERROR(EAGAIN);
> > +        ts.tv_sec  =  delay / 1000000;
> > +        ts.tv_nsec = (delay % 1000000) * 1000;
> > +        nanosleep(&ts, NULL);
> > +    }
> 
> Using e.g. sem_timedwait(), which takes an absolute timeout, might
> give better accuracy here, at least on the off-chance that the process
> is descheduled between the av_gettime() and nanosleep() calls.

I confess that I have not much experience with this, attaching this
change as a second patch for easier review.

> > +    if ((ret = av_new_packet(pkt, fb->frame_size)) < 0)
> > +        return ret;
> > +
> > +    pkt->pts = curtime;
> > +    pin  = fb->data;
> > +    pout = pkt->data;
> > +    for (i = 0; i < fb->height; i++) {
> > +        memcpy(pout, pin, fb->frame_linesize);
> > +        pin  += fb->linesize;
> > +        pout += fb->frame_linesize;
> > +    }
> > +
> > +    return fb->frame_size;
> > +}
> > +
> > +/**
> > + * Close framebuffer input device.
> > + *
> > + * @return a negative value in case of errors, 0 otherwise
> 
> This comment is a lie.  The function always returns 0.  Not that much
> could go wrong...

I removed all the internal documentation, since it looked redundant
(and internal documentation tends to get outdated very easily).

[...]
> > +AVInputFormat framebuffer_demuxer = {
> > +    .name           = "framebuffer",
> > +    .long_name      = NULL_IF_CONFIG_SMALL("Linux framebuffer"),
> > +    .priv_data_size = sizeof(FrameBufferContext),
> > +    .read_header    = framebuffer_read_header,
> > +    .read_packet    = framebuffer_read_packet,
> > +    .read_close     = framebuffer_read_close,
> > +    .flags          = AVFMT_NOFILE,
> > +};
> 
> Is AVFMT_NOFILE really correct here?  I'm not sure what its semantics
> are supposed to be.

It's not explained in the docs, but AVFMT_NOFILE is used for all the
input and output device (possibly is what distinguishes a device from
a muxer/demuxer).

And I'm still observing the 100% CPU problem noted by Luca A. in this
thread, a 0-memcpy solution is possible but it requires some change to
the framework (as the written packet contains aligned data, which is
not supported ATM).
-- 
FFmpeg = Frenzy Fancy Minimal Powerful Extensive Generator