[{"id":10991,"web_url":"https://patchwork.libcamera.org/comment/10991/","msgid":"<20200629214404.GV10681@pendragon.ideasonboard.com>","date":"2020-06-29T21:44:04","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jun 30, 2020 at 12:14:07AM +0900, Paul Elder wrote:\n> Packed formats make it difficult to calculate bytes-per-line as well as\n> buffer size with the fields that PixelFormatInfo currently has.\n> bitsPerPixel is defined as the average number of bits per pixel, and\n> only counts effective bits, so it is not useful for calculating\n> bytes-per-line and bytesused.\n\nI know there are not introduced in this patch, but I think we should\nstandardize on wording through libcamera for the V4L2 bytesperline,\nbytesused and sizeimage concepts.\n\nFor bytesperline, I propose line stride, which can shorten to stride in\ncontexts where this isn't ambiguous. The corresponding variables or\nfunctions would be lineStride or stride.\n\nFor sizeimage, I propose frame size or frame length (we usually talk\nabout frames rather than images).\n\nFor bytesused I'm more undecided, I know I don't like the name much (but\nmaybe I'm biased by too much exposure to V4L2 :-)), and I'm thinking\nabout payload size. Payload length doesn't sound very good for some\nreason, which makes me think that we should use frame size instead of\nframe length. Better options are welcome.\n\n> To fix this, we introduce a concept of a \"pixel group\". The size of this\n> group is defined as the minimum number of pixels (including padding)\n> necessary in a row when the image has only one column of effective\n> pixels.\n\nIs that right ? Thinking about SBGGR12_CSI2P for instance, you set\npixelsPerGroup to 2 and bytesPerGroup to 3, which seems correct to me,\nbut is that really one column ? Maybe we should define the group as the\nminimum number of pixels that are stored in an integer number of bytes ?\nIt should also be explicitly defined as applying in the horizontal\ndirection only. Should we also mention that these values apply before\nany horizontal downsampling ?\n\n> The pixel group has one more attribute, that is the \"bytes per\n> group\". This determines how many bytes one pixel group consumes. These\n> are the fields pixelsPerGroup and bytesPerGroup that are defined in this\n> patch. Defining these two values makes it really simple to calculate\n> bytes-per-line, as ceil(width / pixelsPerGroup) * bytesPerGroup, where\n> width is measured in number of pixels. The ceiling accounts for padding.\n> \n> For example, for something simple like BGR888, it is self-explanatory:\n> the pixel group size is 1, and the bytes necessary is 3. For YUYV, the\n> CbCr pair is shared between two pixels, so even if you have only one\n> pixel, you would still need a padded second Y, therefore the pixel\n> group size is 2, and bytes necessary is 4 (as opposed to 1 and 2).\n\nAnd this invalidates my definition proposal above :-) Maybe we could\nkeep your definition based on \"columns\", but we would then need to\ndefine what a column is. Maybe it could be defined somehow based on the\nconcept of repeating pattern. pixelsPerColumn and bytesPerColumn may\nthen be alternative names for the fields.\n\n> NV12 seems like it shold be 6 bytes with 4 pixels, since there is\n> downsampling in the Y axis as well, however, the pixel group is only\n> in terms of rows, so it is half of that, at 2 pixels and 3 bytes. The\n> IPU3 formats are also self-explanatory, coming from a comment in the\n> driver, a pixel group is 50, and it consumes 64 bytes.\n\nThis also invalidates my definition as we would then have 25 and 32.\n\nWhat bothers me a tiny bit with the IPU3 raw format here is that we're\nmixing two concepts, the natural alignment required by the format (which\nwould lead to 25/32 here), and the alignment required by the IPU3 (which\nis 64 bytes). It could be entirely conceivable that a DMA engine that\nwrites YUYV data would require a 16, 32 or 64 bytes alignment. This is\nsomething that can only be known by the corresponding pipeline handler\n(possibly getting the information from the kernel driver). I'd thus be\ntempted to go for 25/32 for the IPU3 formats, and handle the additional\nalignment constraint in the pipeline handler, even if we know that in\npractice the format will only be used by the IPU3 hardware.\n\nWhat does everybody think ?\n\n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/internal/formats.h | 4 +-\n> src/libcamera/formats.cpp | 95 +++++++++++++++++++++++++++-\n> 2 files changed, 97 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index f59ac8f..dc19492 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -45,13 +45,15 @@ public:\n> \n> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> \n> -\t/* \\todo Add support for non-contiguous memory planes */\n> \tconst char *name;\n> \tPixelFormat format;\n> \tV4L2PixelFormat v4l2Format;\n> \tunsigned int bitsPerPixel;\n> \tenum ColourEncoding colourEncoding;\n> \tbool packed;\n> +\n> +\tunsigned int bytesPerGroup;\n> +\tunsigned int pixelsPerGroup;\n\nMissing documentation :-) Didn't doxygen warn you ?\n\nI would also swap the two fields, I think it's more natural to say that\n2 pixels are stored in 3 bytes than saying that 3 bytes store 2 pixels.\n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index d3b722c..88b5168 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -179,6 +179,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 24,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::RGB888, {\n> \t\t.name = \"RGB888\",\n> @@ -187,6 +189,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 24,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::ABGR8888, {\n> \t\t.name = \"ABGR8888\",\n> @@ -195,6 +199,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 32,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::ARGB8888, {\n> \t\t.name = \"ARGB8888\",\n> @@ -203,6 +209,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 32,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::BGRA8888, {\n> \t\t.name = \"BGRA8888\",\n> @@ -211,6 +219,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 32,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::RGBA8888, {\n> \t\t.name = \"RGBA8888\",\n> @@ -219,6 +229,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 32,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \n> \t/* YUV packed formats. */\n> @@ -229,6 +241,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::YVYU, {\n> \t\t.name = \"YVYU\",\n> @@ -237,6 +251,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::UYVY, {\n> \t\t.name = \"UYVY\",\n> @@ -245,6 +261,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::VYUY, {\n> \t\t.name = \"VYUY\",\n> @@ -253,6 +271,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \n> \t/* YUV planar formats. */\n> @@ -263,6 +283,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n\nShouldn't this be 2/2, as in a Y line, every pixel is stored in 1 byte ?\nSame for the NV16/61 formats. NV24/42 would be 1/1. We'll need an\nadditional field to handle multiplanar formats. Or maybe the fields are\nmeant to average all planes ? In that case this should be defined\nexplicitly, but the ceil(width / pixelsPerGroup) * bytesPerGroup\ncalculation you mention in the commit message wouldn't be correct\nanymore. I'm tempted to say that the values should apply to the first\nplane only, and add other fields to address the other planes (or maybe\nwe need to replicate those two fields per plane ?).\n\n> \t} },\n> \t{ formats::NV21, {\n> \t\t.name = \"NV21\",\n> @@ -271,6 +293,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::NV16, {\n> \t\t.name = \"NV16\",\n> @@ -279,6 +303,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::NV61, {\n> \t\t.name = \"NV61\",\n> @@ -287,6 +313,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::NV24, {\n> \t\t.name = \"NV24\",\n> @@ -295,6 +323,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 24,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::NV42, {\n> \t\t.name = \"NV42\",\n> @@ -303,6 +333,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 24,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \t{ formats::YUV420, {\n> \t\t.name = \"YUV420\",\n> @@ -311,6 +343,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n\nThis should be addresses similarly based on the outcome of the\ndiscussion regarding NV formats.\n\n> \t} },\n> \t{ formats::YUV422, {\n> \t\t.name = \"YUV422\",\n> @@ -319,6 +353,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \n> \t/* Greyscale formats. */\n> @@ -329,6 +365,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 8,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 1,\n> +\t\t.pixelsPerGroup = 1,\n> \t} },\n> \n> \t/* Bayer formats. */\n> @@ -339,6 +377,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 8,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 2,\n> +\t\t.pixelsPerGroup = 2,\n\nFor 8-bit Bayer formats, technically, 1/1 could work, but I suppose it\nwould make little sense. I think a word about Bayer formats in the\ncommit message (and in the documentation, which should copy a large part\nof the commit message) would be useful.\n\n> \t} },\n> \t{ formats::SGBRG8, {\n> \t\t.name = \"SGBRG8\",\n> @@ -347,6 +387,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 8,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 2,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGRBG8, {\n> \t\t.name = \"SGRBG8\",\n> @@ -355,6 +397,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 8,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 2,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SRGGB8, {\n> \t\t.name = \"SRGGB8\",\n> @@ -363,6 +407,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 8,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 2,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SBGGR10, {\n> \t\t.name = \"SBGGR10\",\n> @@ -371,6 +417,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGBRG10, {\n> \t\t.name = \"SGBRG10\",\n> @@ -379,6 +427,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGRBG10, {\n> \t\t.name = \"SGRBG10\",\n> @@ -387,6 +437,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SRGGB10, {\n> \t\t.name = \"SRGGB10\",\n> @@ -395,6 +447,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SBGGR10_CSI2P, {\n> \t\t.name = \"SBGGR10_CSI2P\",\n> @@ -403,6 +457,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 5,\n> +\t\t.pixelsPerGroup = 4,\n> \t} },\n> \t{ formats::SGBRG10_CSI2P, {\n> \t\t.name = \"SGBRG10_CSI2P\",\n> @@ -411,6 +467,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 5,\n> +\t\t.pixelsPerGroup = 4,\n> \t} },\n> \t{ formats::SGRBG10_CSI2P, {\n> \t\t.name = \"SGRBG10_CSI2P\",\n> @@ -419,6 +477,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 5,\n> +\t\t.pixelsPerGroup = 4,\n> \t} },\n> \t{ formats::SRGGB10_CSI2P, {\n> \t\t.name = \"SRGGB10_CSI2P\",\n> @@ -427,6 +487,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 5,\n> +\t\t.pixelsPerGroup = 4,\n> \t} },\n> \t{ formats::SBGGR12, {\n> \t\t.name = \"SBGGR12\",\n> @@ -435,6 +497,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGBRG12, {\n> \t\t.name = \"SGBRG12\",\n> @@ -443,6 +507,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGRBG12, {\n> \t\t.name = \"SGRBG12\",\n> @@ -451,6 +517,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SRGGB12, {\n> \t\t.name = \"SRGGB12\",\n> @@ -459,6 +527,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SBGGR12_CSI2P, {\n> \t\t.name = \"SBGGR12_CSI2P\",\n> @@ -467,6 +537,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGBRG12_CSI2P, {\n> \t\t.name = \"SGBRG12_CSI2P\",\n> @@ -475,6 +547,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SGRBG12_CSI2P, {\n> \t\t.name = \"SGRBG12_CSI2P\",\n> @@ -483,6 +557,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SRGGB12_CSI2P, {\n> \t\t.name = \"SRGGB12_CSI2P\",\n> @@ -491,6 +567,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 12,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 3,\n> +\t\t.pixelsPerGroup = 2,\n> \t} },\n> \t{ formats::SBGGR10_IPU3, {\n> \t\t.name = \"SBGGR10_IPU3\",\n> @@ -499,6 +577,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 64,\n> +\t\t.pixelsPerGroup = 50,\n> \t} },\n> \t{ formats::SGBRG10_IPU3, {\n> \t\t.name = \"SGBRG10_IPU3\",\n> @@ -507,6 +587,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 64,\n> +\t\t.pixelsPerGroup = 50,\n> \t} },\n> \t{ formats::SGRBG10_IPU3, {\n> \t\t.name = \"SGRBG10_IPU3\",\n> @@ -515,6 +597,8 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 64,\n> +\t\t.pixelsPerGroup = 50,\n> \t} },\n> \t{ formats::SRGGB10_IPU3, {\n> \t\t.name = \"SRGGB10_IPU3\",\n> @@ -523,16 +607,25 @@ const std::map pixelFormatInfo{\n> \t\t.bitsPerPixel = 10,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> \t\t.packed = true,\n> +\t\t.bytesPerGroup = 64,\n> +\t\t.pixelsPerGroup = 50,\n> \t} },\n> \n> \t/* Compressed formats. */\n> +\t/*\n> +\t * \\todo Get a better image size estimate for MJPEG, via\n> +\t * StreamConfiguration, instead of using the worst-case\n> +\t * width * height * bpp of uncompressed data.\n> +\t */\n\nI'm not sure this comment belongs here.\n\n> \t{ formats::MJPEG, {\n> \t\t.name = \"MJPEG\",\n> \t\t.format = formats::MJPEG,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> -\t\t.bitsPerPixel = 0,\n> +\t\t.bitsPerPixel = 16,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> \t\t.packed = false,\n> +\t\t.bytesPerGroup = 4,\n> +\t\t.pixelsPerGroup = 2,\n\nI'd rather set these fields to 0 for MJPEG instead of faking them. JPEG\ncan store YUV 4:4:4, 4:2:2 or 4:2:0, so the values are not very\nrelevant. Pipeline handlers that can produce MJPEG (that's just UVC)\nwill need to set the frame size manually instead of using helpers.\n\n> \t} },\n> };\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 25839BF415\n\tfor ;\n\tMon, 29 Jun 2020 21:44:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFCF4609DF;\n\tMon, 29 Jun 2020 23:44:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20B64603B2\n\tfor ;\n\tMon, 29 Jun 2020 23:44:10 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1AFCD299;\n\tMon, 29 Jun 2020 23:44:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lKxw+1wM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593467049;\n\tbh=xVJegk67PrxIgCS7646y2uf+gwVrU7KqiF0z3nsu1Nw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lKxw+1wMdAPTc4EIiN9B9KFNO2fADBpM0Mb5A01VSlXkLZdnBQRrUgW0T1xgH1Vif\n\tkMczmTxEc9kDXigZiqFwlo1yQD19f0x4slEA49UyyfS3qx+4QeGK6V8lDG50CzrtTm\n\t3DnivL9LGiijh6aGHd/4IfBqUvQrFHjDlUMJN1bk=","Date":"Tue, 30 Jun 2020 00:44:04 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200629214404.GV10681@pendragon.ideasonboard.com>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200629151411.216477-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10999,"web_url":"https://patchwork.libcamera.org/comment/10999/","msgid":"<20200630074248.GB198306@pyrite.rasen.tech>","date":"2020-06-30T07:42:48","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Tue, Jun 30, 2020 at 12:44:04AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Tue, Jun 30, 2020 at 12:14:07AM +0900, Paul Elder wrote:\n> > Packed formats make it difficult to calculate bytes-per-line as well as\n> > buffer size with the fields that PixelFormatInfo currently has.\n> > bitsPerPixel is defined as the average number of bits per pixel, and\n> > only counts effective bits, so it is not useful for calculating\n> > bytes-per-line and bytesused.\n> \n> I know there are not introduced in this patch, but I think we should\n> standardize on wording through libcamera for the V4L2 bytesperline,\n> bytesused and sizeimage concepts.\n\nYes.\n\n> For bytesperline, I propose line stride, which can shorten to stride in\n> contexts where this isn't ambiguous. The corresponding variables or\n> functions would be lineStride or stride.\n\nI agree.\n\n> For sizeimage, I propose frame size or frame length (we usually talk\n> about frames rather than images).\n\nI think this is fine too. Or frame buffer size for more explicitness?\n\n> For bytesused I'm more undecided, I know I don't like the name much (but\n> maybe I'm biased by too much exposure to V4L2 :-)), and I'm thinking\n> about payload size. Payload length doesn't sound very good for some\n> reason, which makes me think that we should use frame size instead of\n> frame length. Better options are welcome.\n\nPayload size should be fine. It's the number of effective bytes.\n\n> > To fix this, we introduce a concept of a \"pixel group\". The size of this\n> > group is defined as the minimum number of pixels (including padding)\n> > necessary in a row when the image has only one column of effective\n> > pixels.\n> \n> Is that right ?\n\nIt should be.\n\n> Thinking about SBGGR12_CSI2P for instance, you set\n> pixelsPerGroup to 2 and bytesPerGroup to 3, which seems correct to me,\n> but is that really one column ?\n\nIf you have only one column of pixels, then you still need one B and one\nG, as well as the third byte for the low bits. For the non-packed\nformats I suppose it's ambiguous (maybe just B and no G is fine?), but\nin the packed ones clearly you need the G to align the low bits in the\nthird byte.\n\n> Maybe we should define the group as the\n> minimum number of pixels that are stored in an integer number of bytes ?\n\nAs you've noticed later, no :)\n\n> It should also be explicitly defined as applying in the horizontal\n> direction only.\n\nYes. I thought I did:\n> > necessary in a *row* when the image has only one column of effective\n\n> Should we also mention that these values apply before\n> any horizontal downsampling ?\n\nI was thinking that the horizontal downsampling influences the\nparameters of the pixel group for the format.\n\n> > The pixel group has one more attribute, that is the \"bytes per\n> > group\". This determines how many bytes one pixel group consumes. These\n> > are the fields pixelsPerGroup and bytesPerGroup that are defined in this\n> > patch. Defining these two values makes it really simple to calculate\n> > bytes-per-line, as ceil(width / pixelsPerGroup) * bytesPerGroup, where\n> > width is measured in number of pixels. The ceiling accounts for padding.\n> > \n> > For example, for something simple like BGR888, it is self-explanatory:\n> > the pixel group size is 1, and the bytes necessary is 3. For YUYV, the\n> > CbCr pair is shared between two pixels, so even if you have only one\n> > pixel, you would still need a padded second Y, therefore the pixel\n> > group size is 2, and bytes necessary is 4 (as opposed to 1 and 2).\n> \n> And this invalidates my definition proposal above :-) Maybe we could\n> keep your definition based on \"columns\", but we would then need to\n> define what a column is.\n\nI thought it was sufficient to say one column of *effective pixels*.\nLike, for the IPU3 formats if you have one column of effective pixels,\nyou'll still need 49 more columns of padding pixels. Obviously irl\nyou're not going to have a frame size of 1xH, so this was a theoretical\ndefinition.\n\n> Maybe it could be defined somehow based on the\n> concept of repeating pattern.\n\nI was thinking that, but I couldn't find a concise and precise way to\nword a definition :/\n\n> pixelsPerColumn and bytesPerColumn may\n> then be alternative names for the fields.\n\nThat doesn't quite work either, since these aren't strictly per column.\nFor example, for YUYV, if you have one column of effective pixels,\nyou'll have two total columns, where the second is padding. But if you\nhave two columns of effective pixels, you'll still only have two total\ncolumns.\n\n> > NV12 seems like it shold be 6 bytes with 4 pixels, since there is\n> > downsampling in the Y axis as well, however, the pixel group is only\n> > in terms of rows, so it is half of that, at 2 pixels and 3 bytes. The\n> > IPU3 formats are also self-explanatory, coming from a comment in the\n> > driver, a pixel group is 50, and it consumes 64 bytes.\n> \n> This also invalidates my definition as we would then have 25 and 32.\n> \n> What bothers me a tiny bit with the IPU3 raw format here is that we're\n> mixing two concepts, the natural alignment required by the format (which\n> would lead to 25/32 here), and the alignment required by the IPU3 (which\n> is 64 bytes). It could be entirely conceivable that a DMA engine that\n> writes YUYV data would require a 16, 32 or 64 bytes alignment. This is\n> something that can only be known by the corresponding pipeline handler\n> (possibly getting the information from the kernel driver). I'd thus be\n> tempted to go for 25/32 for the IPU3 formats, and handle the additional\n> alignment constraint in the pipeline handler, even if we know that in\n> practice the format will only be used by the IPU3 hardware.\n\nOh, shoot, yeah, that's a problem :/\n\nI would totally go for adding another field for alignment. In that case\nit is indeed more correct to have the IPU3 pipeline handler to fill it\nout.\n\n> What does everybody think ?\n> \n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/internal/formats.h | 4 +-\n> > src/libcamera/formats.cpp | 95 +++++++++++++++++++++++++++-\n> > 2 files changed, 97 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index f59ac8f..dc19492 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -45,13 +45,15 @@ public:\n> > \n> > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > \n> > -\t/* \\todo Add support for non-contiguous memory planes */\n> > \tconst char *name;\n> > \tPixelFormat format;\n> > \tV4L2PixelFormat v4l2Format;\n> > \tunsigned int bitsPerPixel;\n> > \tenum ColourEncoding colourEncoding;\n> > \tbool packed;\n> > +\n> > +\tunsigned int bytesPerGroup;\n> > +\tunsigned int pixelsPerGroup;\n> \n> Missing documentation :-) Didn't doxygen warn you ?\n\nOops, yes :p\n\nI got used to ignoring doxygen's warnings because it always warns about\nmissing docs in camera and controls...\n\n> I would also swap the two fields, I think it's more natural to say that\n> 2 pixels are stored in 3 bytes than saying that 3 bytes store 2 pixels.\n\nYeah, that's probably better.\n\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index d3b722c..88b5168 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -179,6 +179,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 24,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::RGB888, {\n> > \t\t.name = \"RGB888\",\n> > @@ -187,6 +189,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 24,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::ABGR8888, {\n> > \t\t.name = \"ABGR8888\",\n> > @@ -195,6 +199,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 32,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::ARGB8888, {\n> > \t\t.name = \"ARGB8888\",\n> > @@ -203,6 +209,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 32,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::BGRA8888, {\n> > \t\t.name = \"BGRA8888\",\n> > @@ -211,6 +219,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 32,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::RGBA8888, {\n> > \t\t.name = \"RGBA8888\",\n> > @@ -219,6 +229,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 32,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \n> > \t/* YUV packed formats. */\n> > @@ -229,6 +241,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::YVYU, {\n> > \t\t.name = \"YVYU\",\n> > @@ -237,6 +251,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::UYVY, {\n> > \t\t.name = \"UYVY\",\n> > @@ -245,6 +261,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::VYUY, {\n> > \t\t.name = \"VYUY\",\n> > @@ -253,6 +271,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \n> > \t/* YUV planar formats. */\n> > @@ -263,6 +283,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> \n> Shouldn't this be 2/2, as in a Y line, every pixel is stored in 1 byte ?\n> Same for the NV16/61 formats. NV24/42 would be 1/1. We'll need an\n> additional field to handle multiplanar formats. Or maybe the fields are\n> meant to average all planes ?\n\nYes, I meant to average the planes. Well, averaged in the Y axis. In the\nX axis it's as usual. Since we're only using these fields to calculate\nstride and frame size, I think it should be sufficient.\n\n> In that case this should be defined\n> explicitly, but the ceil(width / pixelsPerGroup) * bytesPerGroup\n> calculation you mention in the commit message wouldn't be correct\n> anymore.\n\nHm? I thought it works. What's wrong with it?\n\n> I'm tempted to say that the values should apply to the first\n> plane only, and add other fields to address the other planes (or maybe\n> we need to replicate those two fields per plane ?).\n> \n> > \t} },\n> > \t{ formats::NV21, {\n> > \t\t.name = \"NV21\",\n> > @@ -271,6 +293,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::NV16, {\n> > \t\t.name = \"NV16\",\n> > @@ -279,6 +303,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::NV61, {\n> > \t\t.name = \"NV61\",\n> > @@ -287,6 +313,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::NV24, {\n> > \t\t.name = \"NV24\",\n> > @@ -295,6 +323,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 24,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::NV42, {\n> > \t\t.name = \"NV42\",\n> > @@ -303,6 +333,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 24,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \t{ formats::YUV420, {\n> > \t\t.name = \"YUV420\",\n> > @@ -311,6 +343,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> \n> This should be addresses similarly based on the outcome of the\n> discussion regarding NV formats.\n> \n> > \t} },\n> > \t{ formats::YUV422, {\n> > \t\t.name = \"YUV422\",\n> > @@ -319,6 +353,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \n> > \t/* Greyscale formats. */\n> > @@ -329,6 +365,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 8,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 1,\n> > +\t\t.pixelsPerGroup = 1,\n> > \t} },\n> > \n> > \t/* Bayer formats. */\n> > @@ -339,6 +377,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 8,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 2,\n> > +\t\t.pixelsPerGroup = 2,\n> \n> For 8-bit Bayer formats, technically, 1/1 could work, but I suppose it\n> would make little sense. I think a word about Bayer formats in the\n> commit message (and in the documentation, which should copy a large part\n> of the commit message) would be useful.\n\nOkay.\n\n> > \t} },\n> > \t{ formats::SGBRG8, {\n> > \t\t.name = \"SGBRG8\",\n> > @@ -347,6 +387,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 8,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 2,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGRBG8, {\n> > \t\t.name = \"SGRBG8\",\n> > @@ -355,6 +397,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 8,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 2,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SRGGB8, {\n> > \t\t.name = \"SRGGB8\",\n> > @@ -363,6 +407,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 8,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 2,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SBGGR10, {\n> > \t\t.name = \"SBGGR10\",\n> > @@ -371,6 +417,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGBRG10, {\n> > \t\t.name = \"SGBRG10\",\n> > @@ -379,6 +427,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGRBG10, {\n> > \t\t.name = \"SGRBG10\",\n> > @@ -387,6 +437,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SRGGB10, {\n> > \t\t.name = \"SRGGB10\",\n> > @@ -395,6 +447,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SBGGR10_CSI2P, {\n> > \t\t.name = \"SBGGR10_CSI2P\",\n> > @@ -403,6 +457,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 5,\n> > +\t\t.pixelsPerGroup = 4,\n> > \t} },\n> > \t{ formats::SGBRG10_CSI2P, {\n> > \t\t.name = \"SGBRG10_CSI2P\",\n> > @@ -411,6 +467,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 5,\n> > +\t\t.pixelsPerGroup = 4,\n> > \t} },\n> > \t{ formats::SGRBG10_CSI2P, {\n> > \t\t.name = \"SGRBG10_CSI2P\",\n> > @@ -419,6 +477,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 5,\n> > +\t\t.pixelsPerGroup = 4,\n> > \t} },\n> > \t{ formats::SRGGB10_CSI2P, {\n> > \t\t.name = \"SRGGB10_CSI2P\",\n> > @@ -427,6 +487,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 5,\n> > +\t\t.pixelsPerGroup = 4,\n> > \t} },\n> > \t{ formats::SBGGR12, {\n> > \t\t.name = \"SBGGR12\",\n> > @@ -435,6 +497,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGBRG12, {\n> > \t\t.name = \"SGBRG12\",\n> > @@ -443,6 +507,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGRBG12, {\n> > \t\t.name = \"SGRBG12\",\n> > @@ -451,6 +517,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SRGGB12, {\n> > \t\t.name = \"SRGGB12\",\n> > @@ -459,6 +527,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SBGGR12_CSI2P, {\n> > \t\t.name = \"SBGGR12_CSI2P\",\n> > @@ -467,6 +537,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGBRG12_CSI2P, {\n> > \t\t.name = \"SGBRG12_CSI2P\",\n> > @@ -475,6 +547,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SGRBG12_CSI2P, {\n> > \t\t.name = \"SGRBG12_CSI2P\",\n> > @@ -483,6 +557,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SRGGB12_CSI2P, {\n> > \t\t.name = \"SRGGB12_CSI2P\",\n> > @@ -491,6 +567,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 12,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 3,\n> > +\t\t.pixelsPerGroup = 2,\n> > \t} },\n> > \t{ formats::SBGGR10_IPU3, {\n> > \t\t.name = \"SBGGR10_IPU3\",\n> > @@ -499,6 +577,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 64,\n> > +\t\t.pixelsPerGroup = 50,\n> > \t} },\n> > \t{ formats::SGBRG10_IPU3, {\n> > \t\t.name = \"SGBRG10_IPU3\",\n> > @@ -507,6 +587,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 64,\n> > +\t\t.pixelsPerGroup = 50,\n> > \t} },\n> > \t{ formats::SGRBG10_IPU3, {\n> > \t\t.name = \"SGRBG10_IPU3\",\n> > @@ -515,6 +597,8 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 64,\n> > +\t\t.pixelsPerGroup = 50,\n> > \t} },\n> > \t{ formats::SRGGB10_IPU3, {\n> > \t\t.name = \"SRGGB10_IPU3\",\n> > @@ -523,16 +607,25 @@ const std::map pixelFormatInfo{\n> > \t\t.bitsPerPixel = 10,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > \t\t.packed = true,\n> > +\t\t.bytesPerGroup = 64,\n> > +\t\t.pixelsPerGroup = 50,\n> > \t} },\n> > \n> > \t/* Compressed formats. */\n> > +\t/*\n> > +\t * \\todo Get a better image size estimate for MJPEG, via\n> > +\t * StreamConfiguration, instead of using the worst-case\n> > +\t * width * height * bpp of uncompressed data.\n> > +\t */\n> \n> I'm not sure this comment belongs here.\n\nI don't think it does.\n\n> > \t{ formats::MJPEG, {\n> > \t\t.name = \"MJPEG\",\n> > \t\t.format = formats::MJPEG,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > -\t\t.bitsPerPixel = 0,\n> > +\t\t.bitsPerPixel = 16,\n> > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > \t\t.packed = false,\n> > +\t\t.bytesPerGroup = 4,\n> > +\t\t.pixelsPerGroup = 2,\n> \n> I'd rather set these fields to 0 for MJPEG instead of faking them. JPEG\n> can store YUV 4:4:4, 4:2:2 or 4:2:0, so the values are not very\n> relevant. Pipeline handlers that can produce MJPEG (that's just UVC)\n> will need to set the frame size manually instead of using helpers.\n\nAh, okay. So... we'll still need check if format is MJPEG and then get\nthe parameters from the pipeline handler instead? I think that'll messy\nup the code... But we want pipeline handlers to fill out the alignment\nparameter anyway, so maybe the pipeline handlers could set format info\nparameters?\n\n> > \t} },\n> > };\n> > \n\n\nThanks,\n\nPaul","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 917B4BF415\n\tfor ;\n\tTue, 30 Jun 2020 07:42:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D91760C58;\n\tTue, 30 Jun 2020 09:42:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3DC7609A9\n\tfor ;\n\tTue, 30 Jun 2020 09:42:57 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C3EF329F;\n\tTue, 30 Jun 2020 09:42:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BUDqLuv5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593502977;\n\tbh=ygSA4XhxNdP2hief4JbFQHgFyyZQlJ3A9eaZhtDmNXc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BUDqLuv5TQQLYNWGG+/3yIUIstyHYyCWHxYGBIvYtsAnXUJPSoaCVEHgwVmMr97wn\n\tkWHdUYj+gphJSFh1HBTy64WKcPvbrxy3/UCHcHy7/KksIKwOfnhcAT58NbXjwR6xsn\n\t05xV9Tv5vQNT4McA5XbX8/fqsIMykUBLHMV3tbAM=","Date":"Tue, 30 Jun 2020 16:42:48 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Message-ID":"<20200630074248.GB198306@pyrite.rasen.tech>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-3-paul.elder@ideasonboard.com>\n\t<20200629214404.GV10681@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200629214404.GV10681@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11000,"web_url":"https://patchwork.libcamera.org/comment/11000/","msgid":"<20200630074910.GC198306@pyrite.rasen.tech>","date":"2020-06-30T07:49:10","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"\n\nOn Tue, Jun 30, 2020 at 04:42:48PM +0900, Paul Elder wrote:\n> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > index f59ac8f..dc19492 100644\n> > > --- a/include/libcamera/internal/formats.h\n> > > +++ b/include/libcamera/internal/formats.h\n> > > @@ -45,13 +45,15 @@ public:\n> > > \n> > > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > > \n> > > -\t/* \\todo Add support for non-contiguous memory planes */\n> > > \tconst char *name;\n> > > \tPixelFormat format;\n> > > \tV4L2PixelFormat v4l2Format;\n> > > \tunsigned int bitsPerPixel;\n> > > \tenum ColourEncoding colourEncoding;\n> > > \tbool packed;\n> > > +\n> > > +\tunsigned int bytesPerGroup;\n> > > +\tunsigned int pixelsPerGroup;\n> > \n> > Missing documentation :-) Didn't doxygen warn you ?\n> \n> Oops, yes :p\n> \n> I got used to ignoring doxygen's warnings because it always warns about\n> missing docs in camera and controls...\n> \n> > I would also swap the two fields, I think it's more natural to say that\n> > 2 pixels are stored in 3 bytes than saying that 3 bytes store 2 pixels.\n> \n> Yeah, that's probably better.\n\nWait, I take that back. I put bytes first then pixels, because then you\ncan say it's 3 bytes per 2 pixels. It's not a sentence, it's more like a\nunit of measurement.\n\n\n\n\nPaul","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 826EFBFFE2\n\tfor ;\n\tTue, 30 Jun 2020 07:49:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE70260C57;\n\tTue, 30 Jun 2020 09:49:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECAE7609A9\n\tfor ;\n\tTue, 30 Jun 2020 09:49:17 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6807529F;\n\tTue, 30 Jun 2020 09:49:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"n+b7rosU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593503357;\n\tbh=u3my4QZk/O4y3uNh/RLHswPR9a+FHQO1gPwiOapbveE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n+b7rosUwyQ7VC7kcmZp5EtkM8WmImHD7JSOEjnHyGP8+CqAGKaghGorea6/O8K82\n\tG9gjN192VeNxl7oOtiKJ/j2VaGmes4FHuPmSW64Y2pzBiD1WWS4qjWO92+tB1i4G/j\n\t2oq5SJ/RXj6BwGahVtViv2z69Pd0DP4/QNE79Zt4=","Date":"Tue, 30 Jun 2020 16:49:10 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Message-ID":"<20200630074910.GC198306@pyrite.rasen.tech>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-3-paul.elder@ideasonboard.com>\n\t<20200629214404.GV10681@pendragon.ideasonboard.com>\n\t<20200630074248.GB198306@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200630074248.GB198306@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11042,"web_url":"https://patchwork.libcamera.org/comment/11042/","msgid":"<20200701123258.GA22532@pendragon.ideasonboard.com>","date":"2020-07-01T12:32:58","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nI realized when reviewing v2 that we never completed this discussion.\nLet's get to the bottom of it to prepare for v3 :-)\n\nOn Tue, Jun 30, 2020 at 04:42:48PM +0900, Paul Elder wrote:\n> On Tue, Jun 30, 2020 at 12:44:04AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 30, 2020 at 12:14:07AM +0900, Paul Elder wrote:\n> > > Packed formats make it difficult to calculate bytes-per-line as well as\n> > > buffer size with the fields that PixelFormatInfo currently has.\n> > > bitsPerPixel is defined as the average number of bits per pixel, and\n> > > only counts effective bits, so it is not useful for calculating\n> > > bytes-per-line and bytesused.\n> > \n> > I know there are not introduced in this patch, but I think we should\n> > standardize on wording through libcamera for the V4L2 bytesperline,\n> > bytesused and sizeimage concepts.\n> \n> Yes.\n> \n> > For bytesperline, I propose line stride, which can shorten to stride in\n> > contexts where this isn't ambiguous. The corresponding variables or\n> > functions would be lineStride or stride.\n> \n> I agree.\n> \n> > For sizeimage, I propose frame size or frame length (we usually talk\n> > about frames rather than images).\n> \n> I think this is fine too. Or frame buffer size for more explicitness?\n\nThe buffer could be allocated larger than the frame size, so I'd rather\nuse frame size.\n\n> > For bytesused I'm more undecided, I know I don't like the name much (but\n> > maybe I'm biased by too much exposure to V4L2 :-)), and I'm thinking\n> > about payload size. Payload length doesn't sound very good for some\n> > reason, which makes me think that we should use frame size instead of\n> > frame length. Better options are welcome.\n> \n> Payload size should be fine. It's the number of effective bytes.\n> \n> > > To fix this, we introduce a concept of a \"pixel group\". The size of this\n> > > group is defined as the minimum number of pixels (including padding)\n> > > necessary in a row when the image has only one column of effective\n> > > pixels.\n> > \n> > Is that right ?\n> \n> It should be.\n> \n> > Thinking about SBGGR12_CSI2P for instance, you set\n> > pixelsPerGroup to 2 and bytesPerGroup to 3, which seems correct to me,\n> > but is that really one column ?\n> \n> If you have only one column of pixels, then you still need one B and one\n> G, as well as the third byte for the low bits. For the non-packed\n> formats I suppose it's ambiguous (maybe just B and no G is fine?), but\n> in the packed ones clearly you need the G to align the low bits in the\n> third byte.\n\nI don't dispute the correctness of your values, but I'm wondering how\nyou define \"column\" in this context. Intuitively a column in an image\nwould be one pixel wide for me, and that doesn't match the description\nyou make here. It's just a matter of wording I think. Please see below\nfor more on this topic (I had a 'aahhh' moment when reading your reply).\n\n> > Maybe we should define the group as the\n> > minimum number of pixels that are stored in an integer number of bytes ?\n> \n> As you've noticed later, no :)\n> \n> > It should also be explicitly defined as applying in the horizontal\n> > direction only.\n> \n> Yes. I thought I did:\n> > > necessary in a *row* when the image has only one column of effective\n\nMy bad.\n\n> > Should we also mention that these values apply before\n> > any horizontal downsampling ?\n> \n> I was thinking that the horizontal downsampling influences the\n> parameters of the pixel group for the format.\n\nWe can discuss this below for the NV formats, but the idea is that\nmulti-planar formats will downsample the chroma plane(s) only. There are\nmultiple ways to express the necessary information, for instance with\none pair of pixels + bytes per group per plane, or with explicit\nsubsampling factors. I'm not sure yet which would be best.\n\n> > > The pixel group has one more attribute, that is the \"bytes per\n> > > group\". This determines how many bytes one pixel group consumes. These\n> > > are the fields pixelsPerGroup and bytesPerGroup that are defined in this\n> > > patch. Defining these two values makes it really simple to calculate\n> > > bytes-per-line, as ceil(width / pixelsPerGroup) * bytesPerGroup, where\n> > > width is measured in number of pixels. The ceiling accounts for padding.\n> > > \n> > > For example, for something simple like BGR888, it is self-explanatory:\n> > > the pixel group size is 1, and the bytes necessary is 3. For YUYV, the\n> > > CbCr pair is shared between two pixels, so even if you have only one\n> > > pixel, you would still need a padded second Y, therefore the pixel\n> > > group size is 2, and bytes necessary is 4 (as opposed to 1 and 2).\n> > \n> > And this invalidates my definition proposal above :-) Maybe we could\n> > keep your definition based on \"columns\", but we would then need to\n> > define what a column is.\n> \n> I thought it was sufficient to say one column of *effective pixels*.\n> Like, for the IPU3 formats if you have one column of effective pixels,\n> you'll still need 49 more columns of padding pixels. Obviously irl\n> you're not going to have a frame size of 1xH, so this was a theoretical\n> definition.\n\nAahhh I get it now. Re-reading your commit message it's pretty clear\nindeed, but for some reason I didn't get it initially.\n\n> > Maybe it could be defined somehow based on the\n> > concept of repeating pattern.\n> \n> I was thinking that, but I couldn't find a concise and precise way to\n> word a definition :/\n> \n> > pixelsPerColumn and bytesPerColumn may\n> > then be alternative names for the fields.\n> \n> That doesn't quite work either, since these aren't strictly per column.\n> For example, for YUYV, if you have one column of effective pixels,\n> you'll have two total columns, where the second is padding. But if you\n> have two columns of effective pixels, you'll still only have two total\n> columns.\n> \n> > > NV12 seems like it shold be 6 bytes with 4 pixels, since there is\n> > > downsampling in the Y axis as well, however, the pixel group is only\n> > > in terms of rows, so it is half of that, at 2 pixels and 3 bytes. The\n> > > IPU3 formats are also self-explanatory, coming from a comment in the\n> > > driver, a pixel group is 50, and it consumes 64 bytes.\n> > \n> > This also invalidates my definition as we would then have 25 and 32.\n> > \n> > What bothers me a tiny bit with the IPU3 raw format here is that we're\n> > mixing two concepts, the natural alignment required by the format (which\n> > would lead to 25/32 here), and the alignment required by the IPU3 (which\n> > is 64 bytes). It could be entirely conceivable that a DMA engine that\n> > writes YUYV data would require a 16, 32 or 64 bytes alignment. This is\n> > something that can only be known by the corresponding pipeline handler\n> > (possibly getting the information from the kernel driver). I'd thus be\n> > tempted to go for 25/32 for the IPU3 formats, and handle the additional\n> > alignment constraint in the pipeline handler, even if we know that in\n> > practice the format will only be used by the IPU3 hardware.\n> \n> Oh, shoot, yeah, that's a problem :/\n> \n> I would totally go for adding another field for alignment. In that case\n> it is indeed more correct to have the IPU3 pipeline handler to fill it\n> out.\n\nI noticed you haven't changed that in v2, not sure if it was\nintentional.\n\nPipeline handlers can't fill their alignment requirements in\nPixelFormatInfo, as we have a global const map of pixel format info.\nThey thus need to either apply their constraints manually, or pass the\nconstraints to the PixelFormatInfo::bytesPerLine() function. The most\ncommon type of constraint is an alignment in bytes, so you could add a\nparameter for that, with a default value of 1. Now, if a device has\nalignment constraints expressed in pixels, that wouldn't work anymore,\nand having multiple arguments to the function to specify different types\nof alignment constraints would quickly make it difficult to use. It may\nbe better to leave it to the pipeline handler entirely.\n\nBecause of this, we also need to pass the stride to the imageSize()\nfunction, to take into account additional constraints on the stride.\n\n> > What does everybody think ?\n> > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > include/libcamera/internal/formats.h | 4 +-\n> > > src/libcamera/formats.cpp | 95 +++++++++++++++++++++++++++-\n> > > 2 files changed, 97 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > index f59ac8f..dc19492 100644\n> > > --- a/include/libcamera/internal/formats.h\n> > > +++ b/include/libcamera/internal/formats.h\n> > > @@ -45,13 +45,15 @@ public:\n> > > \n> > > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > > \n> > > -\t/* \\todo Add support for non-contiguous memory planes */\n> > > \tconst char *name;\n> > > \tPixelFormat format;\n> > > \tV4L2PixelFormat v4l2Format;\n> > > \tunsigned int bitsPerPixel;\n> > > \tenum ColourEncoding colourEncoding;\n> > > \tbool packed;\n> > > +\n> > > +\tunsigned int bytesPerGroup;\n> > > +\tunsigned int pixelsPerGroup;\n> > \n> > Missing documentation :-) Didn't doxygen warn you ?\n> \n> Oops, yes :p\n> \n> I got used to ignoring doxygen's warnings because it always warns about\n> missing docs in camera and controls...\n\nDoes it ? That's not normal. Which version of doxygen are you using ? I\nthink a bug was introduced in 1.8.17 that caused extra warnings, and was\nfixed in 1.8.18.\n\n> > I would also swap the two fields, I think it's more natural to say that\n> > 2 pixels are stored in 3 bytes than saying that 3 bytes store 2 pixels.\n> \n> Yeah, that's probably better.\n> \n> > > };\n> > > \n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > index d3b722c..88b5168 100644\n> > > --- a/src/libcamera/formats.cpp\n> > > +++ b/src/libcamera/formats.cpp\n> > > @@ -179,6 +179,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 24,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::RGB888, {\n> > > \t\t.name = \"RGB888\",\n> > > @@ -187,6 +189,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 24,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::ABGR8888, {\n> > > \t\t.name = \"ABGR8888\",\n> > > @@ -195,6 +199,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 32,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::ARGB8888, {\n> > > \t\t.name = \"ARGB8888\",\n> > > @@ -203,6 +209,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 32,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::BGRA8888, {\n> > > \t\t.name = \"BGRA8888\",\n> > > @@ -211,6 +219,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 32,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::RGBA8888, {\n> > > \t\t.name = \"RGBA8888\",\n> > > @@ -219,6 +229,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 32,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \n> > > \t/* YUV packed formats. */\n> > > @@ -229,6 +241,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::YVYU, {\n> > > \t\t.name = \"YVYU\",\n> > > @@ -237,6 +251,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::UYVY, {\n> > > \t\t.name = \"UYVY\",\n> > > @@ -245,6 +261,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::VYUY, {\n> > > \t\t.name = \"VYUY\",\n> > > @@ -253,6 +271,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \n> > > \t/* YUV planar formats. */\n> > > @@ -263,6 +283,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > \n> > Shouldn't this be 2/2, as in a Y line, every pixel is stored in 1 byte ?\n> > Same for the NV16/61 formats. NV24/42 would be 1/1. We'll need an\n> > additional field to handle multiplanar formats. Or maybe the fields are\n> > meant to average all planes ?\n> \n> Yes, I meant to average the planes. Well, averaged in the Y axis. In the\n> X axis it's as usual. Since we're only using these fields to calculate\n> stride and frame size, I think it should be sufficient.\n> \n> > In that case this should be defined\n> > explicitly, but the ceil(width / pixelsPerGroup) * bytesPerGroup\n> > calculation you mention in the commit message wouldn't be correct\n> > anymore.\n> \n> Hm? I thought it works. What's wrong with it?\n\nLet's take NV12 as an example. For a 640x480 image, the Y plane will\nhave a stride of 640 bytes and a total size of 640*480. The C plane will\nhave a stride of 640 bytes as well (640 pixels / 2 for horizontal\nsubsampling * 2 because there are Cb and Cr samples in the plane), and a\ntotal size of 640*320. The total size of the image is thus 640*480 +\n640*320 = 460800 bytes.\n\nYour code will produce a stride of 640 * 3 / 2 = 960 bytes, and a total\nimage size of 960 * 480 = 460800 bytes. The total size is the same, but\nthe stride isn't, and doesn't match what V4L2 expect (bytesperline is\nthe stride of the first plane for multi-planar formats, using the\nsingle-planar API).\n\n> > I'm tempted to say that the values should apply to the first\n> > plane only, and add other fields to address the other planes (or maybe\n> > we need to replicate those two fields per plane ?).\n> > \n> > > \t} },\n> > > \t{ formats::NV21, {\n> > > \t\t.name = \"NV21\",\n> > > @@ -271,6 +293,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::NV16, {\n> > > \t\t.name = \"NV16\",\n> > > @@ -279,6 +303,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::NV61, {\n> > > \t\t.name = \"NV61\",\n> > > @@ -287,6 +313,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::NV24, {\n> > > \t\t.name = \"NV24\",\n> > > @@ -295,6 +323,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 24,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::NV42, {\n> > > \t\t.name = \"NV42\",\n> > > @@ -303,6 +333,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 24,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \t{ formats::YUV420, {\n> > > \t\t.name = \"YUV420\",\n> > > @@ -311,6 +343,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > \n> > This should be addresses similarly based on the outcome of the\n> > discussion regarding NV formats.\n> > \n> > > \t} },\n> > > \t{ formats::YUV422, {\n> > > \t\t.name = \"YUV422\",\n> > > @@ -319,6 +353,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \n> > > \t/* Greyscale formats. */\n> > > @@ -329,6 +365,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 8,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 1,\n> > > +\t\t.pixelsPerGroup = 1,\n> > > \t} },\n> > > \n> > > \t/* Bayer formats. */\n> > > @@ -339,6 +377,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 8,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 2,\n> > > +\t\t.pixelsPerGroup = 2,\n> > \n> > For 8-bit Bayer formats, technically, 1/1 could work, but I suppose it\n> > would make little sense. I think a word about Bayer formats in the\n> > commit message (and in the documentation, which should copy a large part\n> > of the commit message) would be useful.\n> \n> Okay.\n> \n> > > \t} },\n> > > \t{ formats::SGBRG8, {\n> > > \t\t.name = \"SGBRG8\",\n> > > @@ -347,6 +387,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 8,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 2,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGRBG8, {\n> > > \t\t.name = \"SGRBG8\",\n> > > @@ -355,6 +397,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 8,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 2,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SRGGB8, {\n> > > \t\t.name = \"SRGGB8\",\n> > > @@ -363,6 +407,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 8,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 2,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SBGGR10, {\n> > > \t\t.name = \"SBGGR10\",\n> > > @@ -371,6 +417,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGBRG10, {\n> > > \t\t.name = \"SGBRG10\",\n> > > @@ -379,6 +427,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGRBG10, {\n> > > \t\t.name = \"SGRBG10\",\n> > > @@ -387,6 +437,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SRGGB10, {\n> > > \t\t.name = \"SRGGB10\",\n> > > @@ -395,6 +447,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SBGGR10_CSI2P, {\n> > > \t\t.name = \"SBGGR10_CSI2P\",\n> > > @@ -403,6 +457,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 5,\n> > > +\t\t.pixelsPerGroup = 4,\n> > > \t} },\n> > > \t{ formats::SGBRG10_CSI2P, {\n> > > \t\t.name = \"SGBRG10_CSI2P\",\n> > > @@ -411,6 +467,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 5,\n> > > +\t\t.pixelsPerGroup = 4,\n> > > \t} },\n> > > \t{ formats::SGRBG10_CSI2P, {\n> > > \t\t.name = \"SGRBG10_CSI2P\",\n> > > @@ -419,6 +477,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 5,\n> > > +\t\t.pixelsPerGroup = 4,\n> > > \t} },\n> > > \t{ formats::SRGGB10_CSI2P, {\n> > > \t\t.name = \"SRGGB10_CSI2P\",\n> > > @@ -427,6 +487,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 5,\n> > > +\t\t.pixelsPerGroup = 4,\n> > > \t} },\n> > > \t{ formats::SBGGR12, {\n> > > \t\t.name = \"SBGGR12\",\n> > > @@ -435,6 +497,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGBRG12, {\n> > > \t\t.name = \"SGBRG12\",\n> > > @@ -443,6 +507,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGRBG12, {\n> > > \t\t.name = \"SGRBG12\",\n> > > @@ -451,6 +517,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SRGGB12, {\n> > > \t\t.name = \"SRGGB12\",\n> > > @@ -459,6 +527,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SBGGR12_CSI2P, {\n> > > \t\t.name = \"SBGGR12_CSI2P\",\n> > > @@ -467,6 +537,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGBRG12_CSI2P, {\n> > > \t\t.name = \"SGBRG12_CSI2P\",\n> > > @@ -475,6 +547,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SGRBG12_CSI2P, {\n> > > \t\t.name = \"SGRBG12_CSI2P\",\n> > > @@ -483,6 +557,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SRGGB12_CSI2P, {\n> > > \t\t.name = \"SRGGB12_CSI2P\",\n> > > @@ -491,6 +567,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 12,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 3,\n> > > +\t\t.pixelsPerGroup = 2,\n> > > \t} },\n> > > \t{ formats::SBGGR10_IPU3, {\n> > > \t\t.name = \"SBGGR10_IPU3\",\n> > > @@ -499,6 +577,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 64,\n> > > +\t\t.pixelsPerGroup = 50,\n> > > \t} },\n> > > \t{ formats::SGBRG10_IPU3, {\n> > > \t\t.name = \"SGBRG10_IPU3\",\n> > > @@ -507,6 +587,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 64,\n> > > +\t\t.pixelsPerGroup = 50,\n> > > \t} },\n> > > \t{ formats::SGRBG10_IPU3, {\n> > > \t\t.name = \"SGRBG10_IPU3\",\n> > > @@ -515,6 +597,8 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 64,\n> > > +\t\t.pixelsPerGroup = 50,\n> > > \t} },\n> > > \t{ formats::SRGGB10_IPU3, {\n> > > \t\t.name = \"SRGGB10_IPU3\",\n> > > @@ -523,16 +607,25 @@ const std::map pixelFormatInfo{\n> > > \t\t.bitsPerPixel = 10,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRAW,\n> > > \t\t.packed = true,\n> > > +\t\t.bytesPerGroup = 64,\n> > > +\t\t.pixelsPerGroup = 50,\n> > > \t} },\n> > > \n> > > \t/* Compressed formats. */\n> > > +\t/*\n> > > +\t * \\todo Get a better image size estimate for MJPEG, via\n> > > +\t * StreamConfiguration, instead of using the worst-case\n> > > +\t * width * height * bpp of uncompressed data.\n> > > +\t */\n> > \n> > I'm not sure this comment belongs here.\n> \n> I don't think it does.\n> \n> > > \t{ formats::MJPEG, {\n> > > \t\t.name = \"MJPEG\",\n> > > \t\t.format = formats::MJPEG,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > > -\t\t.bitsPerPixel = 0,\n> > > +\t\t.bitsPerPixel = 16,\n> > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > \t\t.packed = false,\n> > > +\t\t.bytesPerGroup = 4,\n> > > +\t\t.pixelsPerGroup = 2,\n> > \n> > I'd rather set these fields to 0 for MJPEG instead of faking them. JPEG\n> > can store YUV 4:4:4, 4:2:2 or 4:2:0, so the values are not very\n> > relevant. Pipeline handlers that can produce MJPEG (that's just UVC)\n> > will need to set the frame size manually instead of using helpers.\n> \n> Ah, okay. So... we'll still need check if format is MJPEG and then get\n> the parameters from the pipeline handler instead? I think that'll messy\n> up the code... But we want pipeline handlers to fill out the alignment\n> parameter anyway, so maybe the pipeline handlers could set format info\n> parameters?\n\nThey can't do that as the PixelFormatInfo instances are global (and\nconst). Pipeline handlers already report the stride through\nStreamConfiguration::stride. I think we need an additional frameSize (or\nframeLength ? or another name ?) field in there to report the total\nsize, which would be especially important for MJPEG.\n\nNiklas, any opinion ?\n\n> > > \t} },\n> > > };\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2F713BFFE2\n\tfor ;\n\tWed, 1 Jul 2020 12:33:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B61CA60C53;\n\tWed, 1 Jul 2020 14:33:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA01C609A9\n\tfor ;\n\tWed, 1 Jul 2020 14:33:02 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21679556;\n\tWed, 1 Jul 2020 14:33:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FO19oK26\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593606782;\n\tbh=Vtv+iqJY7mjU8CKbUBVCebDeptg00XqvQQ1J0N9OgWM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FO19oK26gST0OM9ualfdTVaD9Yew/X5i9k+3DbWnELi6mqT8/JrTRQjR2E+DQzeMx\n\t7DOLhbxMGcLgzBM4wYKu46ZlA+815wo1nsW/rVV/Hju+8ugRzE/KCxbRouQZaD1qXS\n\t6Lxpx0UAaKiyXN/M3BKUwHdDOKJBYEcAFkmaptsE=","Date":"Wed, 1 Jul 2020 15:32:58 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200701123258.GA22532@pendragon.ideasonboard.com>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-3-paul.elder@ideasonboard.com>\n\t<20200629214404.GV10681@pendragon.ideasonboard.com>\n\t<20200630074248.GB198306@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200630074248.GB198306@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11043,"web_url":"https://patchwork.libcamera.org/comment/11043/","msgid":"<20200701142433.GB2399385@oden.dyn.berto.se>","date":"2020-07-01T14:24:33","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul and Laurent,\n\nOn 2020-07-01 15:32:58 +0300, Laurent Pinchart wrote:\n\n\n\n> > > > \t{ formats::MJPEG, {\n> > > > \t\t.name = \"MJPEG\",\n> > > > \t\t.format = formats::MJPEG,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > > > -\t\t.bitsPerPixel = 0,\n> > > > +\t\t.bitsPerPixel = 16,\n> > > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > > \t\t.packed = false,\n> > > > +\t\t.bytesPerGroup = 4,\n> > > > +\t\t.pixelsPerGroup = 2,\n> > > \n> > > I'd rather set these fields to 0 for MJPEG instead of faking them. JPEG\n> > > can store YUV 4:4:4, 4:2:2 or 4:2:0, so the values are not very\n> > > relevant. Pipeline handlers that can produce MJPEG (that's just UVC)\n> > > will need to set the frame size manually instead of using helpers.\n> > \n> > Ah, okay. So... we'll still need check if format is MJPEG and then get\n> > the parameters from the pipeline handler instead? I think that'll messy\n> > up the code... But we want pipeline handlers to fill out the alignment\n> > parameter anyway, so maybe the pipeline handlers could set format info\n> > parameters?\n> \n> They can't do that as the PixelFormatInfo instances are global (and\n> const). Pipeline handlers already report the stride through\n> StreamConfiguration::stride. I think we need an additional frameSize (or\n> frameLength ? or another name ?) field in there to report the total\n> size, which would be especially important for MJPEG.\n> \n> Niklas, any opinion ?\n\nAlways :-)\n\nFirst so that I understand you correctly frame{Size,Length} would be the \nframe size in bytes without or without stride taken into account? Or \nwould it depend on the format? Which now that I write it sounds odd as \nstride hos no real meaning for MJPEG. Put it another way if I where to \ncopy frame{Size,Length} bytes from the buffer I would be guaranteed to \nhave all have image data, but I still need to depending on format deal \nwith alignment or would it be the size of the data after alignment have \nbeen dealt with?\n\nI see no problem with a pipeline controlling such fields in the same way \nstride is handled. It would be nice if somewhere in core a sanity check \ncould be added. If we take stride as an example, if its initialize to \nzero and we expect the pipeline to updated it, it could be useful for \nsome core code to check that its != 0 before handing the structure to an \napplication. We can't guarantee the value is correct but we can warn if \na pipeline have not attempted to update it.\n\nMy gut is telling me it's nicer to default to 0 then to the worst case \nsince that will force pipelines to do the right thing :-)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 26315BFFE2\n\tfor ;\n\tWed, 1 Jul 2020 14:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5E23603B4;\n\tWed, 1 Jul 2020 16:24:37 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFB0E603B4\n\tfor ;\n\tWed, 1 Jul 2020 16:24:35 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id f5so11343022ljj.10\n\tfor ;\n\tWed, 01 Jul 2020 07:24:35 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv20sm2108054lfe.46.2020.07.01.07.24.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 01 Jul 2020 07:24:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"M5M2J1uQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=0H2Uf8gVXt94OvVgXtBdmiRKRUfZwOCST8RN3r5QNnA=;\n\tb=M5M2J1uQdPYl+Dfv7nhhkQ+VbaB+EnPYp9XQbOW/K9ESMX17zBgqofBsEmOZi7w6xO\n\tygT/CxA13qHm/3SuiYUJY+2Xp/HsdVXT/CNOONpmAr9two4bQnoLTDLX3t1lAYiKLWyj\n\tj0PrV9R7ydrnSrUyYDOyOMoOXP3T9NFWHTtr05g/woYz54WWHyurmIXRZhu16DkHy88O\n\tgu9gceId9CWCPNHqAWPh67QIzkNAk088Pcz9YkhzSnIGevSomj4LsTWwXbOHftua+wgr\n\tYhnukGmfpd/n2qyiHaJ08pV1edFn3zc34cqoC0NHdoWgOAbnpiFyE/Dc0lrqRncFw+ep\n\tcAow==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=0H2Uf8gVXt94OvVgXtBdmiRKRUfZwOCST8RN3r5QNnA=;\n\tb=cynjovtkQG7vJGa4+78LuEVHK5VErG5C4fTzEOiroKycnuW4+RbJznMxKoNzhBvxkp\n\taacsYVfILW+s5KacIWikIhmazXolcseoTGvqRA6fxMm4pe8Len0wlMHCkjqcEPxlD8xY\n\tvbRPb29aFHKbFH2rIZvUc57nTFqC+Pu4kRjfy7yctN/7ugPI3SJcxDFgoAuv1OXZ2loL\n\tT7darDTx5o0k+2qXNH+TnsHSMEG6PXQAU6gWLCqdjrR+qdxcEXVLdSAatDqDK6eIlVX7\n\tcWMMiufYXLxGmkELGan21ItiKvytqKZ/NpqZVLmV8DnTelg85426eI5V/FMTECey45LA\n\tiILQ==","X-Gm-Message-State":"AOAM530luq/9gSsq6S+RfUpsrThc+UbBCqGIQDYwbwgq86hej++3aDDY\n\t6z7EFUMl9+7F4XOf4uPBkF098g==","X-Google-Smtp-Source":"ABdhPJwr/h/pmw738RjYVwYMDh8xKrDaQXtO5wvQG//LDmvaLF+f9qEwyMZ14n0OFpJuLXpId1JoBg==","X-Received":"by 2002:a2e:8016:: with SMTP id\n\tj22mr5010178ljg.405.1593613474952; \n\tWed, 01 Jul 2020 07:24:34 -0700 (PDT)","Date":"Wed, 1 Jul 2020 16:24:33 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200701142433.GB2399385@oden.dyn.berto.se>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-3-paul.elder@ideasonboard.com>\n\t<20200629214404.GV10681@pendragon.ideasonboard.com>\n\t<20200630074248.GB198306@pyrite.rasen.tech>\n\t<20200701123258.GA22532@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701123258.GA22532@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11085,"web_url":"https://patchwork.libcamera.org/comment/11085/","msgid":"<20200702213620.GT12562@pendragon.ideasonboard.com>","date":"2020-07-02T21:36:20","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jul 01, 2020 at 04:24:33PM +0200, Niklas Söderlund wrote:\n> Hi Paul and Laurent,\n> \n> On 2020-07-01 15:32:58 +0300, Laurent Pinchart wrote:\n> \n> \n> \n> > > > > \t{ formats::MJPEG, {\n> > > > > \t\t.name = \"MJPEG\",\n> > > > > \t\t.format = formats::MJPEG,\n> > > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > > > > -\t\t.bitsPerPixel = 0,\n> > > > > +\t\t.bitsPerPixel = 16,\n> > > > > \t\t.colourEncoding = PixelFormatInfo::ColourEncodingYUV,\n> > > > > \t\t.packed = false,\n> > > > > +\t\t.bytesPerGroup = 4,\n> > > > > +\t\t.pixelsPerGroup = 2,\n> > > > \n> > > > I'd rather set these fields to 0 for MJPEG instead of faking them. JPEG\n> > > > can store YUV 4:4:4, 4:2:2 or 4:2:0, so the values are not very\n> > > > relevant. Pipeline handlers that can produce MJPEG (that's just UVC)\n> > > > will need to set the frame size manually instead of using helpers.\n> > > \n> > > Ah, okay. So... we'll still need check if format is MJPEG and then get\n> > > the parameters from the pipeline handler instead? I think that'll messy\n> > > up the code... But we want pipeline handlers to fill out the alignment\n> > > parameter anyway, so maybe the pipeline handlers could set format info\n> > > parameters?\n> > \n> > They can't do that as the PixelFormatInfo instances are global (and\n> > const). Pipeline handlers already report the stride through\n> > StreamConfiguration::stride. I think we need an additional frameSize (or\n> > frameLength ? or another name ?) field in there to report the total\n> > size, which would be especially important for MJPEG.\n> > \n> > Niklas, any opinion ?\n> \n> Always :-)\n> \n> First so that I understand you correctly frame{Size,Length} would be the \n> frame size in bytes without or without stride taken into account? Or \n> would it depend on the format? Which now that I write it sounds odd as \n> stride hos no real meaning for MJPEG. Put it another way if I where to \n> copy frame{Size,Length} bytes from the buffer I would be guaranteed to \n> have all have image data, but I still need to depending on format deal \n> with alignment or would it be the size of the data after alignment have \n> been dealt with?\n\nIf you copy frameSize bytes you will have all the data. For MJPEG that\nwill be a blob, and stride will be 0. For other formats it will be a set\nof lines, each of them stride bytes long, thus including padding as\nnecessary.\n\nWe also need to report stride, size (and offset) per plane.\n\n> I see no problem with a pipeline controlling such fields in the same way \n> stride is handled. It would be nice if somewhere in core a sanity check \n> could be added. If we take stride as an example, if its initialize to \n> zero and we expect the pipeline to updated it, it could be useful for \n> some core code to check that its != 0 before handing the structure to an \n> application. We can't guarantee the value is correct but we can warn if \n> a pipeline have not attempted to update it.\n\nGood idea.\n\n> My gut is telling me it's nicer to default to 0 then to the worst case \n> since that will force pipelines to do the right thing :-)\n\nI agree.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3D121BE905\n\tfor ;\n\tThu, 2 Jul 2020 21:36:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0E65609C7;\n\tThu, 2 Jul 2020 23:36:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3B3E603B4\n\tfor ;\n\tThu, 2 Jul 2020 23:36:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5570F9CB;\n\tThu, 2 Jul 2020 23:36:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TyrOC444\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593725784;\n\tbh=H6sEtgiAUHEFCQ6FaARbGYNsxCKlUGLk7q5zbKmVvMc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TyrOC444F6ZdcImuYHEu7pwIoIkP39537n4OgB+Rk+jgVQclGzNojeMLLznuqE+Vf\n\tI9HAvOyby/velO+mw/OC16TDedgyr4DRv9SspD/ARb4BfdTjnoLMge6GbDbiRg3/3h\n\tXcJxL5tV48374m4u47s9+7nfVQB4aXYoWyD8RfFA=","Date":"Fri, 3 Jul 2020 00:36:20 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200702213620.GT12562@pendragon.ideasonboard.com>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-3-paul.elder@ideasonboard.com>\n\t<20200629214404.GV10681@pendragon.ideasonboard.com>\n\t<20200630074248.GB198306@pyrite.rasen.tech>\n\t<20200701123258.GA22532@pendragon.ideasonboard.com>\n\t<20200701142433.GB2399385@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701142433.GB2399385@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: formats: Add fields to\n\tinfo ease calculating bpl","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]