[raw2rgbpnm PATCH v4 00/11] 10-bit packed raw support, other fixes and improvements

[raw2rgbpnm PATCH v4 00/11] 10-bit packed raw support, other fixes and improvements

[Sakari Ailus]

Hi all,

These patches add support for 10-bit packed raw formats. Adding support
for more bit depths would be trivial (at least up to 16 bits) but I only
needed 10. :-)

The code could be prettier.

since v3:

- Add more patches for cleanups and fixes in general.

- Allow setting stride, with format thhe height is calculated. This is
  practical for use with e.g. yavta, which can write multiple images into
  the same file.

- Simplified calculation of offsets in 10-bit packed Bayer unpacking.

- Compare old format info pointer to determine a supported format has been
  found.

since v2:

- Drop bpc argument from raw_get() and raw_put(). The bpc argument wasn't
  really needed there as the functions don't operate on the values (but
  simply pack or unpack them).

- Use unsigned values where appropriate (i.e. make signed values
  unsigned).

since v1:

- Use __attribute__((fallthrough)) instead of a compiler flag.

- Better error message on what's wrong if a proper unpacked format isn't
  found when processing the newly supported packed 10-bit raw formats.

Sakari Ailus (11):
  Add explicit switch fallthrough notation
  Add compiler options to avoid warnings
  Add 10-bit CSI-2 packed format support
  Support long options and improve help text
  Add "help" for listing formats and de-Bayering algos, document it
  The program has been called raw2rgbpnm, not yuv_to_rgbpnm awhile now
  Arrange headers alphabetically
  Add --stride (-S) option for setting stride
  Improve input validation
  Ensure width is divisible by 4 for packed 10-bit formats
  Add -S option for setting stride

 Makefile     |   2 +-
 raw2rgbpnm.c | 199 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 160 insertions(+), 41 deletions(-)

-- 
2.47.3

[raw2rgbpnm PATCH v4 01/11] Add explicit switch fallthrough notation

[Sakari Ailus]

Use __attribute__((fallthrough)) instead of a comment this is taking
place, to make modern GCC happy.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 raw2rgbpnm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index baeb8efc863a..5df3ff7ab31d 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -362,7 +362,7 @@ static void raw_to_rgb(const struct format_info *info,
 
 	case V4L2_PIX_FMT_NV21:
 		color_pos = 0;
-		/* fallthrough */
+		__attribute__((fallthrough));
 	case V4L2_PIX_FMT_NV12:
 		src_luma = src;
 		src_chroma = &src[src_width * src_height];
@@ -576,7 +576,7 @@ static void raw_to_rgb(const struct format_info *info,
 	case V4L2_PIX_FMT_SRGGB10:
 		if (raw_layout_to_grbg(info, src, src_width, src_height, src_stride))
 			error("Can't convert RAW layout to GRBG");
-		/* fallthrough */
+		__attribute__((fallthrough));
 	case V4L2_PIX_FMT_SGRBG16:
 	case V4L2_PIX_FMT_SGRBG14:
 	case V4L2_PIX_FMT_SGRBG12:
@@ -620,7 +620,7 @@ static void raw_to_rgb(const struct format_info *info,
 	case V4L2_PIX_FMT_SRGGB8:
 		if (raw_layout_to_grbg(info, src, src_width, src_height, src_stride))
 			error("Can't convert RAW layout to GRBG");
-		/* fallthrough */
+		__attribute__((fallthrough));
 	case V4L2_PIX_FMT_SGRBG8:
 		buf = malloc(src_width * src_height * 3);
 		if (buf==NULL) error("out of memory");
@@ -688,7 +688,7 @@ static void raw_to_rgb(const struct format_info *info,
 
 	case V4L2_PIX_FMT_BGR24:
 		swaprb = !swaprb;
-		/* fallthrough */
+		__attribute__((fallthrough));
 	case V4L2_PIX_FMT_RGB24:
 		for (src_y = 0, dst_y = 0; dst_y < src_height; src_y++, dst_y++) {
 			for (src_x = 0, dst_x = 0; dst_x < src_width; ) {
-- 
2.47.3

[raw2rgbpnm PATCH v4 02/11] Add compiler options to avoid warnings

[Sakari Ailus]

Add -Wno-missing-field-initializers option to avoid warnings on modern
GCC.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index ab363501a212..7545682dbcce 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 CROSS_COMPILE ?=
 
 CC	:= $(CROSS_COMPILE)gcc
-CFLAGS	?= -O2 -W -Wall -Iinclude
+CFLAGS	?= -O2 -W -Wall -Iinclude -Wno-missing-field-initializers
 LDFLAGS	?=
 
 %.o : %.c
-- 
2.47.3

[raw2rgbpnm PATCH v4 03/11] Add 10-bit CSI-2 packed format support

[Sakari Ailus]

Add support for the 10-bit CSI-2 packed raw formats.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index 5df3ff7ab31d..14cfa7e7a46c 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -25,6 +25,7 @@
 
 #include <ctype.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
@@ -56,6 +57,7 @@ static const struct format_info {
 	unsigned int cb_pos;
 	unsigned int cr_pos;
 	unsigned int planes;
+	__u32 compat_fmt;
 } v4l2_pix_fmt_str[] = {
 	{ V4L2_PIX_FMT_RGB332,	8,  0,  "RGB332 (8 RGB-3-3-2)", 0, 0, 0, 1 },
 	{ V4L2_PIX_FMT_RGB555,	16, 5,  "RGB555 (16 RGB-5-5-5)", 0, 0, 0, 1 },
@@ -98,6 +100,10 @@ static const struct format_info {
 	{ V4L2_PIX_FMT_SGBRG10,	16, 10, "SGBRG10 (10 GBGB.. RGRG..)", 0, 0, 0, 1 },
 	{ V4L2_PIX_FMT_SGRBG10,	16, 10, "SGRBG10 (10 GRGR.. BGBG..)", 0, 0, 0, 1 },
 	{ V4L2_PIX_FMT_SRGGB10,	16, 10, "SRGGB10 (10 RGRG.. GBGB..)", 0, 0, 0, 1 },
+	{ V4L2_PIX_FMT_SBGGR10P, 10, 10, "SBGGR10P (10 BGBG.. GRGR..)", 0, 0, 0, 1, V4L2_PIX_FMT_SBGGR10 },
+	{ V4L2_PIX_FMT_SGBRG10P, 10, 10, "SGBRG10P (10 GBGB.. RGRG..)", 0, 0, 0, 1, V4L2_PIX_FMT_SGBRG10 },
+	{ V4L2_PIX_FMT_SGRBG10P, 10, 10, "SGRBG10P (10 GRGR.. BGBG..)", 0, 0, 0, 1, V4L2_PIX_FMT_SGRBG10 },
+	{ V4L2_PIX_FMT_SRGGB10P, 10, 10, "SRGGB10P (10 RGRG.. GBGB..)", 0, 0, 0, 1, V4L2_PIX_FMT_SRGGB10 },
 	{ V4L2_PIX_FMT_SBGGR12,	16, 12, "SBGGR12 (12 BGBG.. GRGR..)", 0, 0, 0, 1 },
 	{ V4L2_PIX_FMT_SGBRG12,	16, 12, "SGBRG12 (12 GBGB.. RGRG..)", 0, 0, 0, 1 },
 	{ V4L2_PIX_FMT_SGRBG12,	16, 12, "SGRBG12 (12 GRGR.. BGBG..)", 0, 0, 0, 1 },
@@ -186,6 +192,45 @@ static unsigned char *read_raw_data(char *filename, int width, int height,
 	return b;
 }
 
+static inline uint16_t raw_get(uint8_t bpp, unsigned char *ptr,
+			       unsigned int stride, unsigned int x,
+			       unsigned int y)
+{
+	switch (bpp) {
+	case 10: {
+		unsigned char *base = ptr + y * stride + x / 4 * 5;
+		unsigned int idx = x & 3U;
+
+		return (base[idx] << 2) | ((base[4] >> (idx << 1)) & 3U);
+	}
+	default:
+		error("getting raw %u not supported", bpp);
+	}
+
+	return 0;
+}
+
+static inline void raw_put(uint8_t bpp, unsigned char *ptr, unsigned int stride,
+			   unsigned int x, unsigned int y, uint16_t value)
+{
+	switch (bpp) {
+	case 10: {
+		unsigned char *base = ptr + y * stride + x / 4 * 5;
+		unsigned int idx = x & 3U;
+
+		base[idx] = value >> 2;
+		base[4] &= ~(3U << (idx << 1));
+		base[4] |= (value & 3U) << (idx << 1);
+		break;
+	}
+	case 16:
+		*(uint16_t *)&ptr[y * stride + x * 2] = value;
+		break;
+	default:
+		error("putting raw %u not supported", bpp);
+	}
+}
+
 static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src,
 			      int src_width, int src_height, unsigned int src_stride)
 {
@@ -283,6 +328,7 @@ static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src
 static void raw_to_rgb(const struct format_info *info,
 		       unsigned char *src, int src_width, int src_height, unsigned char *rgb)
 {
+	unsigned char *tmp_src = NULL;
 	unsigned int src_stride = src_width * info->bpp / 8;
 	unsigned int rgb_stride = src_width * 3;
 	unsigned char *src_luma, *src_chroma;
@@ -298,6 +344,40 @@ static void raw_to_rgb(const struct format_info *info,
 	int cr_pos;
 	int shift;
 
+	switch (info->fmt) {
+	case V4L2_PIX_FMT_SBGGR10P:
+	case V4L2_PIX_FMT_SGBRG10P:
+	case V4L2_PIX_FMT_SRGGB10P:
+	case V4L2_PIX_FMT_SGRBG10P: {
+		const struct format_info *old_info = info;
+		unsigned int new_stride = src_width * 2;
+
+		tmp_src = malloc(new_stride * src_height);
+		if (!tmp_src)
+			error("can't allocate memory for the temporary buffer");
+
+		for (src_y = 0; src_y < src_height; src_y++)
+			for (src_x = 0; src_x < src_width; src_x++)
+				raw_put(16, tmp_src, new_stride, src_x, src_y,
+					raw_get(info->bpp, src, src_stride,
+						src_x, src_y));
+
+		src_stride = new_stride;
+		src = tmp_src;
+
+		for (unsigned int i = 0; i < SIZE(v4l2_pix_fmt_str); i++) {
+			if (v4l2_pix_fmt_str[i].fmt == info->compat_fmt) {
+				info = &v4l2_pix_fmt_str[i];
+				break;
+			}
+		}
+
+		if (info == old_info)
+			error("no supported format found for %s",
+			      old_info->name);
+	}
+	}
+
 	switch (info->fmt) {
 	case V4L2_PIX_FMT_VYUY:
 	case V4L2_PIX_FMT_YVYU:
@@ -734,6 +814,8 @@ static void raw_to_rgb(const struct format_info *info,
 		}
 		break;
 	}
+
+	free(tmp_src);
 }
 
 static int parse_format(const char *p, int *w, int *h)
-- 
2.47.3

[raw2rgbpnm PATCH v4 04/11] Support long options and improve help text

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index 14cfa7e7a46c..16ef64e984b9 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -24,6 +24,7 @@
  */
 
 #include <ctype.h>
+#include <getopt.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -836,6 +837,17 @@ static int parse_format(const char *p, int *w, int *h)
 	return 0;
 }
 
+static struct option options[] = {
+	{ "algo", required_argument, NULL, 'a', },
+	{ "brightness", required_argument, NULL, 'b', },
+	{ "format", required_argument, NULL, 'f', },
+	{ "help", no_argument, NULL, 'h', },
+	{ "high-bits", no_argument, NULL, 'g', },
+	{ "size", required_argument, NULL, 's', },
+	{ "swap-rb", no_argument, NULL, 'w', },
+	{ 0 },
+};
+
 int main(int argc, char *argv[])
 {
 	FILE *f;
@@ -849,7 +861,7 @@ int main(int argc, char *argv[])
 	int width = -1;
 
 	for (;;) {
-		int c = getopt(argc, argv, "a:b:f:ghs:w");
+		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
 		if (c==-1) break;
 		switch (c) {
 		case 'a':
@@ -889,14 +901,18 @@ int main(int argc, char *argv[])
 			break;
 		case 'h':
 			printf("%s - Convert headerless raw image to RGB file (PNM)\n"
-			       "Usage: %s [-h] [-w] [-s XxY] <inputfile> <outputfile>\n"
-			       "-a <algo>     Select algorithm, use \"-a ?\" for a list\n"
-			       "-b <bright>   Set brightness (multiplier) to output image (float, default 1.0)\n"
-			       "-f <format>   Specify input file format format (-f ? for list, default UYVY)\n"
-			       "-g            Use high bits for Bayer RAW 10 data\n"
-			       "-h            Show this help\n"
-			       "-s <XxY>      Specify image size\n"
-			       "-w            Swap R and B channels\n", progname, argv[0]);
+			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
+			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
+			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"
+			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
+			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
+			       "                          (float, default 1.0)\n"
+			       "--format, -f <format>     Specify input file format format\n"
+			       "                          (-f ? for list, default UYVY)\n"
+			       "--help, -h                Show this help\n"
+			       "--high-bits, -g           Use high bits for Bayer RAW 10 data\n"
+			       "--size, -s <XxY>          Specify image size\n"
+			       "--swap-rb, -w             Swap R and B channels\n", progname, argv[0]);
 			exit(0);
 		case 's':
 			if (parse_format(optarg, &width, &height) < 0) {
-- 
2.47.3

[raw2rgbpnm PATCH v4 05/11] Add "help" for listing formats and de-Bayering algos, document it

[Sakari Ailus]

'?' is a problematic character for shells.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index 16ef64e984b9..b0151789b0c1 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -865,7 +865,7 @@ int main(int argc, char *argv[])
 		if (c==-1) break;
 		switch (c) {
 		case 'a':
-			if (optarg[0]=='?') {
+			if (optarg[0]=='?' || !strcmp(optarg, "help")) {
 				printf("Available bayer-to-rgb conversion algorithms:\n");
 				qc_print_algorithms();
 				exit(0);
@@ -876,7 +876,7 @@ int main(int argc, char *argv[])
 			brightness = (int)(atof(optarg) * 256.0 + 0.5);
 			break;
 		case 'f':
-			if (optarg[0]=='?' && optarg[1]==0) {
+			if ((optarg[0]=='?' && optarg[1]==0) || !strcmp(optarg, "help")) {
 				unsigned int i,j;
 				printf("Supported formats:\n");
 				for (i=0; i<SIZE(v4l2_pix_fmt_str); i++) {
@@ -904,11 +904,11 @@ int main(int argc, char *argv[])
 			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
 			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
 			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"
-			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
+			       "--algo, -a <algo>         Select algorithm, use \"-a help\" for a list\n"
 			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
 			       "                          (float, default 1.0)\n"
 			       "--format, -f <format>     Specify input file format format\n"
-			       "                          (-f ? for list, default UYVY)\n"
+			       "                          (-f help for list, default UYVY)\n"
 			       "--help, -h                Show this help\n"
 			       "--high-bits, -g           Use high bits for Bayer RAW 10 data\n"
 			       "--size, -s <XxY>          Specify image size\n"
-- 
2.47.3

[raw2rgbpnm PATCH v4 06/11] The program has been called raw2rgbpnm, not yuv_to_rgbpnm awhile now

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index b0151789b0c1..bb9a2d576969 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -43,7 +43,7 @@
 #define MAX(a,b)	((a)>(b)?(a):(b))
 #define MIN(a,b)	((a)<(b)?(a):(b))
 
-char *progname = "yuv_to_rgbpnm";
+char *progname = "raw2rgbpnm";
 
 static int swaprb = 0;
 static int highbits = 0;			/* Bayer RAW10 formats use high bits for data */
-- 
2.47.3

[raw2rgbpnm PATCH v4 07/11] Arrange headers alphabetically

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index bb9a2d576969..0e0a5b1e63e9 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -25,13 +25,13 @@
 
 #include <ctype.h>
 #include <getopt.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
 #include <sys/types.h>
-#include <limits.h>
 #include <linux/videodev2.h>
 #include "utils.h"
 #include "raw_to_rgb.h"
-- 
2.47.3

[raw2rgbpnm PATCH v4 08/11] Add --stride (-S) option for setting stride

[Sakari Ailus]

Being able to set stride explicitly is useful in some cases.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index 0e0a5b1e63e9..9ec9f06de052 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -139,11 +139,11 @@ static const struct format_info *get_format_info(__u32 f)
 }
 
 /* Read and return image data at given width, height and format information. */
-static unsigned char *read_raw_data(char *filename, int width, int height,
+static unsigned char *read_raw_data(char *filename, unsigned int width,
+				    unsigned int height, unsigned int line_length,
 				    const struct format_info *info)
 {
 	/* Get file size */
-	unsigned int line_length;
 	unsigned int padding = 0;
 	unsigned char *b = NULL;
 	unsigned int i;
@@ -174,9 +174,9 @@ static unsigned char *read_raw_data(char *filename, int width, int height,
 	}
 
 	/* Read data */
-	b = xalloc((width*height*bpp+7)/8);
+	b = xalloc(line_length * height);
 	if (padding == 0) {
-		r = fread(b, (width*height*bpp+7)/8, 1, f);
+		r = fread(b, line_length * height, 1, f);
 		if (r != 1)
 			error("fread");
 	} else {
@@ -327,10 +327,11 @@ static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src
 }
 
 static void raw_to_rgb(const struct format_info *info,
-		       unsigned char *src, int src_width, int src_height, unsigned char *rgb)
+		       unsigned char *src, int src_width, int src_height,
+		       unsigned int stride, unsigned char *rgb)
 {
 	unsigned char *tmp_src = NULL;
-	unsigned int src_stride = src_width * info->bpp / 8;
+	unsigned int src_stride = stride ?: src_width * info->bpp / 8;
 	unsigned int rgb_stride = src_width * 3;
 	unsigned char *src_luma, *src_chroma;
 	unsigned char *src_cb, *src_cr;
@@ -844,6 +845,7 @@ static struct option options[] = {
 	{ "help", no_argument, NULL, 'h', },
 	{ "high-bits", no_argument, NULL, 'g', },
 	{ "size", required_argument, NULL, 's', },
+	{ "stride", required_argument, NULL, 'S', },
 	{ "swap-rb", no_argument, NULL, 'w', },
 	{ 0 },
 };
@@ -855,13 +857,14 @@ int main(int argc, char *argv[])
 	char *file_in = NULL, *file_out = NULL;
 	int format = V4L2_PIX_FMT_UYVY;
 	const struct format_info *info;
+	unsigned int stride = 0;
 	int r;
 	char *algorithm_name = NULL;
 	int height = -1;
 	int width = -1;
 
 	for (;;) {
-		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
+		int c = getopt_long(argc, argv, "a:b:f:ghs:S:w", options, NULL);
 		if (c==-1) break;
 		switch (c) {
 		case 'a':
@@ -920,6 +923,9 @@ int main(int argc, char *argv[])
 				exit(0);
 			}
 			break;
+		case 'S':
+			stride = strtoul(optarg, NULL, 10);
+			break;
 		case 'w':
 			swaprb = 1;
 			break;
@@ -943,12 +949,12 @@ int main(int argc, char *argv[])
 	}
 
 	/* Read, convert, and save image */
-	src = read_raw_data(file_in, width, height, info);
+	src = read_raw_data(file_in, width, height, stride, info);
 	printf("Image size: %ix%i, bytes per pixel: %i, format: %s\n",
 	       width, height, info->bpp, info->name);
 	dst = xalloc(width*height*3);
 
-	raw_to_rgb(info, src, width, height, dst);
+	raw_to_rgb(info, src, width, height, stride, dst);
 	printf("Writing to file `%s'...\n", file_out);
 	f = fopen(file_out, "wb");
 	if (!f) error("file open failed");
-- 
2.47.3

[raw2rgbpnm PATCH v4 09/11] Improve input validation

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 raw2rgbpnm.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
index 9ec9f06de052..4b7fa0e18f9b 100644
--- a/raw2rgbpnm.c
+++ b/raw2rgbpnm.c
@@ -153,7 +153,7 @@ static unsigned char *read_raw_data(char *filename, unsigned int width,
 	if (!f) error("fopen failed");
 	int r = fseek(f, 0, SEEK_END);
 	if (r!=0) error("fseek");
-	int file_size = ftell(f);
+	long file_size = ftell(f);
 	if (file_size==-1) error("ftell");
 	r = fseek(f, 0, SEEK_SET);
 	if (r!=0) error("fseek");
@@ -163,13 +163,22 @@ static unsigned char *read_raw_data(char *filename, unsigned int width,
 		height *= info->planes;
 	}
 
-	if (file_size*8 < width*height*bpp) error("out of input data");
-	if (file_size*8 > width*height*bpp) printf("warning: too large image file\n");
+	if (!width || UINT_MAX / width / MAX(8, bpp) < height)
+		error("width or height is bad");
+	line_length = line_length ?: (width * bpp + 7) / 8;
+	if (!line_length || UINT_MAX / line_length < height ||
+	    line_length < width * bpp / 8)
+		error("line_length is bad");
+	if (file_size > UINT_MAX)
+		error("too large file");
+	if ((unsigned int)file_size < line_length * height)
+		error("out of input data");
+	if ((unsigned int)file_size > line_length * height)
+		printf("warning: too large image file\n");
 	if (file_size % height == 0) {
-		line_length = width * bpp / 8;
 		padding = file_size / height - line_length;
 		printf("%u padding bytes detected at end of line\n", padding);
-	} else if ((file_size * 8) % (width * height * bpp) != 0) {
+	} else if ((file_size * 8) % (line_length * height) != 0) {
 		printf("warning: input size not multiple of frame size\n");
 	}
 
-- 
2.47.3

Re: [raw2rgbpnm PATCH v4 02/11] Add compiler options to avoid warnings

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:08AM +0200, Sakari Ailus wrote:
> Add -Wno-missing-field-initializers option to avoid warnings on modern
> GCC.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index ab363501a212..7545682dbcce 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,7 +1,7 @@
>  CROSS_COMPILE ?=
>  
>  CC	:= $(CROSS_COMPILE)gcc
> -CFLAGS	?= -O2 -W -Wall -Iinclude
> +CFLAGS	?= -O2 -W -Wall -Iinclude -Wno-missing-field-initializers
>  LDFLAGS	?=
>  
>  %.o : %.c

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 03/11] Add 10-bit CSI-2 packed format support

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:09AM +0200, Sakari Ailus wrote:
> Add support for the 10-bit CSI-2 packed raw formats.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index 5df3ff7ab31d..14cfa7e7a46c 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -25,6 +25,7 @@
>  
>  #include <ctype.h>
>  #include <stdio.h>
> +#include <stdint.h>

You nearly got the alphabetic order right :-)

>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
> @@ -56,6 +57,7 @@ static const struct format_info {
>  	unsigned int cb_pos;
>  	unsigned int cr_pos;
>  	unsigned int planes;
> +	__u32 compat_fmt;
>  } v4l2_pix_fmt_str[] = {
>  	{ V4L2_PIX_FMT_RGB332,	8,  0,  "RGB332 (8 RGB-3-3-2)", 0, 0, 0, 1 },
>  	{ V4L2_PIX_FMT_RGB555,	16, 5,  "RGB555 (16 RGB-5-5-5)", 0, 0, 0, 1 },
> @@ -98,6 +100,10 @@ static const struct format_info {
>  	{ V4L2_PIX_FMT_SGBRG10,	16, 10, "SGBRG10 (10 GBGB.. RGRG..)", 0, 0, 0, 1 },
>  	{ V4L2_PIX_FMT_SGRBG10,	16, 10, "SGRBG10 (10 GRGR.. BGBG..)", 0, 0, 0, 1 },
>  	{ V4L2_PIX_FMT_SRGGB10,	16, 10, "SRGGB10 (10 RGRG.. GBGB..)", 0, 0, 0, 1 },
> +	{ V4L2_PIX_FMT_SBGGR10P, 10, 10, "SBGGR10P (10 BGBG.. GRGR..)", 0, 0, 0, 1, V4L2_PIX_FMT_SBGGR10 },
> +	{ V4L2_PIX_FMT_SGBRG10P, 10, 10, "SGBRG10P (10 GBGB.. RGRG..)", 0, 0, 0, 1, V4L2_PIX_FMT_SGBRG10 },
> +	{ V4L2_PIX_FMT_SGRBG10P, 10, 10, "SGRBG10P (10 GRGR.. BGBG..)", 0, 0, 0, 1, V4L2_PIX_FMT_SGRBG10 },
> +	{ V4L2_PIX_FMT_SRGGB10P, 10, 10, "SRGGB10P (10 RGRG.. GBGB..)", 0, 0, 0, 1, V4L2_PIX_FMT_SRGGB10 },
>  	{ V4L2_PIX_FMT_SBGGR12,	16, 12, "SBGGR12 (12 BGBG.. GRGR..)", 0, 0, 0, 1 },
>  	{ V4L2_PIX_FMT_SGBRG12,	16, 12, "SGBRG12 (12 GBGB.. RGRG..)", 0, 0, 0, 1 },
>  	{ V4L2_PIX_FMT_SGRBG12,	16, 12, "SGRBG12 (12 GRGR.. BGBG..)", 0, 0, 0, 1 },
> @@ -186,6 +192,45 @@ static unsigned char *read_raw_data(char *filename, int width, int height,
>  	return b;
>  }
>  
> +static inline uint16_t raw_get(uint8_t bpp, unsigned char *ptr,
> +			       unsigned int stride, unsigned int x,
> +			       unsigned int y)
> +{
> +	switch (bpp) {
> +	case 10: {
> +		unsigned char *base = ptr + y * stride + x / 4 * 5;
> +		unsigned int idx = x & 3U;
> +
> +		return (base[idx] << 2) | ((base[4] >> (idx << 1)) & 3U);
> +	}
> +	default:
> +		error("getting raw %u not supported", bpp);
> +	}
> +
> +	return 0;
> +}
> +
> +static inline void raw_put(uint8_t bpp, unsigned char *ptr, unsigned int stride,
> +			   unsigned int x, unsigned int y, uint16_t value)
> +{
> +	switch (bpp) {
> +	case 10: {
> +		unsigned char *base = ptr + y * stride + x / 4 * 5;
> +		unsigned int idx = x & 3U;
> +
> +		base[idx] = value >> 2;
> +		base[4] &= ~(3U << (idx << 1));
> +		base[4] |= (value & 3U) << (idx << 1);
> +		break;
> +	}

I assume you'll need this later in the series, it's not used here.

> +	case 16:
> +		*(uint16_t *)&ptr[y * stride + x * 2] = value;
> +		break;
> +	default:
> +		error("putting raw %u not supported", bpp);
> +	}
> +}
> +
>  static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src,
>  			      int src_width, int src_height, unsigned int src_stride)
>  {
> @@ -283,6 +328,7 @@ static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src
>  static void raw_to_rgb(const struct format_info *info,
>  		       unsigned char *src, int src_width, int src_height, unsigned char *rgb)
>  {
> +	unsigned char *tmp_src = NULL;
>  	unsigned int src_stride = src_width * info->bpp / 8;
>  	unsigned int rgb_stride = src_width * 3;
>  	unsigned char *src_luma, *src_chroma;
> @@ -298,6 +344,40 @@ static void raw_to_rgb(const struct format_info *info,
>  	int cr_pos;
>  	int shift;
>  
> +	switch (info->fmt) {
> +	case V4L2_PIX_FMT_SBGGR10P:
> +	case V4L2_PIX_FMT_SGBRG10P:
> +	case V4L2_PIX_FMT_SRGGB10P:
> +	case V4L2_PIX_FMT_SGRBG10P: {
> +		const struct format_info *old_info = info;
> +		unsigned int new_stride = src_width * 2;

I'd call this unpacked_stride. It's a bit longer, but clearer.

> +
> +		tmp_src = malloc(new_stride * src_height);
> +		if (!tmp_src)

"tmp" isn't very descriptive either.

> +			error("can't allocate memory for the temporary buffer");
> +
> +		for (src_y = 0; src_y < src_height; src_y++)
> +			for (src_x = 0; src_x < src_width; src_x++)
> +				raw_put(16, tmp_src, new_stride, src_x, src_y,
> +					raw_get(info->bpp, src, src_stride,
> +						src_x, src_y));
> +
> +		src_stride = new_stride;
> +		src = tmp_src;
> +
> +		for (unsigned int i = 0; i < SIZE(v4l2_pix_fmt_str); i++) {
> +			if (v4l2_pix_fmt_str[i].fmt == info->compat_fmt) {
> +				info = &v4l2_pix_fmt_str[i];
> +				break;
> +			}
> +		}
> +
> +		if (info == old_info)
> +			error("no supported format found for %s",
> +			      old_info->name);

Maybe you could do this before handling the conversion.

> +	}
> +	}
> +
>  	switch (info->fmt) {
>  	case V4L2_PIX_FMT_VYUY:
>  	case V4L2_PIX_FMT_YVYU:
> @@ -734,6 +814,8 @@ static void raw_to_rgb(const struct format_info *info,
>  		}
>  		break;
>  	}
> +
> +	free(tmp_src);

This makes me which the kernel's __free() macro could be used here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
>  static int parse_format(const char *p, int *w, int *h)

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 04/11] Support long options and improve help text

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:10AM +0200, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index 14cfa7e7a46c..16ef64e984b9 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -24,6 +24,7 @@
>   */
>  
>  #include <ctype.h>
> +#include <getopt.h>
>  #include <stdio.h>
>  #include <stdint.h>
>  #include <stdlib.h>
> @@ -836,6 +837,17 @@ static int parse_format(const char *p, int *w, int *h)
>  	return 0;
>  }
>  
> +static struct option options[] = {
> +	{ "algo", required_argument, NULL, 'a', },
> +	{ "brightness", required_argument, NULL, 'b', },
> +	{ "format", required_argument, NULL, 'f', },
> +	{ "help", no_argument, NULL, 'h', },
> +	{ "high-bits", no_argument, NULL, 'g', },
> +	{ "size", required_argument, NULL, 's', },
> +	{ "swap-rb", no_argument, NULL, 'w', },
> +	{ 0 },
> +};
> +
>  int main(int argc, char *argv[])
>  {
>  	FILE *f;
> @@ -849,7 +861,7 @@ int main(int argc, char *argv[])
>  	int width = -1;
>  
>  	for (;;) {
> -		int c = getopt(argc, argv, "a:b:f:ghs:w");
> +		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
>  		if (c==-1) break;
>  		switch (c) {
>  		case 'a':
> @@ -889,14 +901,18 @@ int main(int argc, char *argv[])
>  			break;
>  		case 'h':
>  			printf("%s - Convert headerless raw image to RGB file (PNM)\n"
> -			       "Usage: %s [-h] [-w] [-s XxY] <inputfile> <outputfile>\n"
> -			       "-a <algo>     Select algorithm, use \"-a ?\" for a list\n"
> -			       "-b <bright>   Set brightness (multiplier) to output image (float, default 1.0)\n"
> -			       "-f <format>   Specify input file format format (-f ? for list, default UYVY)\n"
> -			       "-g            Use high bits for Bayer RAW 10 data\n"
> -			       "-h            Show this help\n"
> -			       "-s <XxY>      Specify image size\n"
> -			       "-w            Swap R and B channels\n", progname, argv[0]);
> +			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
> +			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
> +			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"

That won't scale nicely, but I don't want to implement an options
handling framework in C right now :-)

> +			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
> +			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
> +			       "                          (float, default 1.0)\n"

Do you know if this option is used by anyone ? It seems a bit out of
place. Not an issue with this patch of course.

> +			       "--format, -f <format>     Specify input file format format\n"
> +			       "                          (-f ? for list, default UYVY)\n"
> +			       "--help, -h                Show this help\n"
> +			       "--high-bits, -g           Use high bits for Bayer RAW 10 data\n"
> +			       "--size, -s <XxY>          Specify image size\n"
> +			       "--swap-rb, -w             Swap R and B channels\n", progname, argv[0]);

I'd add a line break before progname.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			exit(0);
>  		case 's':
>  			if (parse_format(optarg, &width, &height) < 0) {

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 05/11] Add "help" for listing formats and de-Bayering algos, document it

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:11AM +0200, Sakari Ailus wrote:
> '?' is a problematic character for shells.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index 16ef64e984b9..b0151789b0c1 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -865,7 +865,7 @@ int main(int argc, char *argv[])
>  		if (c==-1) break;
>  		switch (c) {
>  		case 'a':
> -			if (optarg[0]=='?') {
> +			if (optarg[0]=='?' || !strcmp(optarg, "help")) {
>  				printf("Available bayer-to-rgb conversion algorithms:\n");
>  				qc_print_algorithms();
>  				exit(0);
> @@ -876,7 +876,7 @@ int main(int argc, char *argv[])
>  			brightness = (int)(atof(optarg) * 256.0 + 0.5);
>  			break;
>  		case 'f':
> -			if (optarg[0]=='?' && optarg[1]==0) {
> +			if ((optarg[0]=='?' && optarg[1]==0) || !strcmp(optarg, "help")) {
>  				unsigned int i,j;
>  				printf("Supported formats:\n");
>  				for (i=0; i<SIZE(v4l2_pix_fmt_str); i++) {
> @@ -904,11 +904,11 @@ int main(int argc, char *argv[])
>  			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
>  			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
>  			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"
> -			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
> +			       "--algo, -a <algo>         Select algorithm, use \"-a help\" for a list\n"
>  			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
>  			       "                          (float, default 1.0)\n"
>  			       "--format, -f <format>     Specify input file format format\n"
> -			       "                          (-f ? for list, default UYVY)\n"
> +			       "                          (-f help for list, default UYVY)\n"

While at it, quote "-f help" like done above for -a.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			       "--help, -h                Show this help\n"
>  			       "--high-bits, -g           Use high bits for Bayer RAW 10 data\n"
>  			       "--size, -s <XxY>          Specify image size\n"

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 06/11] The program has been called raw2rgbpnm, not yuv_to_rgbpnm awhile now

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:12AM +0200, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index b0151789b0c1..bb9a2d576969 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -43,7 +43,7 @@
>  #define MAX(a,b)	((a)>(b)?(a):(b))
>  #define MIN(a,b)	((a)<(b)?(a):(b))
>  
> -char *progname = "yuv_to_rgbpnm";
> +char *progname = "raw2rgbpnm";

While at it, make it const.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I wish the program would have been called raw2rgbppm, as it generates
PPM files.

>  
>  static int swaprb = 0;
>  static int highbits = 0;			/* Bayer RAW10 formats use high bits for data */

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 07/11] Arrange headers alphabetically

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:13AM +0200, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index bb9a2d576969..0e0a5b1e63e9 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -25,13 +25,13 @@
>  
>  #include <ctype.h>
>  #include <getopt.h>
> +#include <limits.h>
>  #include <stdio.h>
>  #include <stdint.h>
>  #include <stdlib.h>
>  #include <unistd.h>

Interesting... :-)

>  #include <string.h>
>  #include <sys/types.h>
> -#include <limits.h>
>  #include <linux/videodev2.h>
>  #include "utils.h"
>  #include "raw_to_rgb.h"

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 08/11] Add --stride (-S) option for setting stride

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:14AM +0200, Sakari Ailus wrote:
> Being able to set stride explicitly is useful in some cases.

Definitely.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index 0e0a5b1e63e9..9ec9f06de052 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -139,11 +139,11 @@ static const struct format_info *get_format_info(__u32 f)
>  }
>  
>  /* Read and return image data at given width, height and format information. */
> -static unsigned char *read_raw_data(char *filename, int width, int height,
> +static unsigned char *read_raw_data(char *filename, unsigned int width,
> +				    unsigned int height, unsigned int line_length,
>  				    const struct format_info *info)
>  {
>  	/* Get file size */
> -	unsigned int line_length;
>  	unsigned int padding = 0;
>  	unsigned char *b = NULL;
>  	unsigned int i;
> @@ -174,9 +174,9 @@ static unsigned char *read_raw_data(char *filename, int width, int height,
>  	}
>  
>  	/* Read data */
> -	b = xalloc((width*height*bpp+7)/8);
> +	b = xalloc(line_length * height);
>  	if (padding == 0) {
> -		r = fread(b, (width*height*bpp+7)/8, 1, f);
> +		r = fread(b, line_length * height, 1, f);
>  		if (r != 1)
>  			error("fread");
>  	} else {
> @@ -327,10 +327,11 @@ static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src
>  }
>  
>  static void raw_to_rgb(const struct format_info *info,
> -		       unsigned char *src, int src_width, int src_height, unsigned char *rgb)
> +		       unsigned char *src, int src_width, int src_height,
> +		       unsigned int stride, unsigned char *rgb)
>  {
>  	unsigned char *tmp_src = NULL;
> -	unsigned int src_stride = src_width * info->bpp / 8;
> +	unsigned int src_stride = stride ?: src_width * info->bpp / 8;

I assume the rest of the code already validates the stride value to
avoid buffer overflows. Have you checked that it does so in a way that
avoid integer overflows ? If so,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	unsigned int rgb_stride = src_width * 3;
>  	unsigned char *src_luma, *src_chroma;
>  	unsigned char *src_cb, *src_cr;
> @@ -844,6 +845,7 @@ static struct option options[] = {
>  	{ "help", no_argument, NULL, 'h', },
>  	{ "high-bits", no_argument, NULL, 'g', },
>  	{ "size", required_argument, NULL, 's', },
> +	{ "stride", required_argument, NULL, 'S', },
>  	{ "swap-rb", no_argument, NULL, 'w', },
>  	{ 0 },
>  };
> @@ -855,13 +857,14 @@ int main(int argc, char *argv[])
>  	char *file_in = NULL, *file_out = NULL;
>  	int format = V4L2_PIX_FMT_UYVY;
>  	const struct format_info *info;
> +	unsigned int stride = 0;
>  	int r;
>  	char *algorithm_name = NULL;
>  	int height = -1;
>  	int width = -1;
>  
>  	for (;;) {
> -		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
> +		int c = getopt_long(argc, argv, "a:b:f:ghs:S:w", options, NULL);
>  		if (c==-1) break;
>  		switch (c) {
>  		case 'a':
> @@ -920,6 +923,9 @@ int main(int argc, char *argv[])
>  				exit(0);
>  			}
>  			break;
> +		case 'S':
> +			stride = strtoul(optarg, NULL, 10);
> +			break;
>  		case 'w':
>  			swaprb = 1;
>  			break;
> @@ -943,12 +949,12 @@ int main(int argc, char *argv[])
>  	}
>  
>  	/* Read, convert, and save image */
> -	src = read_raw_data(file_in, width, height, info);
> +	src = read_raw_data(file_in, width, height, stride, info);
>  	printf("Image size: %ix%i, bytes per pixel: %i, format: %s\n",
>  	       width, height, info->bpp, info->name);
>  	dst = xalloc(width*height*3);
>  
> -	raw_to_rgb(info, src, width, height, dst);
> +	raw_to_rgb(info, src, width, height, stride, dst);
>  	printf("Writing to file `%s'...\n", file_out);
>  	f = fopen(file_out, "wb");
>  	if (!f) error("file open failed");

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 09/11] Improve input validation

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 10:46:15AM +0200, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  raw2rgbpnm.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index 9ec9f06de052..4b7fa0e18f9b 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -153,7 +153,7 @@ static unsigned char *read_raw_data(char *filename, unsigned int width,
>  	if (!f) error("fopen failed");
>  	int r = fseek(f, 0, SEEK_END);
>  	if (r!=0) error("fseek");
> -	int file_size = ftell(f);
> +	long file_size = ftell(f);
>  	if (file_size==-1) error("ftell");
>  	r = fseek(f, 0, SEEK_SET);
>  	if (r!=0) error("fseek");
> @@ -163,13 +163,22 @@ static unsigned char *read_raw_data(char *filename, unsigned int width,
>  		height *= info->planes;
>  	}
>  
> -	if (file_size*8 < width*height*bpp) error("out of input data");
> -	if (file_size*8 > width*height*bpp) printf("warning: too large image file\n");
> +	if (!width || UINT_MAX / width / MAX(8, bpp) < height)
> +		error("width or height is bad");

The message isn't very explicit, but I suppose that no valid usage
should lead to this.

> +	line_length = line_length ?: (width * bpp + 7) / 8;
> +	if (!line_length || UINT_MAX / line_length < height ||
> +	    line_length < width * bpp / 8)
> +		error("line_length is bad");

line_length is an internal variable, I'd write stride here.

> +	if (file_size > UINT_MAX)
> +		error("too large file");
> +	if ((unsigned int)file_size < line_length * height)
> +		error("out of input data");
> +	if ((unsigned int)file_size > line_length * height)
> +		printf("warning: too large image file\n");
>  	if (file_size % height == 0) {
> -		line_length = width * bpp / 8;
>  		padding = file_size / height - line_length;
>  		printf("%u padding bytes detected at end of line\n", padding);
> -	} else if ((file_size * 8) % (width * height * bpp) != 0) {
> +	} else if ((file_size * 8) % (line_length * height) != 0) {

I think you need to drop the * 8 here.

>  		printf("warning: input size not multiple of frame size\n");
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 03/11] Add 10-bit CSI-2 packed format support

[Sakari Ailus]

Hi Laurent,

Thank you for the review.

On Mon, Mar 16, 2026 at 04:38:30PM +0200, Laurent Pinchart wrote:
> On Tue, Mar 10, 2026 at 10:46:09AM +0200, Sakari Ailus wrote:
> > Add support for the 10-bit CSI-2 packed raw formats.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  raw2rgbpnm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> > index 5df3ff7ab31d..14cfa7e7a46c 100644
> > --- a/raw2rgbpnm.c
> > +++ b/raw2rgbpnm.c
> > @@ -25,6 +25,7 @@
> >  
> >  #include <ctype.h>
> >  #include <stdio.h>
> > +#include <stdint.h>
> 
> You nearly got the alphabetic order right :-)

Uh-oh.

The patches have been already merged but I'll write another set to address
your review comments.

> > +static inline void raw_put(uint8_t bpp, unsigned char *ptr, unsigned int stride,
> > +			   unsigned int x, unsigned int y, uint16_t value)
> > +{
> > +	switch (bpp) {
> > +	case 10: {
> > +		unsigned char *base = ptr + y * stride + x / 4 * 5;
> > +		unsigned int idx = x & 3U;
> > +
> > +		base[idx] = value >> 2;
> > +		base[4] &= ~(3U << (idx << 1));
> > +		base[4] |= (value & 3U) << (idx << 1);
> > +		break;
> > +	}
> 
> I assume you'll need this later in the series, it's not used here.

Well, almost. It could have been used. I thought first I'd convert the
conversion code to use generic pixel get / put but opted to just convert
the format into something already supported. I'd leave this in place for
now though.

> 
> > +	case 16:
> > +		*(uint16_t *)&ptr[y * stride + x * 2] = value;
> > +		break;
> > +	default:
> > +		error("putting raw %u not supported", bpp);
> > +	}
> > +}
> > +
> >  static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src,
> >  			      int src_width, int src_height, unsigned int src_stride)
> >  {
> > @@ -283,6 +328,7 @@ static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src
> >  static void raw_to_rgb(const struct format_info *info,
> >  		       unsigned char *src, int src_width, int src_height, unsigned char *rgb)
> >  {
> > +	unsigned char *tmp_src = NULL;
> >  	unsigned int src_stride = src_width * info->bpp / 8;
> >  	unsigned int rgb_stride = src_width * 3;
> >  	unsigned char *src_luma, *src_chroma;
> > @@ -298,6 +344,40 @@ static void raw_to_rgb(const struct format_info *info,
> >  	int cr_pos;
> >  	int shift;
> >  
> > +	switch (info->fmt) {
> > +	case V4L2_PIX_FMT_SBGGR10P:
> > +	case V4L2_PIX_FMT_SGBRG10P:
> > +	case V4L2_PIX_FMT_SRGGB10P:
> > +	case V4L2_PIX_FMT_SGRBG10P: {
> > +		const struct format_info *old_info = info;
> > +		unsigned int new_stride = src_width * 2;
> 
> I'd call this unpacked_stride. It's a bit longer, but clearer.

Sounds good to me.

> 
> > +
> > +		tmp_src = malloc(new_stride * src_height);
> > +		if (!tmp_src)
> 
> "tmp" isn't very descriptive either.

It is very temporary, isn't it?

> 
> > +			error("can't allocate memory for the temporary buffer");
> > +
> > +		for (src_y = 0; src_y < src_height; src_y++)
> > +			for (src_x = 0; src_x < src_width; src_x++)
> > +				raw_put(16, tmp_src, new_stride, src_x, src_y,
> > +					raw_get(info->bpp, src, src_stride,
> > +						src_x, src_y));
> > +
> > +		src_stride = new_stride;
> > +		src = tmp_src;
> > +
> > +		for (unsigned int i = 0; i < SIZE(v4l2_pix_fmt_str); i++) {
> > +			if (v4l2_pix_fmt_str[i].fmt == info->compat_fmt) {
> > +				info = &v4l2_pix_fmt_str[i];
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (info == old_info)
> > +			error("no supported format found for %s",
> > +			      old_info->name);
> 
> Maybe you could do this before handling the conversion.

Sounds good.

> 
> > +	}
> > +	}
> > +
> >  	switch (info->fmt) {
> >  	case V4L2_PIX_FMT_VYUY:
> >  	case V4L2_PIX_FMT_YVYU:
> > @@ -734,6 +814,8 @@ static void raw_to_rgb(const struct format_info *info,
> >  		}
> >  		break;
> >  	}
> > +
> > +	free(tmp_src);
> 
> This makes me which the kernel's __free() macro could be used here.

I think raw2rgbpnm could well manage without the infrastructure, couldn't
it? :-)

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you. :-)

-- 
Kind regards,

Sakari Ailus

Re: [raw2rgbpnm PATCH v4 04/11] Support long options and improve help text

[Sakari Ailus]

Moi,

On Mon, Mar 16, 2026 at 04:41:51PM +0200, Laurent Pinchart wrote:
> On Tue, Mar 10, 2026 at 10:46:10AM +0200, Sakari Ailus wrote:
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  raw2rgbpnm.c | 34 +++++++++++++++++++++++++---------
> >  1 file changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> > index 14cfa7e7a46c..16ef64e984b9 100644
> > --- a/raw2rgbpnm.c
> > +++ b/raw2rgbpnm.c
> > @@ -24,6 +24,7 @@
> >   */
> >  
> >  #include <ctype.h>
> > +#include <getopt.h>
> >  #include <stdio.h>
> >  #include <stdint.h>
> >  #include <stdlib.h>
> > @@ -836,6 +837,17 @@ static int parse_format(const char *p, int *w, int *h)
> >  	return 0;
> >  }
> >  
> > +static struct option options[] = {
> > +	{ "algo", required_argument, NULL, 'a', },
> > +	{ "brightness", required_argument, NULL, 'b', },
> > +	{ "format", required_argument, NULL, 'f', },
> > +	{ "help", no_argument, NULL, 'h', },
> > +	{ "high-bits", no_argument, NULL, 'g', },
> > +	{ "size", required_argument, NULL, 's', },
> > +	{ "swap-rb", no_argument, NULL, 'w', },
> > +	{ 0 },
> > +};
> > +
> >  int main(int argc, char *argv[])
> >  {
> >  	FILE *f;
> > @@ -849,7 +861,7 @@ int main(int argc, char *argv[])
> >  	int width = -1;
> >  
> >  	for (;;) {
> > -		int c = getopt(argc, argv, "a:b:f:ghs:w");
> > +		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
> >  		if (c==-1) break;
> >  		switch (c) {
> >  		case 'a':
> > @@ -889,14 +901,18 @@ int main(int argc, char *argv[])
> >  			break;
> >  		case 'h':
> >  			printf("%s - Convert headerless raw image to RGB file (PNM)\n"
> > -			       "Usage: %s [-h] [-w] [-s XxY] <inputfile> <outputfile>\n"
> > -			       "-a <algo>     Select algorithm, use \"-a ?\" for a list\n"
> > -			       "-b <bright>   Set brightness (multiplier) to output image (float, default 1.0)\n"
> > -			       "-f <format>   Specify input file format format (-f ? for list, default UYVY)\n"
> > -			       "-g            Use high bits for Bayer RAW 10 data\n"
> > -			       "-h            Show this help\n"
> > -			       "-s <XxY>      Specify image size\n"
> > -			       "-w            Swap R and B channels\n", progname, argv[0]);
> > +			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
> > +			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
> > +			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"
> 
> That won't scale nicely, but I don't want to implement an options
> handling framework in C right now :-)
> 
> > +			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
> > +			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
> > +			       "                          (float, default 1.0)\n"
> 
> Do you know if this option is used by anyone ? It seems a bit out of
> place. Not an issue with this patch of course.

I have no idea. But keeping it won't do any harm, will it?

> 
> > +			       "--format, -f <format>     Specify input file format format\n"
> > +			       "                          (-f ? for list, default UYVY)\n"
> > +			       "--help, -h                Show this help\n"
> > +			       "--high-bits, -g           Use high bits for Bayer RAW 10 data\n"
> > +			       "--size, -s <XxY>          Specify image size\n"
> > +			       "--swap-rb, -w             Swap R and B channels\n", progname, argv[0]);
> 
> I'd add a line break before progname.

Sounds good.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  			exit(0);
> >  		case 's':
> >  			if (parse_format(optarg, &width, &height) < 0) {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Sakari Ailus

Re: [raw2rgbpnm PATCH v4 04/11] Support long options and improve help text

[Laurent Pinchart]

On Tue, Mar 17, 2026 at 11:49:14AM +0200, Sakari Ailus wrote:
> Moi,
> 
> On Mon, Mar 16, 2026 at 04:41:51PM +0200, Laurent Pinchart wrote:
> > On Tue, Mar 10, 2026 at 10:46:10AM +0200, Sakari Ailus wrote:
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  raw2rgbpnm.c | 34 +++++++++++++++++++++++++---------
> > >  1 file changed, 25 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> > > index 14cfa7e7a46c..16ef64e984b9 100644
> > > --- a/raw2rgbpnm.c
> > > +++ b/raw2rgbpnm.c
> > > @@ -24,6 +24,7 @@
> > >   */
> > >  
> > >  #include <ctype.h>
> > > +#include <getopt.h>
> > >  #include <stdio.h>
> > >  #include <stdint.h>
> > >  #include <stdlib.h>
> > > @@ -836,6 +837,17 @@ static int parse_format(const char *p, int *w, int *h)
> > >  	return 0;
> > >  }
> > >  
> > > +static struct option options[] = {
> > > +	{ "algo", required_argument, NULL, 'a', },
> > > +	{ "brightness", required_argument, NULL, 'b', },
> > > +	{ "format", required_argument, NULL, 'f', },
> > > +	{ "help", no_argument, NULL, 'h', },
> > > +	{ "high-bits", no_argument, NULL, 'g', },
> > > +	{ "size", required_argument, NULL, 's', },
> > > +	{ "swap-rb", no_argument, NULL, 'w', },
> > > +	{ 0 },
> > > +};
> > > +
> > >  int main(int argc, char *argv[])
> > >  {
> > >  	FILE *f;
> > > @@ -849,7 +861,7 @@ int main(int argc, char *argv[])
> > >  	int width = -1;
> > >  
> > >  	for (;;) {
> > > -		int c = getopt(argc, argv, "a:b:f:ghs:w");
> > > +		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
> > >  		if (c==-1) break;
> > >  		switch (c) {
> > >  		case 'a':
> > > @@ -889,14 +901,18 @@ int main(int argc, char *argv[])
> > >  			break;
> > >  		case 'h':
> > >  			printf("%s - Convert headerless raw image to RGB file (PNM)\n"
> > > -			       "Usage: %s [-h] [-w] [-s XxY] <inputfile> <outputfile>\n"
> > > -			       "-a <algo>     Select algorithm, use \"-a ?\" for a list\n"
> > > -			       "-b <bright>   Set brightness (multiplier) to output image (float, default 1.0)\n"
> > > -			       "-f <format>   Specify input file format format (-f ? for list, default UYVY)\n"
> > > -			       "-g            Use high bits for Bayer RAW 10 data\n"
> > > -			       "-h            Show this help\n"
> > > -			       "-s <XxY>      Specify image size\n"
> > > -			       "-w            Swap R and B channels\n", progname, argv[0]);
> > > +			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
> > > +			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
> > > +			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"
> > 
> > That won't scale nicely, but I don't want to implement an options
> > handling framework in C right now :-)
> > 
> > > +			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
> > > +			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
> > > +			       "                          (float, default 1.0)\n"
> > 
> > Do you know if this option is used by anyone ? It seems a bit out of
> > place. Not an issue with this patch of course.
> 
> I have no idea. But keeping it won't do any harm, will it?

It's just an opportunity for cleaning up an unused feature :-) It looks
out of place, as nothing else in raw2rgbpnm performs post-processing on
the converted image (and rightfully so).

> > > +			       "--format, -f <format>     Specify input file format format\n"
> > > +			       "                          (-f ? for list, default UYVY)\n"
> > > +			       "--help, -h                Show this help\n"
> > > +			       "--high-bits, -g           Use high bits for Bayer RAW 10 data\n"
> > > +			       "--size, -s <XxY>          Specify image size\n"
> > > +			       "--swap-rb, -w             Swap R and B channels\n", progname, argv[0]);
> > 
> > I'd add a line break before progname.
> 
> Sounds good.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >  			exit(0);
> > >  		case 's':
> > >  			if (parse_format(optarg, &width, &height) < 0) {

-- 
Regards,

Laurent Pinchart

Re: [raw2rgbpnm PATCH v4 04/11] Support long options and improve help text

[Sakari Ailus]

Hi Laurent,

On Tue, Mar 17, 2026 at 12:49:54PM +0200, Laurent Pinchart wrote:
> On Tue, Mar 17, 2026 at 11:49:14AM +0200, Sakari Ailus wrote:
> > Moi,
> > 
> > On Mon, Mar 16, 2026 at 04:41:51PM +0200, Laurent Pinchart wrote:
> > > On Tue, Mar 10, 2026 at 10:46:10AM +0200, Sakari Ailus wrote:
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  raw2rgbpnm.c | 34 +++++++++++++++++++++++++---------
> > > >  1 file changed, 25 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> > > > index 14cfa7e7a46c..16ef64e984b9 100644
> > > > --- a/raw2rgbpnm.c
> > > > +++ b/raw2rgbpnm.c
> > > > @@ -24,6 +24,7 @@
> > > >   */
> > > >  
> > > >  #include <ctype.h>
> > > > +#include <getopt.h>
> > > >  #include <stdio.h>
> > > >  #include <stdint.h>
> > > >  #include <stdlib.h>
> > > > @@ -836,6 +837,17 @@ static int parse_format(const char *p, int *w, int *h)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static struct option options[] = {
> > > > +	{ "algo", required_argument, NULL, 'a', },
> > > > +	{ "brightness", required_argument, NULL, 'b', },
> > > > +	{ "format", required_argument, NULL, 'f', },
> > > > +	{ "help", no_argument, NULL, 'h', },
> > > > +	{ "high-bits", no_argument, NULL, 'g', },
> > > > +	{ "size", required_argument, NULL, 's', },
> > > > +	{ "swap-rb", no_argument, NULL, 'w', },
> > > > +	{ 0 },
> > > > +};
> > > > +
> > > >  int main(int argc, char *argv[])
> > > >  {
> > > >  	FILE *f;
> > > > @@ -849,7 +861,7 @@ int main(int argc, char *argv[])
> > > >  	int width = -1;
> > > >  
> > > >  	for (;;) {
> > > > -		int c = getopt(argc, argv, "a:b:f:ghs:w");
> > > > +		int c = getopt_long(argc, argv, "a:b:f:ghs:w", options, NULL);
> > > >  		if (c==-1) break;
> > > >  		switch (c) {
> > > >  		case 'a':
> > > > @@ -889,14 +901,18 @@ int main(int argc, char *argv[])
> > > >  			break;
> > > >  		case 'h':
> > > >  			printf("%s - Convert headerless raw image to RGB file (PNM)\n"
> > > > -			       "Usage: %s [-h] [-w] [-s XxY] <inputfile> <outputfile>\n"
> > > > -			       "-a <algo>     Select algorithm, use \"-a ?\" for a list\n"
> > > > -			       "-b <bright>   Set brightness (multiplier) to output image (float, default 1.0)\n"
> > > > -			       "-f <format>   Specify input file format format (-f ? for list, default UYVY)\n"
> > > > -			       "-g            Use high bits for Bayer RAW 10 data\n"
> > > > -			       "-h            Show this help\n"
> > > > -			       "-s <XxY>      Specify image size\n"
> > > > -			       "-w            Swap R and B channels\n", progname, argv[0]);
> > > > +			       "Usage: %s [--algo|-a <algo>] [--brightness|-b <brightness>]\n"
> > > > +			       "       [--format|-f <format>] [--help|-h] [--high-bits|-g] [--size|-s XxY]\n"
> > > > +			       "       [--swap-rb|-w] <inputfile> <outputfile>\n\n"
> > > 
> > > That won't scale nicely, but I don't want to implement an options
> > > handling framework in C right now :-)
> > > 
> > > > +			       "--algo, -a <algo>         Select algorithm, use \"-a ?\" for a list\n"
> > > > +			       "--brightness, -b <bright> Set brightness (multiplier) to output image\n"
> > > > +			       "                          (float, default 1.0)\n"
> > > 
> > > Do you know if this option is used by anyone ? It seems a bit out of
> > > place. Not an issue with this patch of course.
> > 
> > I have no idea. But keeping it won't do any harm, will it?
> 
> It's just an opportunity for cleaning up an unused feature :-) It looks
> out of place, as nothing else in raw2rgbpnm performs post-processing on
> the converted image (and rightfully so).

I think it'd make sense to convert the program to use pixel get/put
functions so the code dealing with actual pixel data wouldn't have to care
about the in-memory format. In that case both will likely be needed.

-- 
Sakari Ailus