[libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to info ease calculating bpl

[Laurent Pinchart]

On Wed, Jul 01, 2020 at 04:24:33PM +0200, Niklas Söderlund wrote:
> Hi Paul and Laurent,
> 
> On 2020-07-01 15:32:58 +0300, Laurent Pinchart wrote:
> 
> <snip>
> 
> > > > >  	{ formats::MJPEG, {
> > > > >  		.name = "MJPEG",
> > > > >  		.format = formats::MJPEG,
> > > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > > > -		.bitsPerPixel = 0,
> > > > > +		.bitsPerPixel = 16,
> > > > >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > > >  		.packed = false,
> > > > > +		.bytesPerGroup = 4,
> > > > > +		.pixelsPerGroup = 2,
> > > > 
> > > > I'd rather set these fields to 0 for MJPEG instead of faking them. JPEG
> > > > can store YUV 4:4:4, 4:2:2 or 4:2:0, so the values are not very
> > > > relevant. Pipeline handlers that can produce MJPEG (that's just UVC)
> > > > will need to set the frame size manually instead of using helpers.
> > > 
> > > Ah, okay. So... we'll still need check if format is MJPEG and then get
> > > the parameters from the pipeline handler instead? I think that'll messy
> > > up the code... But we want pipeline handlers to fill out the alignment
> > > parameter anyway, so maybe the pipeline handlers could set format info
> > > parameters?
> > 
> > They can't do that as the PixelFormatInfo instances are global (and
> > const). Pipeline handlers already report the stride through
> > StreamConfiguration::stride. I think we need an additional frameSize (or
> > frameLength ? or another name ?) field in there to report the total
> > size, which would be especially important for MJPEG.
> > 
> > Niklas, any opinion ?
> 
> Always :-)
> 
> First so that I understand you correctly frame{Size,Length} would be the 
> frame size in bytes without or without stride taken into account? Or 
> would it depend on the format? Which now that I write it sounds odd as 
> stride hos no real meaning for MJPEG. Put it another way if I where to 
> copy frame{Size,Length} bytes from the buffer I would be guaranteed to 
> have all have image data, but I still need to depending on format deal 
> with alignment or would it be the size of the data after alignment have 
> been dealt with?

If you copy frameSize bytes you will have all the data. For MJPEG that
will be a blob, and stride will be 0. For other formats it will be a set
of lines, each of them stride bytes long, thus including padding as
necessary.

We also need to report stride, size (and offset) per plane.

> I see no problem with a pipeline controlling such fields in the same way 
> stride is handled. It would be nice if somewhere in core a sanity check 
> could be added. If we take stride as an example, if its initialize to 
> zero and we expect the pipeline to updated it, it could be useful for 
> some core code to check that its != 0 before handing the structure to an 
> application. We can't guarantee the value is correct but we can warn if 
> a pipeline have not attempted to update it.

Good idea.

> My gut is telling me it's nicer to default to 0 then to the worst case 
> since that will force pipelines to do the right thing :-)

I agree.

-- 
Regards,

Laurent Pinchart