| Message ID | 20260121090705.274081-3-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 01. 21. 10:07 keltezéssel, Umang Jain írta: > Each frame generator can decide what colorspace is deemed fit for their > generated frames. Provide a virtual getter colorspace() and implement it > the TestPatternGenerator and ImageFrameGenerator classes. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > .../pipeline/virtual/frame_generator.h | 3 +++ > .../pipeline/virtual/image_frame_generator.cpp | 9 +++++++++ > .../pipeline/virtual/image_frame_generator.h | 2 ++ > .../virtual/test_pattern_generator.cpp | 18 ++++++++++++++++++ > .../pipeline/virtual/test_pattern_generator.h | 4 ++++ > 5 files changed, 36 insertions(+) > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > index a0658c45..acbf2cc1 100644 > --- a/src/libcamera/pipeline/virtual/frame_generator.h > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > @@ -7,6 +7,7 @@ > > #pragma once > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > > @@ -19,6 +20,8 @@ public: > > virtual void configure(const Size &size) = 0; > > + virtual const ColorSpace colorspace() = 0; `const ColorSpace &` or just `ColorSpace` > + > virtual int generateFrame(const Size &size, > const FrameBuffer *buffer) = 0; > > [...]
On 2026-01-21 15:06, Barnabás Pőcze wrote: > Hi > > > 2026. 01. 21. 10:07 keltezéssel, Umang Jain írta: >> Each frame generator can decide what colorspace is deemed fit for their >> generated frames. Provide a virtual getter colorspace() and implement it >> the TestPatternGenerator and ImageFrameGenerator classes. >> >> Signed-off-by: Umang Jain <uajain@igalia.com> >> --- >> .../pipeline/virtual/frame_generator.h | 3 +++ >> .../pipeline/virtual/image_frame_generator.cpp | 9 +++++++++ >> .../pipeline/virtual/image_frame_generator.h | 2 ++ >> .../virtual/test_pattern_generator.cpp | 18 ++++++++++++++++++ >> .../pipeline/virtual/test_pattern_generator.h | 4 ++++ >> 5 files changed, 36 insertions(+) >> >> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h >> index a0658c45..acbf2cc1 100644 >> --- a/src/libcamera/pipeline/virtual/frame_generator.h >> +++ b/src/libcamera/pipeline/virtual/frame_generator.h >> @@ -7,6 +7,7 @@ >> #pragma once >> +#include <libcamera/color_space.h> >> #include <libcamera/framebuffer.h> >> #include <libcamera/geometry.h> >> @@ -19,6 +20,8 @@ public: >> virtual void configure(const Size &size) = 0; >> + virtual const ColorSpace colorspace() = 0; > > `const ColorSpace &` or just `ColorSpace` Ack, thanks for noticing, Fix for using just 'ColorSpace' applied locally. > > >> + >> virtual int generateFrame(const Size &size, >> const FrameBuffer *buffer) = 0; >> [...]
On Wed, Jan 21, 2026 at 02:37:04PM +0530, Umang Jain wrote: > Each frame generator can decide what colorspace is deemed fit for their It's not about what colorspace "is deemed fit", but about what colorspace is used to generate the images. I'd write Each frame generator knows what colorspace is used for the frames it produces. > generated frames. Provide a virtual getter colorspace() and implement it > the TestPatternGenerator and ImageFrameGenerator classes. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > .../pipeline/virtual/frame_generator.h | 3 +++ > .../pipeline/virtual/image_frame_generator.cpp | 9 +++++++++ > .../pipeline/virtual/image_frame_generator.h | 2 ++ > .../virtual/test_pattern_generator.cpp | 18 ++++++++++++++++++ > .../pipeline/virtual/test_pattern_generator.h | 4 ++++ > 5 files changed, 36 insertions(+) > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > index a0658c45..acbf2cc1 100644 > --- a/src/libcamera/pipeline/virtual/frame_generator.h > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > @@ -7,6 +7,7 @@ > > #pragma once > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > > @@ -19,6 +20,8 @@ public: > > virtual void configure(const Size &size) = 0; > > + virtual const ColorSpace colorspace() = 0; > + > virtual int generateFrame(const Size &size, > const FrameBuffer *buffer) = 0; > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index d1545b5d..bab3cfdc 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -149,6 +149,15 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > return 0; > } > > +const ColorSpace ImageFrameGenerator::colorspace() > +{ > + /* > + * libyuv ensures sYCC colorspace of frames during MJPGToNV12() > + * conversion. Does it ? Looking at the source code, I don't see that. > + */ > + return ColorSpace::Sycc; > +} > + > /* > * \var ImageFrameGenerator::imageFrameDatas_ > * \brief List of pointers to the not scaled image buffers > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > index 851ddbc0..24c362e6 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > @@ -13,6 +13,7 @@ > #include <sys/types.h> > #include <vector> > > +#include <libcamera/color_space.h> Drop this, see 1/3. Same in test_pattern_generator.h. > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > > @@ -41,6 +42,7 @@ private: > > void configure(const Size &size) override; > int generateFrame(const Size &size, const FrameBuffer *buffer) override; > + const ColorSpace colorspace() override; > > std::vector<ImageFrameData> imageFrameDatas_; > std::vector<ImageFrameData> scaledFrameDatas_; > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 745be83b..04efe6cc 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -62,6 +62,24 @@ int TestPatternGenerator::generateFrame(const Size &size, > return ret; > } > > +/* > + * libyuv internally converts ARGB<>YUV following the BT.601 colorspace. > + * The corresponding YCbCr encoding is ColorSpace::YcbcrEncoding::Rec601 > + * with limited range. > + * > + * Since the test patterns generation occurs in RGB, transfer function is set > + * to ColorSpace::TransferFunction::Srgb. Color primaries is assumed > + * ColorSpace::Primaries::Rec709 for the RGB test patterns. Why ? That seems to be a random pick. > + */ > +const ColorSpace TestPatternGenerator::colorspace() > +{ > + ColorSpace colorspace{ ColorSpace::Primaries::Rec709, > + ColorSpace::TransferFunction::Srgb, > + ColorSpace::YcbcrEncoding::Rec601, > + ColorSpace::Range::Limited }; > + return colorspace; > +} > + > void ColorBarsGenerator::configure(const Size &size) > { > constexpr uint8_t kColorBar[8][3] = { > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h > index 2a51bd31..fb2de65f 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > @@ -9,6 +9,7 @@ > > #include <memory> > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > > @@ -29,6 +30,9 @@ public: > protected: > /* Buffer of test pattern template */ > std::unique_ptr<uint8_t[]> template_; > + > +private: > + const ColorSpace colorspace() override; Why private ? > }; > > class ColorBarsGenerator : public TestPatternGenerator
On Thu, Jan 22, 2026 at 02:00:35AM +0200, Laurent Pinchart wrote: > On Wed, Jan 21, 2026 at 02:37:04PM +0530, Umang Jain wrote: > > Each frame generator can decide what colorspace is deemed fit for their > > It's not about what colorspace "is deemed fit", but about what > colorspace is used to generate the images. I'd write > > Each frame generator knows what colorspace is used for the frames it > produces. > > > generated frames. Provide a virtual getter colorspace() and implement it > > the TestPatternGenerator and ImageFrameGenerator classes. > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > --- > > .../pipeline/virtual/frame_generator.h | 3 +++ > > .../pipeline/virtual/image_frame_generator.cpp | 9 +++++++++ > > .../pipeline/virtual/image_frame_generator.h | 2 ++ > > .../virtual/test_pattern_generator.cpp | 18 ++++++++++++++++++ > > .../pipeline/virtual/test_pattern_generator.h | 4 ++++ > > 5 files changed, 36 insertions(+) > > > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > > index a0658c45..acbf2cc1 100644 > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > @@ -7,6 +7,7 @@ > > > > #pragma once > > > > +#include <libcamera/color_space.h> > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > > > @@ -19,6 +20,8 @@ public: > > > > virtual void configure(const Size &size) = 0; > > > > + virtual const ColorSpace colorspace() = 0; > > + > > virtual int generateFrame(const Size &size, > > const FrameBuffer *buffer) = 0; > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > index d1545b5d..bab3cfdc 100644 > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -149,6 +149,15 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > return 0; > > } > > > > +const ColorSpace ImageFrameGenerator::colorspace() > > +{ > > + /* > > + * libyuv ensures sYCC colorspace of frames during MJPGToNV12() > > + * conversion. > > Does it ? Looking at the source code, I don't see that. > > > + */ > > + return ColorSpace::Sycc; > > +} > > + > > /* > > * \var ImageFrameGenerator::imageFrameDatas_ > > * \brief List of pointers to the not scaled image buffers > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > > index 851ddbc0..24c362e6 100644 > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > @@ -13,6 +13,7 @@ > > #include <sys/types.h> > > #include <vector> > > > > +#include <libcamera/color_space.h> > > Drop this, see 1/3. Same in test_pattern_generator.h. > > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > > > @@ -41,6 +42,7 @@ private: > > > > void configure(const Size &size) override; > > int generateFrame(const Size &size, const FrameBuffer *buffer) override; > > + const ColorSpace colorspace() override; > > > > std::vector<ImageFrameData> imageFrameDatas_; > > std::vector<ImageFrameData> scaledFrameDatas_; > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > index 745be83b..04efe6cc 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > @@ -62,6 +62,24 @@ int TestPatternGenerator::generateFrame(const Size &size, > > return ret; > > } > > > > +/* > > + * libyuv internally converts ARGB<>YUV following the BT.601 colorspace. > > + * The corresponding YCbCr encoding is ColorSpace::YcbcrEncoding::Rec601 > > + * with limited range. > > + * > > + * Since the test patterns generation occurs in RGB, transfer function is set > > + * to ColorSpace::TransferFunction::Srgb. Color primaries is assumed > > + * ColorSpace::Primaries::Rec709 for the RGB test patterns. > > Why ? That seems to be a random pick. I should have read the cover letter before reviewing this patch, as you explained there that you don't know why. Looking at the test pattern generator, the transfer function seems irrelevant. All the colours used by the TPG are fully saturated (R, G and B are either 0.0 or 1.0). All transfer functions map 0.0 to 0.0 and 1.0 to 1.0. If we wanted a test pattern with non-saturated values (a linear gradient being a good example), we would need to pick and apply a transfer function. We can therefore select any transfer function we want, and I think we should pick one that creates a standard colorspace. The colour primaries are similarly irrelevant. They tell what red, green and blue mean. As this is a test pattern, the question doesn't make sense, the pixels we output are not meant to represent a specific colour. We can select any colour primaries, and should also make a pick that creates a standard colorspace. > > + */ > > +const ColorSpace TestPatternGenerator::colorspace() > > +{ > > + ColorSpace colorspace{ ColorSpace::Primaries::Rec709, > > + ColorSpace::TransferFunction::Srgb, > > + ColorSpace::YcbcrEncoding::Rec601, > > + ColorSpace::Range::Limited }; > > + return colorspace; > > +} > > + > > void ColorBarsGenerator::configure(const Size &size) > > { > > constexpr uint8_t kColorBar[8][3] = { > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > index 2a51bd31..fb2de65f 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > @@ -9,6 +9,7 @@ > > > > #include <memory> > > > > +#include <libcamera/color_space.h> > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > > > @@ -29,6 +30,9 @@ public: > > protected: > > /* Buffer of test pattern template */ > > std::unique_ptr<uint8_t[]> template_; > > + > > +private: > > + const ColorSpace colorspace() override; > > Why private ? > > > }; > > > > class ColorBarsGenerator : public TestPatternGenerator
diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h index a0658c45..acbf2cc1 100644 --- a/src/libcamera/pipeline/virtual/frame_generator.h +++ b/src/libcamera/pipeline/virtual/frame_generator.h @@ -7,6 +7,7 @@ #pragma once +#include <libcamera/color_space.h> #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> @@ -19,6 +20,8 @@ public: virtual void configure(const Size &size) = 0; + virtual const ColorSpace colorspace() = 0; + virtual int generateFrame(const Size &size, const FrameBuffer *buffer) = 0; diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp index d1545b5d..bab3cfdc 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -149,6 +149,15 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff return 0; } +const ColorSpace ImageFrameGenerator::colorspace() +{ + /* + * libyuv ensures sYCC colorspace of frames during MJPGToNV12() + * conversion. + */ + return ColorSpace::Sycc; +} + /* * \var ImageFrameGenerator::imageFrameDatas_ * \brief List of pointers to the not scaled image buffers diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h index 851ddbc0..24c362e6 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.h +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h @@ -13,6 +13,7 @@ #include <sys/types.h> #include <vector> +#include <libcamera/color_space.h> #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> @@ -41,6 +42,7 @@ private: void configure(const Size &size) override; int generateFrame(const Size &size, const FrameBuffer *buffer) override; + const ColorSpace colorspace() override; std::vector<ImageFrameData> imageFrameDatas_; std::vector<ImageFrameData> scaledFrameDatas_; diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp index 745be83b..04efe6cc 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp @@ -62,6 +62,24 @@ int TestPatternGenerator::generateFrame(const Size &size, return ret; } +/* + * libyuv internally converts ARGB<>YUV following the BT.601 colorspace. + * The corresponding YCbCr encoding is ColorSpace::YcbcrEncoding::Rec601 + * with limited range. + * + * Since the test patterns generation occurs in RGB, transfer function is set + * to ColorSpace::TransferFunction::Srgb. Color primaries is assumed + * ColorSpace::Primaries::Rec709 for the RGB test patterns. + */ +const ColorSpace TestPatternGenerator::colorspace() +{ + ColorSpace colorspace{ ColorSpace::Primaries::Rec709, + ColorSpace::TransferFunction::Srgb, + ColorSpace::YcbcrEncoding::Rec601, + ColorSpace::Range::Limited }; + return colorspace; +} + void ColorBarsGenerator::configure(const Size &size) { constexpr uint8_t kColorBar[8][3] = { diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h index 2a51bd31..fb2de65f 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h @@ -9,6 +9,7 @@ #include <memory> +#include <libcamera/color_space.h> #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> @@ -29,6 +30,9 @@ public: protected: /* Buffer of test pattern template */ std::unique_ptr<uint8_t[]> template_; + +private: + const ColorSpace colorspace() override; }; class ColorBarsGenerator : public TestPatternGenerator
Each frame generator can decide what colorspace is deemed fit for their generated frames. Provide a virtual getter colorspace() and implement it the TestPatternGenerator and ImageFrameGenerator classes. Signed-off-by: Umang Jain <uajain@igalia.com> --- .../pipeline/virtual/frame_generator.h | 3 +++ .../pipeline/virtual/image_frame_generator.cpp | 9 +++++++++ .../pipeline/virtual/image_frame_generator.h | 2 ++ .../virtual/test_pattern_generator.cpp | 18 ++++++++++++++++++ .../pipeline/virtual/test_pattern_generator.h | 4 ++++ 5 files changed, 36 insertions(+)