[FFmpeg-devel] Fwd: framebuffer device demuxer

[Stefano Sabatini]

On date Wednesday 2011-01-26 23:18:38 +0100, Stefano Sabatini encoded:
> On date Wednesday 2011-01-26 10:55:56 +0100, Diego Biurrun encoded:
> > On Tue, Jan 25, 2011 at 11:47:35PM +0100, Stefano Sabatini wrote:
> > > 
> > > Not yet for commit, I want to clean it up still a bit (and maybe add
> > > some more features), review is welcome of course.
> > > 
> > > --- a/configure
> > > +++ b/configure
> > > @@ -2850,6 +2851,7 @@ fi
> > >  
> > >  check_header linux/videodev.h
> > >  check_header linux/videodev2.h
> > > +check_header linux/fb.h
> > >  check_header sys/videoio.h
> > 
> > alphabetical order
> > 
> > > --- a/doc/indevs.texi
> > > +++ b/doc/indevs.texi
> > > @@ -59,6 +59,16 @@ BSD video input device.
> > >  
> > >  Linux DV 1394 input device.
> > >  
> > > + at section framebuffer
> > > +
> > > +Linux frame buffer input device.
> > 
> > framebuffer
> > 
> > > +For example, to record from the frame buffer device /dev/fb0 with
> > 
> > ditto
> > 
> > > --- /dev/null
> > > +++ b/libavdevice/framebuffer.c
> > > @@ -0,0 +1,220 @@
> > > +
> > > +/**
> > > + * @file
> > > + * Linux Frame Buffer input device
> > 
> > framebuffer
> > 
> > > + * Based on code from fbgrab.c by Gunnar Monell.
> > 
> > What license was that code?
> 
> GPL, I concluded that there is not enough code in common to impose the
> use of GPL, though I have no problem at re-licensing the file under
> GPL if someone insists on this.
> 
> BTW fbgrab.c is bugged.
> 
> > > +#define DEBUG
> > > +
> > > +#include <unistd.h>
> > > +#include <fcntl.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/time.h>
> > > +#include <sys/mman.h>
> > > +#include <time.h>
> > > +#include <linux/fb.h>
> > > +#include "libavutil/mem.h"
> > 
> > extra good karma for an empty line between local and system headers
> > 
> > > +#include "libavutil/pixdesc.h"
> > > +#include "libavformat/avformat.h"
> > > +#define _LINUX_TIME_H 1
> > 
> > Where does this #define have an effect if you place it after #includes?
> 
> Looks useless, removed.
> 
> > 
> > > +av_cold static int frame_buffer_read_header(AVFormatContext *avctx, AVFormatParameters *ap)
> > > +{
> > > +    struct fb_var_screeninfo fb_varinfo;
> > > +
> > > +    memset(&fb_varinfo, 0, sizeof(struct fb_var_screeninfo));
> > 
> > Is the nulling necessary?  Simpler should be
> 
> Good catch, removed.
> 
> >   struct fb_var_screeninfo fb_varinfo = { 0 };
> > 
> > > +    if (ioctl(fb_ctx->fd, FBIOGET_VSCREENINFO, &fb_varinfo) < 0){
> > > +
> > > +    if (ioctl(fb_ctx->fd, FBIOGET_FSCREENINFO, &fb_fixinfo) < 0){
> > 
> > ) {
> > 
> > > +    fb_ctx->frame_size = fb_varinfo.xres * fb_varinfo.yres * ((bits_per_pixel+7)>>3);
> > > +    fb_ctx->linesize = fb_varinfo.xres * ((bits_per_pixel+7)>>3);
> > > +    fb_ctx->frame_linesize = fb_fixinfo.line_length;
> > > +    fb_ctx->time_base  = ap->time_base;
> > > +    fb_ctx->time_frame = av_gettime() / av_q2d(ap->time_base); /* timestamp in time_base unit */
> > > +    fb_ctx->data = mmap(NULL, fb_fixinfo.line_length * fb_varinfo.yres, PROT_READ, MAP_SHARED, fb_ctx->fd, 0);
> > 
> > Vertical alignment and spaces around operators could make this more
> > readable.
> > 
> > > +AVInputFormat framebuffer_demuxer = {
> > > +    .name           = "framebuffer",
> > > +    .long_name      = NULL_IF_CONFIG_SMALL("Linux Frame Buffer"),
> > 
> > framebuffer
> 
> All remaining nits fixed.
> 
> Luca A., can you have a look at it?
[...]
> +    fb->data = mmap(NULL, fb_fixinfo.line_length * fb_varinfo.yres,
> +                    PROT_READ, MAP_SHARED, fb->fd, 0);
> +    if (!fb->data) {

Autofix:
if ((int)fb->data == -1)
...

Updated.
-- 
FFmpeg = Fast & Funny Meaningless Prodigious Elastic Geek