[Ffmpeg-devel] [PATCH] video grab support VIDEO_PALETTE_RGB565

[Michael Niedermayer]

Hi

On Mon, Apr 30, 2007 at 07:30:24PM +0800, yi li wrote:
> I made some changes accordingly.  Hope it is better.
> 
> Regards,
> -Yi
> 
> On 4/29/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> >Hi
> >
> >On Sun, Apr 29, 2007 at 03:20:03PM +0800, yi li wrote:
> >> Hi,
> >>
> >> A small patch for libavformat/grab.c to make it support v4l device with
> >> "VIDEO_PALETTE_RGB565" format.
> >> Any comments?
> >>
> >> Regards,
> >>
> >> -Yi
> >
> >> --- grab.c.orig       2007-04-29 15:14:43.000000000 +0800
> >> +++ grab.c    2007-04-29 15:13:36.000000000 +0800
> >> @@ -174,8 +174,13 @@
> >>                      pict.palette=VIDEO_PALETTE_GREY;
> >>                      pict.depth=8;
> >>                      ret = ioctl(video_fd, VIDIOCSPICT, &pict);
> >> -                    if (ret < 0)
> >> -                        goto fail1;
> >> +                    if (ret < 0) {
> >> +                     pict.palette=VIDEO_PALETTE_RGB565;
> >> +                     pict.depth=16;
> >> +                     ret = ioctl(video_fd, VIDIOCSPICT, &pict);
> >> +                     if (ret < 0)
> >> +                             goto fail1;
> >> +                 }
> >>                  }
> >
> >tabs are forbidden in svn
> >rgb565 should be checked before gray
> >and ideally the whole should be simplified, a simple table with
> >VIDEO_PALETTE_*, depth, pix_fmt and frame_size factor could be used
[...]

> --- grab.c.orig	2007-04-30 13:52:26.000000000 +0800
> +++ grab.c	2007-04-30 19:21:38.000000000 +0800
> @@ -53,6 +53,19 @@
>      uint8_t *lum_m4_mem;
>  } VideoData;
>  
> +struct {
> +    int palette;
> +    int depth;
> +    enum PixelFormat pix_fmt; 
> +} video_formats [] = {
> +    {.palette = VIDEO_PALETTE_YUV420P, .depth = 12, .pix_fmt = PIX_FMT_YUV420P},
> +    {.palette = VIDEO_PALETTE_YUV422, .depth = 16, .pix_fmt = PIX_FMT_YUYV422},
> +    {.palette = VIDEO_PALETTE_RGB24, .depth = 24, 

trailing whitespace is forbidden in svn


> +        .pix_fmt = PIX_FMT_BGR24}, /* NOTE: v4l uses BGR24, not RGB24 ! */
> +    {.palette = VIDEO_PALETTE_RGB565, .depth = 16, .pix_fmt = PIX_FMT_BGR565},
> +    {.palette = VIDEO_PALETTE_GREY, .depth = 8, .pix_fmt = PIX_FMT_GRAY8}
> +};

this would be more readable if things where vertically aligned like:

{.palette = VIDEO_PALETTE_RGB565, .depth = 16, .pix_fmt = PIX_FMT_BGR565},
{.palette = VIDEO_PALETTE_GREY  , .depth =  8, .pix_fmt = PIX_FMT_GRAY8}

also i think the .palette, .depth, ... hurts readability more than it helps



[...]
> @@ -158,27 +172,15 @@
>      /* try to choose a suitable video format */
>      pict.palette = desired_palette;
>      pict.depth= desired_depth;
> -    if (desired_palette == -1 || (ret = ioctl(video_fd, VIDIOCSPICT, &pict)) < 0) {
> -        pict.palette=VIDEO_PALETTE_YUV420P;
> -        pict.depth=12;
> -        ret = ioctl(video_fd, VIDIOCSPICT, &pict);
> -        if (ret < 0) {
> -            pict.palette=VIDEO_PALETTE_YUV422;
> -            pict.depth=16;
> -            ret = ioctl(video_fd, VIDIOCSPICT, &pict);
> -            if (ret < 0) {
> -                pict.palette=VIDEO_PALETTE_RGB24;
> -                pict.depth=24;
> -                ret = ioctl(video_fd, VIDIOCSPICT, &pict);
> -                if (ret < 0) {
> -                    pict.palette=VIDEO_PALETTE_GREY;
> -                    pict.depth=8;
> -                    ret = ioctl(video_fd, VIDIOCSPICT, &pict);
> -                    if (ret < 0)
> -                        goto fail1;
> -                }
> -            }
> +    if (desired_palette == -1) {
> +        for (j = 0; j < vformat_num; j++) {
> +            pict.palette = video_formats[j].palette;
> +            pict.depth = video_formats[j].depth;
> +            if (-1 != ioctl(video_fd, VIDIOCSPICT, &pict))
> +                break;

i think this should rather stay <0 instead of ==-1


[...]
> @@ -248,27 +250,18 @@
>          s->frame_format = s->gb_buf.format;
>          s->use_mmap = 1;
>      }
> -
> -    switch(s->frame_format) {
> -    case VIDEO_PALETTE_YUV420P:
> -        frame_size = (width * height * 3) / 2;
> -        st->codec->pix_fmt = PIX_FMT_YUV420P;
> -        break;
> -    case VIDEO_PALETTE_YUV422:
> -        frame_size = width * height * 2;
> -        st->codec->pix_fmt = PIX_FMT_YUYV422;
> -        break;
> -    case VIDEO_PALETTE_RGB24:
> -        frame_size = width * height * 3;
> -        st->codec->pix_fmt = PIX_FMT_BGR24; /* NOTE: v4l uses BGR24, not RGB24 ! */
> -        break;
> -    case VIDEO_PALETTE_GREY:
> -        frame_size = width * height * 1;
> -        st->codec->pix_fmt = PIX_FMT_GRAY8;
> -        break;
> -    default:
> -        goto fail;
> +    
> +    for (j = 0; j < vformat_num; j++) {
> +        if (s->frame_format == video_formats[j].palette) {
> +            frame_size = width * height * video_formats[j].depth / sizeof(char);

sizeof(char) = 1


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070430/19d802ed/attachment.pgp>