| Message ID | 20200629151411.216477-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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. 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. For sizeimage, I propose frame size or frame length (we usually talk about frames rather than images). 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. > 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 ? 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 ? Maybe we should define the group as the minimum number of pixels that are stored in an integer number of bytes ? It should also be explicitly defined as applying in the horizontal direction only. Should we also mention that these values apply before any horizontal downsampling ? > 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. Maybe it could be defined somehow based on the concept of repeating pattern. pixelsPerColumn and bytesPerColumn may then be alternative names for the fields. > 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. What does everybody think ? > Signed-off-by: Paul Elder <paul.elder@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 ? 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. > }; > > } /* 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 ? 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. 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. > } }, > { 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. > { 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. > } }, > }; >
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@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
<snip> On Tue, Jun 30, 2020 at 04:42:48PM +0900, Paul Elder wrote: > > > 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. Wait, I take that back. I put bytes first then pixels, because then you can say it's 3 bytes per 2 pixels. It's not a sentence, it's more like a unit of measurement. <snip> Paul
Hi Paul, I realized when reviewing v2 that we never completed this discussion. Let's get to the bottom of it to prepare for v3 :-) On Tue, Jun 30, 2020 at 04:42:48PM +0900, Paul Elder wrote: > On Tue, Jun 30, 2020 at 12:44:04AM +0300, Laurent Pinchart wrote: > > 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? The buffer could be allocated larger than the frame size, so I'd rather use frame size. > > 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. I don't dispute the correctness of your values, but I'm wondering how you define "column" in this context. Intuitively a column in an image would be one pixel wide for me, and that doesn't match the description you make here. It's just a matter of wording I think. Please see below for more on this topic (I had a 'aahhh' moment when reading your reply). > > 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 My bad. > > 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. We can discuss this below for the NV formats, but the idea is that multi-planar formats will downsample the chroma plane(s) only. There are multiple ways to express the necessary information, for instance with one pair of pixels + bytes per group per plane, or with explicit subsampling factors. I'm not sure yet which would be best. > > > 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. Aahhh I get it now. Re-reading your commit message it's pretty clear indeed, but for some reason I didn't get it initially. > > 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. I noticed you haven't changed that in v2, not sure if it was intentional. Pipeline handlers can't fill their alignment requirements in PixelFormatInfo, as we have a global const map of pixel format info. They thus need to either apply their constraints manually, or pass the constraints to the PixelFormatInfo::bytesPerLine() function. The most common type of constraint is an alignment in bytes, so you could add a parameter for that, with a default value of 1. Now, if a device has alignment constraints expressed in pixels, that wouldn't work anymore, and having multiple arguments to the function to specify different types of alignment constraints would quickly make it difficult to use. It may be better to leave it to the pipeline handler entirely. Because of this, we also need to pass the stride to the imageSize() function, to take into account additional constraints on the stride. > > What does everybody think ? > > > > > Signed-off-by: Paul Elder <paul.elder@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... Does it ? That's not normal. Which version of doxygen are you using ? I think a bug was introduced in 1.8.17 that caused extra warnings, and was fixed in 1.8.18. > > 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? Let's take NV12 as an example. For a 640x480 image, the Y plane will have a stride of 640 bytes and a total size of 640*480. The C plane will have a stride of 640 bytes as well (640 pixels / 2 for horizontal subsampling * 2 because there are Cb and Cr samples in the plane), and a total size of 640*320. The total size of the image is thus 640*480 + 640*320 = 460800 bytes. Your code will produce a stride of 640 * 3 / 2 = 960 bytes, and a total image size of 960 * 480 = 460800 bytes. The total size is the same, but the stride isn't, and doesn't match what V4L2 expect (bytesperline is the stride of the first plane for multi-planar formats, using the single-planar API). > > 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? 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 ? > > > } }, > > > }; > > >
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 :-)
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.
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; }; } /* 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, } }, { 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, } }, { 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, } }, { 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. + */ { 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, } }, };
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. 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. 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). 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. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- include/libcamera/internal/formats.h | 4 +- src/libcamera/formats.cpp | 95 +++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 2 deletions(-)