Hi Gerd/Michael again,

some comments on the current API:

> #define V4L2_MAJOR_VERSION    0
> #define V4L2_MINOR_VERSION    20

<nitpicking> increase this to 21 please. ;-). </nitpicking>

> enum v4l2_buf_type {
>       V4L2_BUF_TYPE_VIDEO_CAPTURE  = 1,
>       V4L2_BUF_TYPE_VIDEO_OUTPUT   = 2,
>       V4L2_BUF_TYPE_VIDEO_OVERLAY  = 3,
>       V4L2_BUF_TYPE_VBI_CAPTURE    = 4,
>       V4L2_BUF_TYPE_VBI_OUTPUT     = 5,
>       V4L2_BUF_TYPE_PRIVATE        = 0x80,
> };

In the old approach, there was a separate VIDEO_CODECIN/OUT, I kind of
understand why it was removed, but that makes some other parts a it
harder. 

> struct v4l2_fmtdesc
> {
>       __u32               index;             /* Format number      */
>       enum v4l2_buf_type  type;              /* buffer type        */
>       __u8                description[32];   /* Description string */
>       __u32               pixelformat;       /* Format fourcc      */
>       __u32               reserved[4];
> };

We see the type here, and the 'flag' field is removed too. Now, if I
understand correctly, we need to define separate picture formats for
input and output. That's fine, though I actually expected a flag instead
of a type field for this (i.e., flag the thing as input-only, or
input|output, etc.). That's fine, now let's get back to the
compressed/codec thing. For gstreamer, we actually used this field to
define the mime type of the stream: if flag&compressed, mimetype =
sprintf("video/%4.4s", fourcc), else, mimetype="video/raw",type=fourcc.
I think this was particularly useful. therefore, I'd propose to change
this into:

#define V4L2_FMT_FLAG_COMPRESSED 0x0001

struct v4l2_fmtdesc
{
        __u32               index;             /* Format number      */
        enum v4l2_buf_type  type;              /* buffer type        */
        __u32               flags;
        __u8                description[32];   /* Description string */
        __u32               pixelformat;       /* Format fourcc      */
        __u32               reserved[4];
};

There's probably more flags that can be useful, but this flag was
particularly useful in gstreamer. It makes video4linux useful for both
compressed capture as well as non-compressed, since we can distinguish
the two, and we actually *need* to (in the application). With this, we
could write a universal v4l2 element that supports any format that might
ever be there... Now, that's much harder.

The other solution (which I like too, but you probably don't ;-) ) is to
add CODECIN/OUT as v4l2_buf_type, like it as in the old v4l2.

> struct v4l2_capability
> {
>       __u8    name[32];       /* Descriptive, and unique */
>       __u32   capabilities;   /* Device capabilities */
>       __u32   flags;          /* Feature flags, see below */
>       __u32   reserved[4];
> };
> 
> /* Values for 'capabilities' field */
> #define V4L2_CAP_VIDEO_CAPTURE        0x00000001  /* Is a video capture device */
> #define V4L2_CAP_VIDEO_OUTPUT 0x00000002  /* Is a video output device */
> #define V4L2_CAP_VIDEO_OVERLAY        0x00000004  /* Can do video overlay */
> #define V4L2_CAP_VBI_CAPTURE  0x00000010  /* Is a VBI capture device */
> #define V4L2_CAP_VBI_OUTPUT   0x00000020  /* Is a VBI output device */
> #define V4L2_CAP_RDS_CAPTURE  0x00000100  /* RDS data capture */
> 
> #define V4L2_CAP_TUNER                0x00010000  /* Has a tuner */

I'd like to see V4L2_CAP_AUDIO be added here, is that possible?

> #if 0
> /* ### generic compression settings don't work, there is too much
>  * ### codec-specific stuff.  Maybe reuse that for MPEG codec settings
>  * ### later ... */
> struct v4l2_compression

Just rename it to v4l2_mpegcompression?

> struct v4l2_window
> {
>       struct v4l2_rect        w;
>       __u32                   chromakey;
>       struct v4l2_clip        *clips;
>       __u32                   clipcount;
>       void                    *bitmap;
> };

This is not right, w can't have unsigned x/y parameters, they *need* to
be signed because the window can be offscreen:

struct v4l2_window
{
        struct {
                __s32           x, y;
                __u32           width, height;
        } w;
        __u32                   chromakey;
        struct v4l2_clip        *clips;
        __u32                   clipcount;
        void                    *bitmap;
};

sounds better to me.

> 
> /*
>  *    C A P T U R E   P A R A M E T E R S
>  */
> struct v4l2_captureparm
> {
>       __u32              capability;    /*  Supported modes */
>       __u32              capturemode;   /*  Current mode */
>       struct v4l2_fract  timeperframe;  /*  Time per frame in .1us units */
>       __u32              extendedmode;  /*  Driver-specific extensions */
>       __u32              readbuffers;   /*  # of buffers for read */
>       __u32              reserved[4];
> };
> /*  Flags for 'capability' and 'capturemode' fields */
> #define V4L2_MODE_HIGHQUALITY 0x0001  /*  High quality imaging mode */
> #define V4L2_CAP_TIMEPERFRAME 0x1000  /*  timeperframe field is supported */
> 
> struct v4l2_outputparm
> {
>       __u32              capability;   /*  Supported modes */
>       __u32              outputmode;   /*  Current mode */
>       struct v4l2_fract  timeperframe; /*  Time per frame in seconds */
>       __u32              extendedmode; /*  Driver-specific extensions */
>       __u32              writebuffers; /*  # of buffers for write */
>       __u32              reserved[4];
> };

I must have missed something, what's this for?

> struct v4l2_crop {
>       enum v4l2_buf_type      type;
>       struct v4l2_rect        c;
> };

I'm seeing some problems for cropped capture. For cropping, you
basically first need to set the capture size/format, since cropping
depends on the format being used. But after that, if the application
sets cropping rectangles, the size changes. Is that how it's supposed to
work (and is the application then supposed to re-request size by G_FMT),
or is this indeed a problem?

> struct v4l2_enumstd
> {
>       __u32                   index;
>       struct v4l2_standard    std;
>       __u32                   inputset;  /* set of inputs that */
>                                          /* support this standard */
>       __u32                   outputset; /* set of outputs that */
>                                          /* support this standard */
>       __u32                   reserved[2];
> };

inputset/outputset is just the same as inputs/outputs, or are they  a
bitfield where (1<<x) means that input/output x supports this std?

I think this is all... I like some of your changes, as I've said before.
Let's hope this still will make it into 2.5/6.x.

Ronald



_______________________________________________
Video4linux-list mailing list
[EMAIL PROTECTED]
https://listman.redhat.com/mailman/listinfo/video4linux-list

Reply via email to