Hi Hans,
thank you for your comments as always.
>Hans Verkuil wrote <hverkuil@xxxxxxxxx>:
>Hi Pawel,
>
>Looks good, but I have a few small suggestions:
>
>On Friday 09 July 2010 20:59:45 Pawel Osciak wrote:
(snip)
>> struct v4l2_format {
>> enum v4l2_buf_type type;
>> union {
>> struct v4l2_pix_format pix; /*
>V4L2_BUF_TYPE_VIDEO_CAPTURE */
>> + struct v4l2_pix_format_mplane mp_pix; /*
>V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
>
>I would probably go with pix_mp to be consistent with the name of the struct.
>
ok.
>> +/**
>> + * struct v4l2_pix_format_mplane - multiplanar format definition
>> + * @pix_fmt: definition of an image format for all planes
>> + * @plane_fmt: per-plane information
>> + */
>> +struct v4l2_pix_format_mplane {
>> + struct v4l2_pix_format pix_fmt;
>> + struct v4l2_plane_format plane_fmt[VIDEO_MAX_PLANES];
>> +} __attribute__ ((packed));
>
>How do you know how many planes there are? I wonder whether it wouldn't be
>smarter
>to just copy the relevant fields from struct v4l2_pix_format to struct
>v4l2_pix_format_mplane
>instead of embedded that struct. That way you can 1) add a 'planes' field and
>2) get rid
>of the no longer needed bytesperline and sizeimage fields. And I think the
>priv field
>should also go, just have a reserved[2] instead.
>
By mean "planes" you mean a field indicating the number of planes in the
current format, right?
Number of planes can be inferred from fourcc, but you are right, it's still
useful to have to have a field for that.
What do you think of this:
/**
* struct v4l2_pix_format_mplane - multiplanar format definition
* @width: image width in pixels
* @height: image height in pixels
* @pixelformat: little endian four character code (fourcc)
* @field: field order (for interlaced video)
* @colorspace: supplemental to pixelformat
* @plane_fmt: per-plane information
* @num_planes: number of planes for this format and number of valid
* elements in plane_fmt array
*/
struct v4l2_pix_format_mplane {
__u32 width;
__u32 height;
__u32 pixelformat;
enum v4l2_field field;
enum v4l2_colorspace colorspace;
struct v4l2_plane_format plane_fmt[VIDEO_MAX_PLANES];
__u8 num_planes;
__u8 reserved[11];
} __attribute__ ((packed));
v4l2_plane_format stays the same (see below).
8 * struct v4l2_plane_format + 3 * 4 + 2 * enum + 12 * 1 = 8 * 20 + 40 = 200
>> The plane format struct is as follows:
>>
>> +/**
>> + * struct v4l2_plane_format - additional, per-plane format definition
>> + * @sizeimage: maximum size in bytes required for data, for which
>> + * this plane will be used
>> + * @bytesperline: distance in bytes between the leftmost pixels in
>two
>> + * adjacent lines
>> + */
>> +struct v4l2_plane_format {
>> + __u32 sizeimage;
>> + __u16 bytesperline;
>> + __u16 reserved[7];
>> +} __attribute__ ((packed));
>>
>> Note that bytesperline is u16 now, but that shouldn't hurt.
>>
>>
>> Fitting everything into v4l2_format's union (which is 200 bytes long):
>> v4l2_pix_format shouldn't be larger than 40 bytes.
>> 8 * struct v4l2_plane_format + struct v4l2_pix_format = 8 * 20 + 40 = 200
>>
(snip)
>> 2. Plane description struct
>> ----------------------------------
>>
>> +/**
>> + * struct v4l2_plane - plane info for multiplanar buffers
>> + * @bytesused: number of bytes occupied by data in the plane (payload)
>> + * @mem_off: when memory in the associated struct v4l2_buffer is
>> + * V4L2_MEMORY_MMAP, equals the offset from the start of the
>> + * device memory for this plane (or is a "cookie" that should
>be
>> + * passed to mmap() called on the video node)
>> + * @userptr: when memory is V4L2_MEMORY_USERPTR, a userspace pointer
>pointing
>> + * to this plane
>> + * @length: size of this plane (NOT the payload) in bytes
>> + * @data_off: offset in plane to the start of data/end of header, if
>relevant
>> + *
>> + * Multi-plane buffers consist of two or more planes, e.g. an YCbCr buffer
>> + * with two planes has one plane for Y, and another for interleaved CbCr
>> + * components. Each plane can reside in a separate memory buffer, or in
>> + * a completely separate memory chip even (e.g. in embedded devices).
>> + */
>> +struct v4l2_plane {
>> + __u32 bytesused;
>> + __u32 length;
>> + union {
>> + __u32 mem_off;
>
>Rename to mem_offset. This prevents confusion since 'off' can also be
>interpreted as the opposite of 'on'.
>
>> + unsigned long userptr;
>> + } m;
>> + __u32 data_off;
>
>Rename to data_offset.
>
Ok to both.
Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
