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

[Niklas Söderlund]

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?

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.

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 :-)

-- 
Regards,
Niklas Söderlund