| Message ID | 20250901141643.29136-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Mon, Sep 01, 2025 at 04:16:42PM +0200, Milan Zamazal wrote: > GPU ISP can produce only 32-bit output. Let's add support to the PPM > writer for all the common RGB image formats so that we can store GPU ISP > output as PPM files. Contingent alpha values are ignored as there is no > support for the alpha channel in PPM. > > There is no obvious performance penalty in my environment compared to > output in the raw format. > > Co-developed-by: Pavel Machek <pavel@ucw.cz> You need a SoB line from Pavel here. > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/apps/common/ppm_writer.cpp | 66 ++++++++++++++++++++++++++++++++-- > 1 file changed, 63 insertions(+), 3 deletions(-) > > diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp > index 368de8bf6..9ba0216a0 100644 > --- a/src/apps/common/ppm_writer.cpp > +++ b/src/apps/common/ppm_writer.cpp > @@ -10,6 +10,8 @@ > #include <errno.h> > #include <fstream> > #include <iostream> > +#include <stddef.h> > +#include <vector> > > #include <libcamera/formats.h> > #include <libcamera/pixel_format.h> > @@ -20,8 +22,56 @@ 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 (" > + size_t rPos, gPos, bPos, bytesPerPixel; > + switch (config.pixelFormat) { > + case libcamera::formats::R8: > + rPos = 0; > + gPos = 0; > + bPos = 0; > + bytesPerPixel = 1; > + break; > + case libcamera::formats::RGB888: > + rPos = 2; > + gPos = 1; > + bPos = 0; > + bytesPerPixel = 3; > + break; > + case libcamera::formats::BGR888: > + rPos = 0; > + gPos = 1; > + bPos = 2; > + bytesPerPixel = 3; > + break; > + case libcamera::formats::ARGB8888: > + case libcamera::formats::XRGB8888: > + rPos = 2; > + gPos = 1; > + bPos = 0; > + bytesPerPixel = 4; > + break; > + case libcamera::formats::RGBA8888: > + case libcamera::formats::RGBX8888: > + rPos = 3; > + gPos = 2; > + bPos = 1; > + bytesPerPixel = 4; > + break; > + case libcamera::formats::ABGR8888: > + case libcamera::formats::XBGR8888: > + rPos = 0; > + gPos = 1; > + bPos = 2; > + bytesPerPixel = 4; > + break; > + case libcamera::formats::BGRA8888: > + case libcamera::formats::BGRX8888: > + rPos = 1; > + gPos = 2; > + bPos = 3; > + bytesPerPixel = 4; > + break; How about a static const map indexed by format ? Something like struct FormatTransformation { unsigned int rPos; unsigned int gPos; unsigned int bPos; unsigned int bytesPerPixel; }; static const std::map<libcamera::PixelFormat> transforms = { { libcamera::formats::R8, { 0, 0, 0, 1 } }, ... }; > + default: > + std::cerr << "Only RGB output pixel formats are supported (" > << config.pixelFormat << " requested)" << std::endl; > return -EINVAL; > } > @@ -42,8 +92,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::BGR888; > + 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 * bytesPerPixel + rPos]; > + transformedRow[x * 3 + 1] = row[x * bytesPerPixel + gPos]; > + transformedRow[x * 3 + 2] = row[x * bytesPerPixel + bPos]; > + } Missing outer curly brackets. > + > + output.write((transform ? transformedRow.data() : row), rowLength); No need for inner parentheses :-) > if (!output) { > std::cerr << "Failed to write image data at row " << y << std::endl; > return -EIO;
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Mon, Sep 01, 2025 at 04:16:42PM +0200, Milan Zamazal wrote: >> GPU ISP can produce only 32-bit output. Let's add support to the PPM >> writer for all the common RGB image formats so that we can store GPU ISP >> output as PPM files. Contingent alpha values are ignored as there is no >> support for the alpha channel in PPM. >> >> There is no obvious performance penalty in my environment compared to >> output in the raw format. >> >> Co-developed-by: Pavel Machek <pavel@ucw.cz> > > You need a SoB line from Pavel here. Pavel, can you provide one? (Or can I take the one from https://patchwork.libcamera.org/patch/23177/ ?) >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/apps/common/ppm_writer.cpp | 66 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 63 insertions(+), 3 deletions(-) >> >> diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp >> index 368de8bf6..9ba0216a0 100644 >> --- a/src/apps/common/ppm_writer.cpp >> +++ b/src/apps/common/ppm_writer.cpp >> @@ -10,6 +10,8 @@ >> #include <errno.h> >> #include <fstream> >> #include <iostream> >> +#include <stddef.h> >> +#include <vector> >> >> #include <libcamera/formats.h> >> #include <libcamera/pixel_format.h> >> @@ -20,8 +22,56 @@ 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 (" >> + size_t rPos, gPos, bPos, bytesPerPixel; >> + switch (config.pixelFormat) { >> + case libcamera::formats::R8: >> + rPos = 0; >> + gPos = 0; >> + bPos = 0; >> + bytesPerPixel = 1; >> + break; >> + case libcamera::formats::RGB888: >> + rPos = 2; >> + gPos = 1; >> + bPos = 0; >> + bytesPerPixel = 3; >> + break; >> + case libcamera::formats::BGR888: >> + rPos = 0; >> + gPos = 1; >> + bPos = 2; >> + bytesPerPixel = 3; >> + break; >> + case libcamera::formats::ARGB8888: >> + case libcamera::formats::XRGB8888: >> + rPos = 2; >> + gPos = 1; >> + bPos = 0; >> + bytesPerPixel = 4; >> + break; >> + case libcamera::formats::RGBA8888: >> + case libcamera::formats::RGBX8888: >> + rPos = 3; >> + gPos = 2; >> + bPos = 1; >> + bytesPerPixel = 4; >> + break; >> + case libcamera::formats::ABGR8888: >> + case libcamera::formats::XBGR8888: >> + rPos = 0; >> + gPos = 1; >> + bPos = 2; >> + bytesPerPixel = 4; >> + break; >> + case libcamera::formats::BGRA8888: >> + case libcamera::formats::BGRX8888: >> + rPos = 1; >> + gPos = 2; >> + bPos = 3; >> + bytesPerPixel = 4; >> + break; > > How about a static const map indexed by format ? Something like > > struct FormatTransformation { > unsigned int rPos; > unsigned int gPos; > unsigned int bPos; > unsigned int bytesPerPixel; > }; > > static const std::map<libcamera::PixelFormat> transforms = { > { libcamera::formats::R8, { 0, 0, 0, 1 } }, > ... > }; Yes, looks better, I'll change it in v3. >> + default: >> + std::cerr << "Only RGB output pixel formats are supported (" >> << config.pixelFormat << " requested)" << std::endl; >> return -EINVAL; >> } >> @@ -42,8 +92,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::BGR888; >> + 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 * bytesPerPixel + rPos]; >> + transformedRow[x * 3 + 1] = row[x * bytesPerPixel + gPos]; >> + transformedRow[x * 3 + 2] = row[x * bytesPerPixel + bPos]; >> + } > > Missing outer curly brackets. OK. >> + >> + output.write((transform ? transformedRow.data() : row), rowLength); > > No need for inner parentheses :-) Damned infix languages. :-) >> if (!output) { >> std::cerr << "Failed to write image data at row " << y << std::endl; >> return -EIO;
Hi! > > Thank you for the patch. > > > > On Mon, Sep 01, 2025 at 04:16:42PM +0200, Milan Zamazal wrote: > >> GPU ISP can produce only 32-bit output. Let's add support to the PPM > >> writer for all the common RGB image formats so that we can store GPU ISP > >> output as PPM files. Contingent alpha values are ignored as there is no > >> support for the alpha channel in PPM. > >> > >> There is no obvious performance penalty in my environment compared to > >> output in the raw format. > >> > >> Co-developed-by: Pavel Machek <pavel@ucw.cz> > > > > You need a SoB line from Pavel here. I believe it should not be needed, since this is different from my code, > Pavel, can you provide one? > > (Or can I take the one from https://patchwork.libcamera.org/patch/23177/ ?) but I don't care deeply. Signed-off-by: Pavel Machek <pavel@ucw.cz> And... thanks for taking this forward! Best regards, Pavel
diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp index 368de8bf6..9ba0216a0 100644 --- a/src/apps/common/ppm_writer.cpp +++ b/src/apps/common/ppm_writer.cpp @@ -10,6 +10,8 @@ #include <errno.h> #include <fstream> #include <iostream> +#include <stddef.h> +#include <vector> #include <libcamera/formats.h> #include <libcamera/pixel_format.h> @@ -20,8 +22,56 @@ 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 (" + size_t rPos, gPos, bPos, bytesPerPixel; + switch (config.pixelFormat) { + case libcamera::formats::R8: + rPos = 0; + gPos = 0; + bPos = 0; + bytesPerPixel = 1; + break; + case libcamera::formats::RGB888: + rPos = 2; + gPos = 1; + bPos = 0; + bytesPerPixel = 3; + break; + case libcamera::formats::BGR888: + rPos = 0; + gPos = 1; + bPos = 2; + bytesPerPixel = 3; + break; + case libcamera::formats::ARGB8888: + case libcamera::formats::XRGB8888: + rPos = 2; + gPos = 1; + bPos = 0; + bytesPerPixel = 4; + break; + case libcamera::formats::RGBA8888: + case libcamera::formats::RGBX8888: + rPos = 3; + gPos = 2; + bPos = 1; + bytesPerPixel = 4; + break; + case libcamera::formats::ABGR8888: + case libcamera::formats::XBGR8888: + rPos = 0; + gPos = 1; + bPos = 2; + bytesPerPixel = 4; + break; + case libcamera::formats::BGRA8888: + case libcamera::formats::BGRX8888: + rPos = 1; + gPos = 2; + bPos = 3; + bytesPerPixel = 4; + break; + default: + std::cerr << "Only RGB output pixel formats are supported (" << config.pixelFormat << " requested)" << std::endl; return -EINVAL; } @@ -42,8 +92,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::BGR888; + 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 * bytesPerPixel + rPos]; + transformedRow[x * 3 + 1] = row[x * bytesPerPixel + gPos]; + transformedRow[x * 3 + 2] = row[x * bytesPerPixel + bPos]; + } + + 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 all the common RGB image formats so that we can store GPU ISP output as PPM files. Contingent alpha values are ignored as there is no support for the alpha channel in PPM. There is no obvious performance penalty in my environment compared to output in the raw format. Co-developed-by: Pavel Machek <pavel@ucw.cz> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/apps/common/ppm_writer.cpp | 66 ++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 3 deletions(-)