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

[Paul Elder]

Hi Laurent,

Thank you for the review.

On Tue, Jun 30, 2020 at 12:44:04AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Tue, Jun 30, 2020 at 12:14:07AM +0900, Paul Elder wrote:
> > Packed formats make it difficult to calculate bytes-per-line as well as
> > buffer size with the fields that PixelFormatInfo currently has.
> > bitsPerPixel is defined as the average number of bits per pixel, and
> > only counts effective bits, so it is not useful for calculating
> > bytes-per-line and bytesused.
> 
> I know there are not introduced in this patch, but I think we should
> standardize on wording through libcamera for the V4L2 bytesperline,
> bytesused and sizeimage concepts.

Yes.

> For bytesperline, I propose line stride, which can shorten to stride in
> contexts where this isn't ambiguous. The corresponding variables or
> functions would be lineStride or stride.

I agree.

> For sizeimage, I propose frame size or frame length (we usually talk
> about frames rather than images).

I think this is fine too. Or frame buffer size for more explicitness?

> For bytesused I'm more undecided, I know I don't like the name much (but
> maybe I'm biased by too much exposure to V4L2 :-)), and I'm thinking
> about payload size. Payload length doesn't sound very good for some
> reason, which makes me think that we should use frame size instead of
> frame length. Better options are welcome.

Payload size should be fine. It's the number of effective bytes.

> > To fix this, we introduce a concept of a "pixel group". The size of this
> > group is defined as the minimum number of pixels (including padding)
> > necessary in a row when the image has only one column of effective
> > pixels.
> 
> Is that right ?

It should be.

> Thinking about SBGGR12_CSI2P for instance, you set
> pixelsPerGroup to 2 and bytesPerGroup to 3, which seems correct to me,
> but is that really one column ?

If you have only one column of pixels, then you still need one B and one
G, as well as the third byte for the low bits. For the non-packed
formats I suppose it's ambiguous (maybe just B and no G is fine?), but
in the packed ones clearly you need the G to align the low bits in the
third byte.

> Maybe we should define the group as the
> minimum number of pixels that are stored in an integer number of bytes ?

As you've noticed later, no :)

> It should also be explicitly defined as applying in the horizontal
> direction only.

Yes. I thought I did:
> > necessary in a *row* when the image has only one column of effective

> Should we also mention that these values apply before
> any horizontal downsampling ?

I was thinking that the horizontal downsampling influences the
parameters of the pixel group for the format.

> > The pixel group has one more attribute, that is the "bytes per
> > group". This determines how many bytes one pixel group consumes. These
> > are the fields pixelsPerGroup and bytesPerGroup that are defined in this
> > patch. Defining these two values makes it really simple to calculate
> > bytes-per-line, as ceil(width / pixelsPerGroup) * bytesPerGroup, where
> > width is measured in number of pixels. The ceiling accounts for padding.
> > 
> > For example, for something simple like BGR888, it is self-explanatory:
> > the pixel group size is 1, and the bytes necessary is 3. For YUYV, the
> > CbCr pair is shared between two pixels, so even if you have only one
> > pixel, you would still need a padded second Y, therefore the pixel
> > group size is 2, and bytes necessary is 4 (as opposed to 1 and 2).
> 
> And this invalidates my definition proposal above :-) Maybe we could
> keep your definition based on "columns", but we would then need to
> define what a column is.

I thought it was sufficient to say one column of *effective pixels*.
Like, for the IPU3 formats if you have one column of effective pixels,
you'll still need 49 more columns of padding pixels. Obviously irl
you're not going to have a frame size of 1xH, so this was a theoretical
definition.

> Maybe it could be defined somehow based on the
> concept of repeating pattern.

I was thinking that, but I couldn't find a concise and precise way to
word a definition :/

> pixelsPerColumn and bytesPerColumn may
> then be alternative names for the fields.

That doesn't quite work either, since these aren't strictly per column.
For example, for YUYV, if you have one column of effective pixels,
you'll have two total columns, where the second is padding. But if you
have two columns of effective pixels, you'll still only have two total
columns.

> > NV12 seems like it shold be 6 bytes with 4 pixels, since there is
> > downsampling in the Y axis as well, however, the pixel group is only
> > in terms of rows, so it is half of that, at 2 pixels and 3 bytes. The
> > IPU3 formats are also self-explanatory, coming from a comment in the
> > driver, a pixel group is 50, and it consumes 64 bytes.
> 
> This also invalidates my definition as we would then have 25 and 32.
> 
> What bothers me a tiny bit with the IPU3 raw format here is that we're
> mixing two concepts, the natural alignment required by the format (which
> would lead to 25/32 here), and the alignment required by the IPU3 (which
> is 64 bytes). It could be entirely conceivable that a DMA engine that
> writes YUYV data would require a 16, 32 or 64 bytes alignment. This is
> something that can only be known by the corresponding pipeline handler
> (possibly getting the information from the kernel driver). I'd thus be
> tempted to go for 25/32 for the IPU3 formats, and handle the additional
> alignment constraint in the pipeline handler, even if we know that in
> practice the format will only be used by the IPU3 hardware.

Oh, shoot, yeah, that's a problem :/

I would totally go for adding another field for alignment. In that case
it is indeed more correct to have the IPU3 pipeline handler to fill it
out.

> What does everybody think ?
> 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  include/libcamera/internal/formats.h |  4 +-
> >  src/libcamera/formats.cpp            | 95 +++++++++++++++++++++++++++-
> >  2 files changed, 97 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > index f59ac8f..dc19492 100644
> > --- a/include/libcamera/internal/formats.h
> > +++ b/include/libcamera/internal/formats.h
> > @@ -45,13 +45,15 @@ public:
> >  
> >  	static const PixelFormatInfo &info(const PixelFormat &format);
> >  
> > -	/* \todo Add support for non-contiguous memory planes */
> >  	const char *name;
> >  	PixelFormat format;
> >  	V4L2PixelFormat v4l2Format;
> >  	unsigned int bitsPerPixel;
> >  	enum ColourEncoding colourEncoding;
> >  	bool packed;
> > +
> > +	unsigned int bytesPerGroup;
> > +	unsigned int pixelsPerGroup;
> 
> Missing documentation :-) Didn't doxygen warn you ?

Oops, yes :p

I got used to ignoring doxygen's warnings because it always warns about
missing docs in camera and controls...

> I would also swap the two fields, I think it's more natural to say that
> 2 pixels are stored in 3 bytes than saying that 3 bytes store 2 pixels.

Yeah, that's probably better.

> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index d3b722c..88b5168 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -179,6 +179,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 24,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::RGB888, {
> >  		.name = "RGB888",
> > @@ -187,6 +189,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 24,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::ABGR8888, {
> >  		.name = "ABGR8888",
> > @@ -195,6 +199,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 32,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::ARGB8888, {
> >  		.name = "ARGB8888",
> > @@ -203,6 +209,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 32,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::BGRA8888, {
> >  		.name = "BGRA8888",
> > @@ -211,6 +219,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 32,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::RGBA8888, {
> >  		.name = "RGBA8888",
> > @@ -219,6 +229,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 32,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  
> >  	/* YUV packed formats. */
> > @@ -229,6 +241,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::YVYU, {
> >  		.name = "YVYU",
> > @@ -237,6 +251,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::UYVY, {
> >  		.name = "UYVY",
> > @@ -245,6 +261,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::VYUY, {
> >  		.name = "VYUY",
> > @@ -253,6 +271,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  
> >  	/* YUV planar formats. */
> > @@ -263,6 +283,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> 
> Shouldn't this be 2/2, as in a Y line, every pixel is stored in 1 byte ?
> Same for the NV16/61 formats. NV24/42 would be 1/1. We'll need an
> additional field to handle multiplanar formats. Or maybe the fields are
> meant to average all planes ?

Yes, I meant to average the planes. Well, averaged in the Y axis. In the
X axis it's as usual. Since we're only using these fields to calculate
stride and frame size, I think it should be sufficient.

> In that case this should be defined
> explicitly, but the ceil(width / pixelsPerGroup) * bytesPerGroup
> calculation you mention in the commit message wouldn't be correct
> anymore.

Hm? I thought it works. What's wrong with it?

> I'm tempted to say that the values should apply to the first
> plane only, and add other fields to address the other planes (or maybe
> we need to replicate those two fields per plane ?).
> 
> >  	} },
> >  	{ formats::NV21, {
> >  		.name = "NV21",
> > @@ -271,6 +293,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::NV16, {
> >  		.name = "NV16",
> > @@ -279,6 +303,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::NV61, {
> >  		.name = "NV61",
> > @@ -287,6 +313,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::NV24, {
> >  		.name = "NV24",
> > @@ -295,6 +323,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 24,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::NV42, {
> >  		.name = "NV42",
> > @@ -303,6 +333,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 24,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  	{ formats::YUV420, {
> >  		.name = "YUV420",
> > @@ -311,6 +343,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> 
> This should be addresses similarly based on the outcome of the
> discussion regarding NV formats.
> 
> >  	} },
> >  	{ formats::YUV422, {
> >  		.name = "YUV422",
> > @@ -319,6 +353,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 16,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  
> >  	/* Greyscale formats. */
> > @@ -329,6 +365,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 8,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> >  		.packed = false,
> > +		.bytesPerGroup = 1,
> > +		.pixelsPerGroup = 1,
> >  	} },
> >  
> >  	/* Bayer formats. */
> > @@ -339,6 +377,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 8,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 2,
> > +		.pixelsPerGroup = 2,
> 
> For 8-bit Bayer formats, technically, 1/1 could work, but I suppose it
> would make little sense. I think a word about Bayer formats in the
> commit message (and in the documentation, which should copy a large part
> of the commit message) would be useful.

Okay.

> >  	} },
> >  	{ formats::SGBRG8, {
> >  		.name = "SGBRG8",
> > @@ -347,6 +387,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 8,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 2,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGRBG8, {
> >  		.name = "SGRBG8",
> > @@ -355,6 +397,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 8,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 2,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SRGGB8, {
> >  		.name = "SRGGB8",
> > @@ -363,6 +407,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 8,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 2,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SBGGR10, {
> >  		.name = "SBGGR10",
> > @@ -371,6 +417,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGBRG10, {
> >  		.name = "SGBRG10",
> > @@ -379,6 +427,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGRBG10, {
> >  		.name = "SGRBG10",
> > @@ -387,6 +437,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SRGGB10, {
> >  		.name = "SRGGB10",
> > @@ -395,6 +447,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SBGGR10_CSI2P, {
> >  		.name = "SBGGR10_CSI2P",
> > @@ -403,6 +457,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 5,
> > +		.pixelsPerGroup = 4,
> >  	} },
> >  	{ formats::SGBRG10_CSI2P, {
> >  		.name = "SGBRG10_CSI2P",
> > @@ -411,6 +467,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 5,
> > +		.pixelsPerGroup = 4,
> >  	} },
> >  	{ formats::SGRBG10_CSI2P, {
> >  		.name = "SGRBG10_CSI2P",
> > @@ -419,6 +477,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 5,
> > +		.pixelsPerGroup = 4,
> >  	} },
> >  	{ formats::SRGGB10_CSI2P, {
> >  		.name = "SRGGB10_CSI2P",
> > @@ -427,6 +487,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 5,
> > +		.pixelsPerGroup = 4,
> >  	} },
> >  	{ formats::SBGGR12, {
> >  		.name = "SBGGR12",
> > @@ -435,6 +497,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGBRG12, {
> >  		.name = "SGBRG12",
> > @@ -443,6 +507,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGRBG12, {
> >  		.name = "SGRBG12",
> > @@ -451,6 +517,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SRGGB12, {
> >  		.name = "SRGGB12",
> > @@ -459,6 +527,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = false,
> > +		.bytesPerGroup = 4,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SBGGR12_CSI2P, {
> >  		.name = "SBGGR12_CSI2P",
> > @@ -467,6 +537,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGBRG12_CSI2P, {
> >  		.name = "SGBRG12_CSI2P",
> > @@ -475,6 +547,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SGRBG12_CSI2P, {
> >  		.name = "SGRBG12_CSI2P",
> > @@ -483,6 +557,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SRGGB12_CSI2P, {
> >  		.name = "SRGGB12_CSI2P",
> > @@ -491,6 +567,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 12,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 3,
> > +		.pixelsPerGroup = 2,
> >  	} },
> >  	{ formats::SBGGR10_IPU3, {
> >  		.name = "SBGGR10_IPU3",
> > @@ -499,6 +577,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 64,
> > +		.pixelsPerGroup = 50,
> >  	} },
> >  	{ formats::SGBRG10_IPU3, {
> >  		.name = "SGBRG10_IPU3",
> > @@ -507,6 +587,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 64,
> > +		.pixelsPerGroup = 50,
> >  	} },
> >  	{ formats::SGRBG10_IPU3, {
> >  		.name = "SGRBG10_IPU3",
> > @@ -515,6 +597,8 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 64,
> > +		.pixelsPerGroup = 50,
> >  	} },
> >  	{ formats::SRGGB10_IPU3, {
> >  		.name = "SRGGB10_IPU3",
> > @@ -523,16 +607,25 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.bitsPerPixel = 10,
> >  		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >  		.packed = true,
> > +		.bytesPerGroup = 64,
> > +		.pixelsPerGroup = 50,
> >  	} },
> >  
> >  	/* Compressed formats. */
> > +	/*
> > +	 * \todo Get a better image size estimate for MJPEG, via
> > +	 * StreamConfiguration, instead of using the worst-case
> > +	 * width * height * bpp of uncompressed data.
> > +	 */
> 
> I'm not sure this comment belongs here.

I don't think it does.

> >  	{ 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?

> >  	} },
> >  };
> >  


Thanks,

Paul