Re: [RFC] [media] v4l2: add V4L2 pixel format array and helper functions

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Philipp,

On Monday 25 August 2014 17:41:17 Philipp Zabel wrote:
> Am Montag, den 25.08.2014, 17:13 +0200 schrieb Laurent Pinchart:
> [...]
> 
> >>>> +static inline unsigned int v4l2_sizeimage(const struct v4l2_pixfmt
> >>>> *fmt,
> >>>> +					  unsigned int width,
> >>>> +					  unsigned int height)
> >>>> +{
> >>>> +	return width * height * fmt->bpp_image / 8;
> >>> 
> >>> Ditto: return height * v4l2_bytesperline(fmt, width);
> >> 
> >> I can't use v4l2_bytesperline because that might be zero for macroblock
> >> tiled formats and uses the wrong bpp value for planar formats with
> >> subsampled chroma.
> >> 
> >> This nearly works:
> >>     return height * DIV_ROUND_UP(width * fmt->bpp_image, 8)
> > 
> > Isn't that exactly height * v4l2_bytesperline(fmt, width) ?
> 
> Only if bpp_image == bpp_line ...

Oops, I've missed that subtle difference, sorry.

> >> For the planar 4:2:0 subsampled formats and Y41P (bpp_image == 12),
> >> width has to be even, so that is ok.
> >> Most other formats have a bpp_image that is divisible by 8, but there
> >> are the 4:1:0 subsampled formats (bpp_image == 9). Those would round up
> >> width to a multiple of eight, even though it only has to be a multiple
> >> 
> >> of four. I'd fall back to:
> >>     if (fmt->bpp_image == fmt->bpp_line) {
> >>     
> >>         return height * DIV_ROUND_UP(width * fmt->bpp_image, 8);
> 
> ... as is the case here. I'll use v4l2_bytesperline, then.
> 
> >>     } else {
> >>     
> >>         /* we know that v4l2_bytesperline doesn't round for planar */
> >>         return height * width * fmt->bpp_image / 8;
> >>     
> >>     }
> > 
> > Isn't that growing slightly too big for an inline function ?
> 
> Yes, I think this is slightly over the edge. Is room for a function to
> accompany the preexisting v4l2_fill_pix_format (say,
> v4l2_fill_pix_format_size) to set both the bytesperline and sizeimage
> values in a struct v4l2_pix_format?

That sounds sensible to me, provided it would be used by drivers of course. I 
wouldn't remove v4l2_bytesperline() and v4l2_sizeimage(), as the values might 
be needed by drivers in places where a v4l2_pix_format structure isn't 
available.

> Also, is anybody bothered by the v4l2_pix_format / v4l2_pixfmt
> similarity in name?

How about renaming v4l2_pixfmt to v4l2_pix_format_info ?

-- 
Regards,

Laurent Pinchart