[libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to info ease calculating bpl
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 29 23:44:04 CEST 2020
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 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 ?
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.
> } },
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list