| Message ID | 20250829150804.53498-1-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-08-30 00:08:04) > GPU ISP can produce only 32-bit output. Let's add support to the PPM > writer for XBGR8888 image format so that we can store GPU ISP output as > PPM files. > > There is no obvious performance penalty in my environment compared to > output in the raw format. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/apps/common/ppm_writer.cpp | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp > index 368de8bf6..6c12b11d7 100644 > --- a/src/apps/common/ppm_writer.cpp > +++ b/src/apps/common/ppm_writer.cpp > @@ -10,6 +10,7 @@ > #include <errno.h> > #include <fstream> > #include <iostream> > +#include <vector> > > #include <libcamera/formats.h> > #include <libcamera/pixel_format.h> > @@ -20,9 +21,11 @@ int PPMWriter::write(const char *filename, > const StreamConfiguration &config, > const Span<uint8_t> &data) > { > - if (config.pixelFormat != formats::BGR888) { > - std::cerr << "Only BGR888 output pixel format is supported (" > - << config.pixelFormat << " requested)" << std::endl; > + if (config.pixelFormat != formats::BGR888 && > + config.pixelFormat != formats::XBGR8888) { > + std::cerr > + << "Only BGR888 and XBGR8888 output pixel formats are supported (" > + << config.pixelFormat << " requested)" << std::endl; > return -EINVAL; > } > > @@ -42,8 +45,18 @@ int PPMWriter::write(const char *filename, > > const unsigned int rowLength = config.size.width * 3; > const char *row = reinterpret_cast<const char *>(data.data()); > + const bool transform = config.pixelFormat == formats::XBGR8888; > + std::vector<char> transformedRow(transform ? rowLength : 0); > + > for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) { > - output.write(row, rowLength); > + if (transform) > + for (unsigned int x = 0; x < config.size.width; x++) { > + transformedRow[x * 3] = row[x * 4]; > + transformedRow[x * 3 + 1] = row[x * 4 + 1]; > + transformedRow[x * 3 + 2] = row[x * 4 + 2]; > + } > + > + output.write((transform ? transformedRow.data() : row), rowLength); > if (!output) { > std::cerr << "Failed to write image data at row " << y << std::endl; > return -EIO; > -- > 2.51.0 >
Hi Milan, Quoting Milan Zamazal (2025-08-29 16:08:04) > GPU ISP can produce only 32-bit output. Let's add support to the PPM > writer for XBGR8888 image format so that we can store GPU ISP output as > PPM files. > > There is no obvious performance penalty in my environment compared to > output in the raw format. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/apps/common/ppm_writer.cpp | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp > index 368de8bf6..6c12b11d7 100644 > --- a/src/apps/common/ppm_writer.cpp > +++ b/src/apps/common/ppm_writer.cpp > @@ -10,6 +10,7 @@ > #include <errno.h> > #include <fstream> > #include <iostream> > +#include <vector> > > #include <libcamera/formats.h> > #include <libcamera/pixel_format.h> > @@ -20,9 +21,11 @@ int PPMWriter::write(const char *filename, > const StreamConfiguration &config, > const Span<uint8_t> &data) > { > - if (config.pixelFormat != formats::BGR888) { > - std::cerr << "Only BGR888 output pixel format is supported (" > - << config.pixelFormat << " requested)" << std::endl; > + if (config.pixelFormat != formats::BGR888 && > + config.pixelFormat != formats::XBGR8888) { > + std::cerr > + << "Only BGR888 and XBGR8888 output pixel formats are supported (" > + << config.pixelFormat << " requested)" << std::endl; > return -EINVAL; > } > > @@ -42,8 +45,18 @@ int PPMWriter::write(const char *filename, > > const unsigned int rowLength = config.size.width * 3; > const char *row = reinterpret_cast<const char *>(data.data()); > + const bool transform = config.pixelFormat == formats::XBGR8888; > + std::vector<char> transformedRow(transform ? rowLength : 0); > + > for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) { > - output.write(row, rowLength); > + if (transform) > + for (unsigned int x = 0; x < config.size.width; x++) { > + transformedRow[x * 3] = row[x * 4]; > + transformedRow[x * 3 + 1] = row[x * 4 + 1]; > + transformedRow[x * 3 + 2] = row[x * 4 + 2]; > + } > + If we end up with transformations - perhaps we should support things commonly as Pavel suggested in https://patchwork.libcamera.org/patch/23177/ ? Could you try that patch please? -- Kieran > + output.write((transform ? transformedRow.data() : row), rowLength); > if (!output) { > std::cerr << "Failed to write image data at row " << y << std::endl; > return -EIO; > -- > 2.51.0 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Hi Milan, > > Quoting Milan Zamazal (2025-08-29 16:08:04) >> GPU ISP can produce only 32-bit output. Let's add support to the PPM >> writer for XBGR8888 image format so that we can store GPU ISP output as >> PPM files. >> >> There is no obvious performance penalty in my environment compared to >> output in the raw format. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/apps/common/ppm_writer.cpp | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp >> index 368de8bf6..6c12b11d7 100644 >> --- a/src/apps/common/ppm_writer.cpp >> +++ b/src/apps/common/ppm_writer.cpp >> @@ -10,6 +10,7 @@ >> #include <errno.h> >> #include <fstream> >> #include <iostream> >> +#include <vector> >> >> #include <libcamera/formats.h> >> #include <libcamera/pixel_format.h> >> @@ -20,9 +21,11 @@ int PPMWriter::write(const char *filename, >> const StreamConfiguration &config, >> const Span<uint8_t> &data) >> { >> - if (config.pixelFormat != formats::BGR888) { >> - std::cerr << "Only BGR888 output pixel format is supported (" >> - << config.pixelFormat << " requested)" << std::endl; >> + if (config.pixelFormat != formats::BGR888 && >> + config.pixelFormat != formats::XBGR8888) { >> + std::cerr >> + << "Only BGR888 and XBGR8888 output pixel formats are supported (" >> + << config.pixelFormat << " requested)" << std::endl; >> return -EINVAL; >> } >> >> @@ -42,8 +45,18 @@ int PPMWriter::write(const char *filename, >> >> const unsigned int rowLength = config.size.width * 3; >> const char *row = reinterpret_cast<const char *>(data.data()); >> + const bool transform = config.pixelFormat == formats::XBGR8888; >> + std::vector<char> transformedRow(transform ? rowLength : 0); >> + >> for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) { >> - output.write(row, rowLength); >> + if (transform) >> + for (unsigned int x = 0; x < config.size.width; x++) { >> + transformedRow[x * 3] = row[x * 4]; >> + transformedRow[x * 3 + 1] = row[x * 4 + 1]; >> + transformedRow[x * 3 + 2] = row[x * 4 + 2]; >> + } >> + > > If we end up with transformations - perhaps we should support things > commonly as Pavel suggested in https://patchwork.libcamera.org/patch/23177/ ? Oh, reinventing a wheel here. Indeed, now we have a clear use case for Pavel's patch. > Could you try that patch please? Works for me with GPU ISP. > -- > Kieran > > >> + output.write((transform ? transformedRow.data() : row), rowLength); >> if (!output) { >> std::cerr << "Failed to write image data at row " << y << std::endl; >> return -EIO; >> -- >> 2.51.0 >>
On Sat 2025-08-30 22:25:58, Milan Zamazal wrote: > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Hi Milan, > > > > Quoting Milan Zamazal (2025-08-29 16:08:04) > >> GPU ISP can produce only 32-bit output. Let's add support to the PPM > >> writer for XBGR8888 image format so that we can store GPU ISP output as > >> PPM files. > >> > >> There is no obvious performance penalty in my environment compared to > >> output in the raw format. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/apps/common/ppm_writer.cpp | 21 +++++++++++++++++---- > >> 1 file changed, 17 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp > >> index 368de8bf6..6c12b11d7 100644 > >> --- a/src/apps/common/ppm_writer.cpp > >> +++ b/src/apps/common/ppm_writer.cpp > >> @@ -10,6 +10,7 @@ > >> #include <errno.h> > >> #include <fstream> > >> #include <iostream> > >> +#include <vector> > >> > >> #include <libcamera/formats.h> > >> #include <libcamera/pixel_format.h> > >> @@ -20,9 +21,11 @@ int PPMWriter::write(const char *filename, > >> const StreamConfiguration &config, > >> const Span<uint8_t> &data) > >> { > >> - if (config.pixelFormat != formats::BGR888) { > >> - std::cerr << "Only BGR888 output pixel format is supported (" > >> - << config.pixelFormat << " requested)" << std::endl; > >> + if (config.pixelFormat != formats::BGR888 && > >> + config.pixelFormat != formats::XBGR8888) { > >> + std::cerr > >> + << "Only BGR888 and XBGR8888 output pixel formats are supported (" > >> + << config.pixelFormat << " requested)" << std::endl; > >> return -EINVAL; > >> } > >> > >> @@ -42,8 +45,18 @@ int PPMWriter::write(const char *filename, > >> > >> const unsigned int rowLength = config.size.width * 3; > >> const char *row = reinterpret_cast<const char *>(data.data()); > >> + const bool transform = config.pixelFormat == formats::XBGR8888; > >> + std::vector<char> transformedRow(transform ? rowLength : 0); > >> + > >> for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) { > >> - output.write(row, rowLength); > >> + if (transform) > >> + for (unsigned int x = 0; x < config.size.width; x++) { > >> + transformedRow[x * 3] = row[x * 4]; > >> + transformedRow[x * 3 + 1] = row[x * 4 + 1]; > >> + transformedRow[x * 3 + 2] = row[x * 4 + 2]; > >> + } > >> + > > > > If we end up with transformations - perhaps we should support things > > commonly as Pavel suggested in https://patchwork.libcamera.org/patch/23177/ ? > > Oh, reinventing a wheel here. Indeed, now we have a clear use case for > Pavel's patch. > > > Could you try that patch please? > > Works for me with GPU ISP. I'm currently hacking on clicks machine; I'll be happy if someone picks this up. Best regards, Pavel
Pavel Machek <pavel@ucw.cz> writes: > On Sat 2025-08-30 22:25:58, Milan Zamazal wrote: >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> > >> > Hi Milan, >> > >> > Quoting Milan Zamazal (2025-08-29 16:08:04) >> >> GPU ISP can produce only 32-bit output. Let's add support to the PPM >> >> writer for XBGR8888 image format so that we can store GPU ISP output as >> >> PPM files. >> >> >> >> There is no obvious performance penalty in my environment compared to >> >> output in the raw format. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/apps/common/ppm_writer.cpp | 21 +++++++++++++++++---- >> >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp >> >> index 368de8bf6..6c12b11d7 100644 >> >> --- a/src/apps/common/ppm_writer.cpp >> >> +++ b/src/apps/common/ppm_writer.cpp >> >> @@ -10,6 +10,7 @@ >> >> #include <errno.h> >> >> #include <fstream> >> >> #include <iostream> >> >> +#include <vector> >> >> >> >> #include <libcamera/formats.h> >> >> #include <libcamera/pixel_format.h> >> >> @@ -20,9 +21,11 @@ int PPMWriter::write(const char *filename, >> >> const StreamConfiguration &config, >> >> const Span<uint8_t> &data) >> >> { >> >> - if (config.pixelFormat != formats::BGR888) { >> >> - std::cerr << "Only BGR888 output pixel format is supported (" >> >> - << config.pixelFormat << " requested)" << std::endl; >> >> + if (config.pixelFormat != formats::BGR888 && >> >> + config.pixelFormat != formats::XBGR8888) { >> >> + std::cerr >> >> + << "Only BGR888 and XBGR8888 output pixel formats are supported (" >> >> + << config.pixelFormat << " requested)" << std::endl; >> >> return -EINVAL; >> >> } >> >> >> >> @@ -42,8 +45,18 @@ int PPMWriter::write(const char *filename, >> >> >> >> const unsigned int rowLength = config.size.width * 3; >> >> const char *row = reinterpret_cast<const char *>(data.data()); >> >> + const bool transform = config.pixelFormat == formats::XBGR8888; >> >> + std::vector<char> transformedRow(transform ? rowLength : 0); >> >> + >> >> for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) { >> >> - output.write(row, rowLength); >> >> + if (transform) >> >> + for (unsigned int x = 0; x < config.size.width; x++) { >> >> + transformedRow[x * 3] = row[x * 4]; >> >> + transformedRow[x * 3 + 1] = row[x * 4 + 1]; >> >> + transformedRow[x * 3 + 2] = row[x * 4 + 2]; >> >> + } >> >> + >> > >> > If we end up with transformations - perhaps we should support things >> > commonly as Pavel suggested in https://patchwork.libcamera.org/patch/23177/ ? >> >> Oh, reinventing a wheel here. Indeed, now we have a clear use case for >> Pavel's patch. >> >> > Could you try that patch please? >> >> Works for me with GPU ISP. > > I'm currently hacking on clicks machine; I'll be happy if someone > picks this up. Looking closer, I think I can simply go on with my patch by incorporating the "conversion table" from your patch. In any way, I'll take care of this matter, thanks for your contribution and good luck with clicks machine :-). > Best regards, > Pavel
diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp index 368de8bf6..6c12b11d7 100644 --- a/src/apps/common/ppm_writer.cpp +++ b/src/apps/common/ppm_writer.cpp @@ -10,6 +10,7 @@ #include <errno.h> #include <fstream> #include <iostream> +#include <vector> #include <libcamera/formats.h> #include <libcamera/pixel_format.h> @@ -20,9 +21,11 @@ int PPMWriter::write(const char *filename, const StreamConfiguration &config, const Span<uint8_t> &data) { - if (config.pixelFormat != formats::BGR888) { - std::cerr << "Only BGR888 output pixel format is supported (" - << config.pixelFormat << " requested)" << std::endl; + if (config.pixelFormat != formats::BGR888 && + config.pixelFormat != formats::XBGR8888) { + std::cerr + << "Only BGR888 and XBGR8888 output pixel formats are supported (" + << config.pixelFormat << " requested)" << std::endl; return -EINVAL; } @@ -42,8 +45,18 @@ int PPMWriter::write(const char *filename, const unsigned int rowLength = config.size.width * 3; const char *row = reinterpret_cast<const char *>(data.data()); + const bool transform = config.pixelFormat == formats::XBGR8888; + std::vector<char> transformedRow(transform ? rowLength : 0); + for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) { - output.write(row, rowLength); + if (transform) + for (unsigned int x = 0; x < config.size.width; x++) { + transformedRow[x * 3] = row[x * 4]; + transformedRow[x * 3 + 1] = row[x * 4 + 1]; + transformedRow[x * 3 + 2] = row[x * 4 + 2]; + } + + output.write((transform ? transformedRow.data() : row), rowLength); if (!output) { std::cerr << "Failed to write image data at row " << y << std::endl; return -EIO;
GPU ISP can produce only 32-bit output. Let's add support to the PPM writer for XBGR8888 image format so that we can store GPU ISP output as PPM files. There is no obvious performance penalty in my environment compared to output in the raw format. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/apps/common/ppm_writer.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)