| Message ID | 20220711105835.48254-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote: > Provide a 10-bit bayer packing utility for the unpacked data produced > by ipu3-unpack. > > Usage: ipu3-unpack input-file output-file > > The output-file can be "-" to unpack the data to stdout. s/unpack/pack/ > The output file generated by ipu3-pack can be directly fed to IMGU > for streaming. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++ > utils/ipu3/meson.build | 1 + > 2 files changed, 101 insertions(+) > create mode 100644 utils/ipu3/ipu3-pack.c > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > new file mode 100644 > index 00000000..328e3cd6 > --- /dev/null > +++ b/utils/ipu3/ipu3-pack.c > @@ -0,0 +1,100 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer I've commented on the usage message in v1 but overlooked this: * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats > + * > + * Copyright 2022 Umang Jain <umang.jain@ideasonboard.com> > + */ > +#define _GNU_SOURCE > + > +#include <errno.h> > +#include <fcntl.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <unistd.h> > + > +static void usage(const char *argv0) > +{ > + printf("Usage:%s input-file output-file('-' to use stdout)\n", basename(argv0)); > + printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); printf("Usage: %s input-file output-file\n", basename(argv0)); printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); printf("If the output-file '-', output data will be written to standard output\n"); > +} > + > +int main(int argc, char *argv[]) > +{ > + int in_fd; > + int out_fd; > + int ret; > + > + if (argc != 3) { > + usage(argv[0]); > + return 1; > + } > + > + in_fd = open(argv[1], O_RDONLY); > + if (in_fd == -1) { > + fprintf(stderr, "Failed to open input file '%s': %s\n", > + argv[1], strerror(errno)); > + return 1; > + } > + > + if (strcmp(argv[2], "-") == 0) { > + out_fd = STDOUT_FILENO; > + fflush(stdout); Is fflush() needed ? > + } else { > + out_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644); > + if (out_fd == -1) { > + fprintf(stderr, "Failed to open output file '%s': %s\n", > + argv[2], strerror(errno)); close(in_fd); > + return 1; > + } > + } > + > + while (1) { > + uint16_t in_data[25]; > + uint8_t out_data[32]; > + unsigned int i; > + > + int bytes_to_read = sizeof(in_data); s/int/size_t/ But I'd use sizeof(in_data) directly below. Adding a variable adds a level of indirection, which makes the code more difficult to read. > + ret = read(in_fd, in_data, bytes_to_read); > + if (ret == -1) { > + fprintf(stderr, "Failed to read input data: %s\n", > + strerror(errno)); > + goto done; > + } > + > + if (ret < bytes_to_read) { > + if (ret != 0) > + fprintf(stderr, "%u bytes of stray data at end of input\n", > + ret); > + break; I would either 'goto done' here or 'break' in the places where you 'goto done' for consistency. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } > + > + for (i = 0; i < 30; ++i) { > + unsigned int index = (i * 8) / 10; > + unsigned int msb_shift = (i * 8) % 10; > + unsigned int lsb_shift = 10 - msb_shift; > + > + out_data[i] = ((in_data[index] >> msb_shift) & 0xff) > + | ((in_data[index+1] << lsb_shift) & 0xff); > + } > + > + out_data[30] = (in_data[24] >> 0) & 0xff; > + out_data[31] = (in_data[24] >> 8) & 0x03; > + > + ret = write(out_fd, out_data, sizeof(out_data)); > + if (ret < 0) { > + fprintf(stderr, "Failed to write output data: %s\n", > + strerror(errno)); > + goto done; > + } > + } > + > +done: > + close(in_fd); > + if (out_fd != STDOUT_FILENO) > + close(out_fd); > + > + return ret ? 1 : 0; > +} > diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build > index 88049f58..c92cc658 100644 > --- a/utils/ipu3/meson.build > +++ b/utils/ipu3/meson.build > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: CC0-1.0 > > +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c') > ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')
Hi Laurent, On 7/12/22 03:26, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote: >> Provide a 10-bit bayer packing utility for the unpacked data produced >> by ipu3-unpack. >> >> Usage: ipu3-unpack input-file output-file >> >> The output-file can be "-" to unpack the data to stdout. > s/unpack/pack/ > >> The output file generated by ipu3-pack can be directly fed to IMGU >> for streaming. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++ >> utils/ipu3/meson.build | 1 + >> 2 files changed, 101 insertions(+) >> create mode 100644 utils/ipu3/ipu3-pack.c >> >> diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c >> new file mode 100644 >> index 00000000..328e3cd6 >> --- /dev/null >> +++ b/utils/ipu3/ipu3-pack.c >> @@ -0,0 +1,100 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer > I've commented on the usage message in v1 but overlooked this: > > * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats Me too :( > >> + * >> + * Copyright 2022 Umang Jain <umang.jain@ideasonboard.com> >> + */ >> +#define _GNU_SOURCE >> + >> +#include <errno.h> >> +#include <fcntl.h> >> +#include <stdint.h> >> +#include <stdio.h> >> +#include <string.h> >> +#include <sys/stat.h> >> +#include <sys/types.h> >> +#include <unistd.h> >> + >> +static void usage(const char *argv0) >> +{ >> + printf("Usage:%s input-file output-file('-' to use stdout)\n", basename(argv0)); >> + printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); > printf("Usage: %s input-file output-file\n", basename(argv0)); > printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); > printf("If the output-file '-', output data will be written to standard output\n"); > >> +} >> + >> +int main(int argc, char *argv[]) >> +{ >> + int in_fd; >> + int out_fd; >> + int ret; >> + >> + if (argc != 3) { >> + usage(argv[0]); >> + return 1; >> + } >> + >> + in_fd = open(argv[1], O_RDONLY); >> + if (in_fd == -1) { >> + fprintf(stderr, "Failed to open input file '%s': %s\n", >> + argv[1], strerror(errno)); >> + return 1; >> + } >> + >> + if (strcmp(argv[2], "-") == 0) { >> + out_fd = STDOUT_FILENO; >> + fflush(stdout); > Is fflush() needed ? So, to be honest I am bit uncomfortable with writing to stdout really. The descriptor is open always, so I guess any other process can write() to it? Won't it corrupt the packed data if that happens, as it write interleaved output data to the file. So, at least from the starting point, I prefer to fflush()ing the stdout so any buffered data is cleared off to the console, before we start using it for our purpose? Does it makes sense, I might probably have missed something.. > >> + } else { >> + out_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644); >> + if (out_fd == -1) { >> + fprintf(stderr, "Failed to open output file '%s': %s\n", >> + argv[2], strerror(errno)); > close(in_fd); > >> + return 1; >> + } >> + } >> + >> + while (1) { >> + uint16_t in_data[25]; >> + uint8_t out_data[32]; >> + unsigned int i; >> + >> + int bytes_to_read = sizeof(in_data); > s/int/size_t/ > > But I'd use sizeof(in_data) directly below. Adding a variable adds a > level of indirection, which makes the code more difficult to read. > >> + ret = read(in_fd, in_data, bytes_to_read); >> + if (ret == -1) { >> + fprintf(stderr, "Failed to read input data: %s\n", >> + strerror(errno)); >> + goto done; >> + } >> + >> + if (ret < bytes_to_read) { >> + if (ret != 0) >> + fprintf(stderr, "%u bytes of stray data at end of input\n", >> + ret); >> + break; > I would either 'goto done' here or 'break' in the places where you 'goto > done' for consistency. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + } >> + >> + for (i = 0; i < 30; ++i) { >> + unsigned int index = (i * 8) / 10; >> + unsigned int msb_shift = (i * 8) % 10; >> + unsigned int lsb_shift = 10 - msb_shift; >> + >> + out_data[i] = ((in_data[index] >> msb_shift) & 0xff) >> + | ((in_data[index+1] << lsb_shift) & 0xff); >> + } >> + >> + out_data[30] = (in_data[24] >> 0) & 0xff; >> + out_data[31] = (in_data[24] >> 8) & 0x03; >> + >> + ret = write(out_fd, out_data, sizeof(out_data)); >> + if (ret < 0) { >> + fprintf(stderr, "Failed to write output data: %s\n", >> + strerror(errno)); >> + goto done; >> + } >> + } >> + >> +done: >> + close(in_fd); >> + if (out_fd != STDOUT_FILENO) >> + close(out_fd); >> + >> + return ret ? 1 : 0; >> +} >> diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build >> index 88049f58..c92cc658 100644 >> --- a/utils/ipu3/meson.build >> +++ b/utils/ipu3/meson.build >> @@ -1,3 +1,4 @@ >> # SPDX-License-Identifier: CC0-1.0 >> >> +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c') >> ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')
Hi Umang, On Tue, Jul 12, 2022 at 01:07:52PM +0530, Umang Jain wrote: > On 7/12/22 03:26, Laurent Pinchart wrote: > > On Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote: > >> Provide a 10-bit bayer packing utility for the unpacked data produced > >> by ipu3-unpack. > >> > >> Usage: ipu3-unpack input-file output-file > >> > >> The output-file can be "-" to unpack the data to stdout. > > s/unpack/pack/ > > > >> The output file generated by ipu3-pack can be directly fed to IMGU > >> for streaming. > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++ > >> utils/ipu3/meson.build | 1 + > >> 2 files changed, 101 insertions(+) > >> create mode 100644 utils/ipu3/ipu3-pack.c > >> > >> diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > >> new file mode 100644 > >> index 00000000..328e3cd6 > >> --- /dev/null > >> +++ b/utils/ipu3/ipu3-pack.c > >> @@ -0,0 +1,100 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ > >> +/* > >> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer > > > > I've commented on the usage message in v1 but overlooked this: > > > > * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats > > Me too :( > > >> + * > >> + * Copyright 2022 Umang Jain <umang.jain@ideasonboard.com> > >> + */ > >> +#define _GNU_SOURCE > >> + > >> +#include <errno.h> > >> +#include <fcntl.h> > >> +#include <stdint.h> > >> +#include <stdio.h> > >> +#include <string.h> > >> +#include <sys/stat.h> > >> +#include <sys/types.h> > >> +#include <unistd.h> > >> + > >> +static void usage(const char *argv0) > >> +{ > >> + printf("Usage:%s input-file output-file('-' to use stdout)\n", basename(argv0)); > >> + printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); > > > > printf("Usage: %s input-file output-file\n", basename(argv0)); > > printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); > > printf("If the output-file '-', output data will be written to standard output\n"); > > > >> +} > >> + > >> +int main(int argc, char *argv[]) > >> +{ > >> + int in_fd; > >> + int out_fd; > >> + int ret; > >> + > >> + if (argc != 3) { > >> + usage(argv[0]); > >> + return 1; > >> + } > >> + > >> + in_fd = open(argv[1], O_RDONLY); > >> + if (in_fd == -1) { > >> + fprintf(stderr, "Failed to open input file '%s': %s\n", > >> + argv[1], strerror(errno)); > >> + return 1; > >> + } > >> + > >> + if (strcmp(argv[2], "-") == 0) { > >> + out_fd = STDOUT_FILENO; > >> + fflush(stdout); > > > > Is fflush() needed ? > > So, to be honest I am bit uncomfortable with writing to stdout really. > > The descriptor is open always, so I guess any other process can write() > to it? Won't it corrupt the packed data if that happens, as it write > interleaved output data to the file. File descriptors are per process, so no other process can write to stdout of ipu3-pack. > So, at least from the starting point, I prefer to fflush()ing the stdout > so any buffered data is cleared off to the console, before we start > using it for our purpose? Does it makes sense, I might probably have > missed something.. When outputting to stdout, ipu3-pack will likely be used with something similar as ipu3-pack frame.bin - >> frames-packed.bin Flushing stdout for the ipu3-pack process will make no difference, anything that this program writes to stdout will be redirected to the file. > >> + } else { > >> + out_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644); > >> + if (out_fd == -1) { > >> + fprintf(stderr, "Failed to open output file '%s': %s\n", > >> + argv[2], strerror(errno)); > > close(in_fd); > > > >> + return 1; > >> + } > >> + } > >> + > >> + while (1) { > >> + uint16_t in_data[25]; > >> + uint8_t out_data[32]; > >> + unsigned int i; > >> + > >> + int bytes_to_read = sizeof(in_data); > > s/int/size_t/ > > > > But I'd use sizeof(in_data) directly below. Adding a variable adds a > > level of indirection, which makes the code more difficult to read. > > > >> + ret = read(in_fd, in_data, bytes_to_read); > >> + if (ret == -1) { > >> + fprintf(stderr, "Failed to read input data: %s\n", > >> + strerror(errno)); > >> + goto done; > >> + } > >> + > >> + if (ret < bytes_to_read) { > >> + if (ret != 0) > >> + fprintf(stderr, "%u bytes of stray data at end of input\n", > >> + ret); > >> + break; > > I would either 'goto done' here or 'break' in the places where you 'goto > > done' for consistency. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + } > >> + > >> + for (i = 0; i < 30; ++i) { > >> + unsigned int index = (i * 8) / 10; > >> + unsigned int msb_shift = (i * 8) % 10; > >> + unsigned int lsb_shift = 10 - msb_shift; > >> + > >> + out_data[i] = ((in_data[index] >> msb_shift) & 0xff) > >> + | ((in_data[index+1] << lsb_shift) & 0xff); > >> + } > >> + > >> + out_data[30] = (in_data[24] >> 0) & 0xff; > >> + out_data[31] = (in_data[24] >> 8) & 0x03; > >> + > >> + ret = write(out_fd, out_data, sizeof(out_data)); > >> + if (ret < 0) { > >> + fprintf(stderr, "Failed to write output data: %s\n", > >> + strerror(errno)); > >> + goto done; > >> + } > >> + } > >> + > >> +done: > >> + close(in_fd); > >> + if (out_fd != STDOUT_FILENO) > >> + close(out_fd); > >> + > >> + return ret ? 1 : 0; > >> +} > >> diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build > >> index 88049f58..c92cc658 100644 > >> --- a/utils/ipu3/meson.build > >> +++ b/utils/ipu3/meson.build > >> @@ -1,3 +1,4 @@ > >> # SPDX-License-Identifier: CC0-1.0 > >> > >> +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c') > >> ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')
diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c new file mode 100644 index 00000000..328e3cd6 --- /dev/null +++ b/utils/ipu3/ipu3-pack.c @@ -0,0 +1,100 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer + * + * Copyright 2022 Umang Jain <umang.jain@ideasonboard.com> + */ +#define _GNU_SOURCE + +#include <errno.h> +#include <fcntl.h> +#include <stdint.h> +#include <stdio.h> +#include <string.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <unistd.h> + +static void usage(const char *argv0) +{ + printf("Usage:%s input-file output-file('-' to use stdout)\n", basename(argv0)); + printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n"); +} + +int main(int argc, char *argv[]) +{ + int in_fd; + int out_fd; + int ret; + + if (argc != 3) { + usage(argv[0]); + return 1; + } + + in_fd = open(argv[1], O_RDONLY); + if (in_fd == -1) { + fprintf(stderr, "Failed to open input file '%s': %s\n", + argv[1], strerror(errno)); + return 1; + } + + if (strcmp(argv[2], "-") == 0) { + out_fd = STDOUT_FILENO; + fflush(stdout); + } else { + out_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644); + if (out_fd == -1) { + fprintf(stderr, "Failed to open output file '%s': %s\n", + argv[2], strerror(errno)); + return 1; + } + } + + while (1) { + uint16_t in_data[25]; + uint8_t out_data[32]; + unsigned int i; + + int bytes_to_read = sizeof(in_data); + ret = read(in_fd, in_data, bytes_to_read); + if (ret == -1) { + fprintf(stderr, "Failed to read input data: %s\n", + strerror(errno)); + goto done; + } + + if (ret < bytes_to_read) { + if (ret != 0) + fprintf(stderr, "%u bytes of stray data at end of input\n", + ret); + break; + } + + for (i = 0; i < 30; ++i) { + unsigned int index = (i * 8) / 10; + unsigned int msb_shift = (i * 8) % 10; + unsigned int lsb_shift = 10 - msb_shift; + + out_data[i] = ((in_data[index] >> msb_shift) & 0xff) + | ((in_data[index+1] << lsb_shift) & 0xff); + } + + out_data[30] = (in_data[24] >> 0) & 0xff; + out_data[31] = (in_data[24] >> 8) & 0x03; + + ret = write(out_fd, out_data, sizeof(out_data)); + if (ret < 0) { + fprintf(stderr, "Failed to write output data: %s\n", + strerror(errno)); + goto done; + } + } + +done: + close(in_fd); + if (out_fd != STDOUT_FILENO) + close(out_fd); + + return ret ? 1 : 0; +} diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build index 88049f58..c92cc658 100644 --- a/utils/ipu3/meson.build +++ b/utils/ipu3/meson.build @@ -1,3 +1,4 @@ # SPDX-License-Identifier: CC0-1.0 +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c') ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')
Provide a 10-bit bayer packing utility for the unpacked data produced by ipu3-unpack. Usage: ipu3-unpack input-file output-file The output-file can be "-" to unpack the data to stdout. The output file generated by ipu3-pack can be directly fed to IMGU for streaming. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++ utils/ipu3/meson.build | 1 + 2 files changed, 101 insertions(+) create mode 100644 utils/ipu3/ipu3-pack.c