Message ID | 20240930152039.72459-2-umang.jain@ideasonboard.com |
---|---|
State | Rejected |
Headers | show |
Series |
|
Related | show |
Hi Umang On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote: > Add a isRaw() helper to the PixelFormat class, to know whether the > pixel format has RAW encoding. > > This will used by validation and configuration code paths in pipeline > handlers, to know whether a pixel format is a raw format or not. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Do you know why RPi does instead bool PipelineHandlerBase::isRaw(const PixelFormat &pixFmt) { /* This test works for both Bayer and raw mono formats. */ return BayerFormat::fromPixelFormat(pixFmt).isValid(); } cc RPi which has upstreamed BayerFormat to know if there's a specfic reason why they do this instead of using PixelFormatInfo. Seems like we have two maps of raw formants, one part of the canonical PixelFormatInfo and one in BayerFormat. They store more or less the same information, BayerFormat has a 'packing' field that PixelFormatInfo doesn't have, but I wonder why we have duplicated this instead of properly extending PixelFormatInfo PixelFormatInfo: { formats::RGGB_PISP_COMP1, { .name = "RGGB_PISP_COMP1", .format = formats::RGGB_PISP_COMP1, .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), }, .bitsPerPixel = 8, .colourEncoding = PixelFormatInfo::ColourEncodingRAW, .packed = true, .pixelsPerGroup = 2, .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }}, } }, BayerFormat: { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 }, { formats::RGGB_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } }, True, BayerFormat can be constructed from an mbusCode (something that has proven to be problematic in the past, as multiple codes map to the same BayerFormat), I wonder if that's the main reason. > --- > include/libcamera/pixel_format.h | 1 + > src/libcamera/pixel_format.cpp | 11 +++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h > index 1b4d8c7c..aed53ea6 100644 > --- a/include/libcamera/pixel_format.h > +++ b/include/libcamera/pixel_format.h > @@ -37,6 +37,7 @@ public: > constexpr uint64_t modifier() const { return modifier_; } > > std::string toString() const; > + bool isRaw() const; > > static PixelFormat fromString(const std::string &name); > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp > index 314179a8..436ef5fb 100644 > --- a/src/libcamera/pixel_format.cpp > +++ b/src/libcamera/pixel_format.cpp > @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const > * \return DRM modifier > */ > > +/** > + * \brief Checks if \a this is a RAW pixel format > + * \return True if \a this is a RAW pixel format, false otherwise > + */ > +bool PixelFormat::isRaw() const > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(*this); > + > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > +} > + > /** > * \brief Assemble and return a string describing the pixel format > * \return A string describing the pixel format > -- > 2.45.2 >
Hi Jacopo, You read my mind - I was going to respond to this next. On Tue, 1 Oct 2024 at 08:48, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Umang > > On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote: > > Add a isRaw() helper to the PixelFormat class, to know whether the > > pixel format has RAW encoding. > > > > This will used by validation and configuration code paths in pipeline > > handlers, to know whether a pixel format is a raw format or not. > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > Do you know why RPi does instead > > bool PipelineHandlerBase::isRaw(const PixelFormat &pixFmt) > { > /* This test works for both Bayer and raw mono formats. */ > return BayerFormat::fromPixelFormat(pixFmt).isValid(); > } > > cc RPi which has upstreamed BayerFormat to know if there's a specfic reason > why they do this instead of using PixelFormatInfo. I assume RAW in this case means Bayer right? I think looking at colourEncoding == ColourEncodingRAW might be incomplete as mono formats (correctly) set colourEncoding to ColourEncodingYUV. Regards, Naush > > Seems like we have two maps of raw formants, one part of the canonical > PixelFormatInfo and one in BayerFormat. They store more or less the > same information, BayerFormat has a 'packing' field that > PixelFormatInfo doesn't have, but I wonder why we have duplicated > this instead of properly extending PixelFormatInfo > > PixelFormatInfo: > > { formats::RGGB_PISP_COMP1, { > .name = "RGGB_PISP_COMP1", > .format = formats::RGGB_PISP_COMP1, > .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), }, > .bitsPerPixel = 8, > .colourEncoding = PixelFormatInfo::ColourEncodingRAW, > .packed = true, > .pixelsPerGroup = 2, > .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }}, > } }, > > BayerFormat: > { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 }, > { formats::RGGB_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } }, > > True, BayerFormat can be constructed from an mbusCode (something that > has proven to be problematic in the past, as multiple codes map to the > same BayerFormat), I wonder if that's the main reason. > > > --- > > include/libcamera/pixel_format.h | 1 + > > src/libcamera/pixel_format.cpp | 11 +++++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h > > index 1b4d8c7c..aed53ea6 100644 > > --- a/include/libcamera/pixel_format.h > > +++ b/include/libcamera/pixel_format.h > > @@ -37,6 +37,7 @@ public: > > constexpr uint64_t modifier() const { return modifier_; } > > > > std::string toString() const; > > + bool isRaw() const; > > > > static PixelFormat fromString(const std::string &name); > > > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp > > index 314179a8..436ef5fb 100644 > > --- a/src/libcamera/pixel_format.cpp > > +++ b/src/libcamera/pixel_format.cpp > > @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const > > * \return DRM modifier > > */ > > > > +/** > > + * \brief Checks if \a this is a RAW pixel format > > + * \return True if \a this is a RAW pixel format, false otherwise > > + */ > > +bool PixelFormat::isRaw() const > > +{ > > + const PixelFormatInfo &info = PixelFormatInfo::info(*this); > > + > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > > +} > > + > > /** > > * \brief Assemble and return a string describing the pixel format > > * \return A string describing the pixel format > > -- > > 2.45.2 > >
Hi Naush On Tue, Oct 01, 2024 at 08:53:09AM GMT, Naushir Patuck wrote: > Hi Jacopo, > > You read my mind - I was going to respond to this next. > > On Tue, 1 Oct 2024 at 08:48, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Umang > > > > On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote: > > > Add a isRaw() helper to the PixelFormat class, to know whether the > > > pixel format has RAW encoding. > > > > > > This will used by validation and configuration code paths in pipeline > > > handlers, to know whether a pixel format is a raw format or not. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > Do you know why RPi does instead > > > > bool PipelineHandlerBase::isRaw(const PixelFormat &pixFmt) > > { > > /* This test works for both Bayer and raw mono formats. */ > > return BayerFormat::fromPixelFormat(pixFmt).isValid(); > > } > > > > cc RPi which has upstreamed BayerFormat to know if there's a specfic reason > > why they do this instead of using PixelFormatInfo. > > I assume RAW in this case means Bayer right? > > I think looking at colourEncoding == ColourEncodingRAW might be incomplete as > mono formats (correctly) set colourEncoding to ColourEncodingYUV. Ok, thanks for clarifying it. We should then replace all current usages of colourEncoding == ColourEncodingRAW and leave BayerFormat be. That's what Umang did already and there won't be regressions introduced this way. > > Regards, > Naush > > > > > Seems like we have two maps of raw formants, one part of the canonical > > PixelFormatInfo and one in BayerFormat. They store more or less the > > same information, BayerFormat has a 'packing' field that > > PixelFormatInfo doesn't have, but I wonder why we have duplicated > > this instead of properly extending PixelFormatInfo > > > > PixelFormatInfo: > > > > { formats::RGGB_PISP_COMP1, { > > .name = "RGGB_PISP_COMP1", > > .format = formats::RGGB_PISP_COMP1, > > .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), }, > > .bitsPerPixel = 8, > > .colourEncoding = PixelFormatInfo::ColourEncodingRAW, > > .packed = true, > > .pixelsPerGroup = 2, > > .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }}, > > } }, > > > > BayerFormat: > > { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 }, > > { formats::RGGB_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } }, > > > > True, BayerFormat can be constructed from an mbusCode (something that > > has proven to be problematic in the past, as multiple codes map to the > > same BayerFormat), I wonder if that's the main reason. > > > > > --- > > > include/libcamera/pixel_format.h | 1 + > > > src/libcamera/pixel_format.cpp | 11 +++++++++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h > > > index 1b4d8c7c..aed53ea6 100644 > > > --- a/include/libcamera/pixel_format.h > > > +++ b/include/libcamera/pixel_format.h > > > @@ -37,6 +37,7 @@ public: > > > constexpr uint64_t modifier() const { return modifier_; } > > > > > > std::string toString() const; > > > + bool isRaw() const; > > > > > > static PixelFormat fromString(const std::string &name); > > > > > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp > > > index 314179a8..436ef5fb 100644 > > > --- a/src/libcamera/pixel_format.cpp > > > +++ b/src/libcamera/pixel_format.cpp > > > @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const > > > * \return DRM modifier > > > */ > > > > > > +/** > > > + * \brief Checks if \a this is a RAW pixel format > > > + * \return True if \a this is a RAW pixel format, false otherwise > > > + */ > > > +bool PixelFormat::isRaw() const > > > +{ > > > + const PixelFormatInfo &info = PixelFormatInfo::info(*this); > > > + > > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > > > +} > > > + > > > /** > > > * \brief Assemble and return a string describing the pixel format > > > * \return A string describing the pixel format > > > -- > > > 2.45.2 > > >
Hi Umang On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote: > Add a isRaw() helper to the PixelFormat class, to know whether the > pixel format has RAW encoding. > > This will used by validation and configuration code paths in pipeline > handlers, to know whether a pixel format is a raw format or not. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/pixel_format.h | 1 + > src/libcamera/pixel_format.cpp | 11 +++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h > index 1b4d8c7c..aed53ea6 100644 > --- a/include/libcamera/pixel_format.h > +++ b/include/libcamera/pixel_format.h > @@ -37,6 +37,7 @@ public: > constexpr uint64_t modifier() const { return modifier_; } > > std::string toString() const; > + bool isRaw() const; > > static PixelFormat fromString(const std::string &name); > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp > index 314179a8..436ef5fb 100644 > --- a/src/libcamera/pixel_format.cpp > +++ b/src/libcamera/pixel_format.cpp > @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const > * \return DRM modifier > */ > > +/** > + * \brief Checks if \a this is a RAW pixel format Well, I'm not sure I want to go there, as talking about colour spaces and colour encoding for RAW formats is a bit of a minefield. Indeed we use the colour encoding of a format to identify it as RAW, so the \brief here is somewhat correct. However I would mention briefly that we check if a pixel format is RAW by checking its color encoding. /** * \brief Check if \this is RAW pixel format * * Check if a pixel format is RAW by validating that its colour * encoding is ColourEncodingRAW. * * \return True if \a this has RAW colour encoding, false otherwise */ With this, if you think it's appropriate: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > + * \return True if \a this is a RAW pixel format, false otherwise > + */ > +bool PixelFormat::isRaw() const > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(*this); > + > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > +} > + > /** > * \brief Assemble and return a string describing the pixel format > * \return A string describing the pixel format > -- > 2.45.2 >
On Tue, Oct 01, 2024 at 08:41:51PM +0200, Jacopo Mondi wrote: > Hi Umang > > On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote: > > Add a isRaw() helper to the PixelFormat class, to know whether the > > pixel format has RAW encoding. > > > > This will used by validation and configuration code paths in pipeline > > handlers, to know whether a pixel format is a raw format or not. > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > include/libcamera/pixel_format.h | 1 + > > src/libcamera/pixel_format.cpp | 11 +++++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h > > index 1b4d8c7c..aed53ea6 100644 > > --- a/include/libcamera/pixel_format.h > > +++ b/include/libcamera/pixel_format.h > > @@ -37,6 +37,7 @@ public: > > constexpr uint64_t modifier() const { return modifier_; } > > > > std::string toString() const; > > + bool isRaw() const; > > > > static PixelFormat fromString(const std::string &name); > > > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp > > index 314179a8..436ef5fb 100644 > > --- a/src/libcamera/pixel_format.cpp > > +++ b/src/libcamera/pixel_format.cpp > > @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const > > * \return DRM modifier > > */ > > > > +/** > > + * \brief Checks if \a this is a RAW pixel format > > Well, I'm not sure I want to go there, as talking about colour spaces > and colour encoding for RAW formats is a bit of a minefield. Indeed we > use the colour encoding of a format to identify it as RAW, so the > \brief here is somewhat correct. I'm also concerned, sorry :-( "RAW" is ill-defined here. It's actually not defined at all in this patch :-) The PixelFormat is a public class, so its API needs to be clearly defined, we can't just implement PixelFormat::isRaw() as "what libcamera does now internally in most place". And once you try to define the concept, you'll likely realize that it's not an easy exercize. A good example is the R8 format, which is used for 8-bit greyscale images. An image in that format can be a raw frame captured directly from the snesor, or a processed frame at the output of the ISP. > However I would mention briefly that we check if a pixel format is RAW > by checking its color encoding. > > > /** > * \brief Check if \this is RAW pixel format > * > * Check if a pixel format is RAW by validating that its colour > * encoding is ColourEncodingRAW. > * > * \return True if \a this has RAW colour encoding, false otherwise > */ > > With this, if you think it's appropriate: > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > + * \return True if \a this is a RAW pixel format, false otherwise > > + */ > > +bool PixelFormat::isRaw() const > > +{ > > + const PixelFormatInfo &info = PixelFormatInfo::info(*this); > > + > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > > +} > > + > > /** > > * \brief Assemble and return a string describing the pixel format > > * \return A string describing the pixel format
Hi Laurent, On 03/10/24 8:57 pm, Laurent Pinchart wrote: > On Tue, Oct 01, 2024 at 08:41:51PM +0200, Jacopo Mondi wrote: >> Hi Umang >> >> On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote: >>> Add a isRaw() helper to the PixelFormat class, to know whether the >>> pixel format has RAW encoding. >>> >>> This will used by validation and configuration code paths in pipeline >>> handlers, to know whether a pixel format is a raw format or not. >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> include/libcamera/pixel_format.h | 1 + >>> src/libcamera/pixel_format.cpp | 11 +++++++++++ >>> 2 files changed, 12 insertions(+) >>> >>> diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h >>> index 1b4d8c7c..aed53ea6 100644 >>> --- a/include/libcamera/pixel_format.h >>> +++ b/include/libcamera/pixel_format.h >>> @@ -37,6 +37,7 @@ public: >>> constexpr uint64_t modifier() const { return modifier_; } >>> >>> std::string toString() const; >>> + bool isRaw() const; >>> >>> static PixelFormat fromString(const std::string &name); >>> >>> diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp >>> index 314179a8..436ef5fb 100644 >>> --- a/src/libcamera/pixel_format.cpp >>> +++ b/src/libcamera/pixel_format.cpp >>> @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const >>> * \return DRM modifier >>> */ >>> >>> +/** >>> + * \brief Checks if \a this is a RAW pixel format >> Well, I'm not sure I want to go there, as talking about colour spaces >> and colour encoding for RAW formats is a bit of a minefield. Indeed we >> use the colour encoding of a format to identify it as RAW, so the >> \brief here is somewhat correct. > I'm also concerned, sorry :-( > > "RAW" is ill-defined here. It's actually not defined at all in this > patch :-) The PixelFormat is a public class, so its API needs to be > clearly defined, we can't just implement PixelFormat::isRaw() as "what > libcamera does now internally in most place". And once you try to define > the concept, you'll likely realize that it's not an easy exercize. A > good example is the R8 format, which is used for 8-bit greyscale images. > An image in that format can be a raw frame captured directly from the > snesor, or a processed frame at the output of the ISP. Thanks for the comment, it indeed is a difficult exercise to define 'raw'. I will drop the series. >> However I would mention briefly that we check if a pixel format is RAW >> by checking its color encoding. >> >> >> /** >> * \brief Check if \this is RAW pixel format >> * >> * Check if a pixel format is RAW by validating that its colour >> * encoding is ColourEncodingRAW. >> * >> * \return True if \a this has RAW colour encoding, false otherwise >> */ >> >> With this, if you think it's appropriate: >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> >>> + * \return True if \a this is a RAW pixel format, false otherwise >>> + */ >>> +bool PixelFormat::isRaw() const >>> +{ >>> + const PixelFormatInfo &info = PixelFormatInfo::info(*this); >>> + >>> + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; >>> +} >>> + >>> /** >>> * \brief Assemble and return a string describing the pixel format >>> * \return A string describing the pixel format
diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h index 1b4d8c7c..aed53ea6 100644 --- a/include/libcamera/pixel_format.h +++ b/include/libcamera/pixel_format.h @@ -37,6 +37,7 @@ public: constexpr uint64_t modifier() const { return modifier_; } std::string toString() const; + bool isRaw() const; static PixelFormat fromString(const std::string &name); diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp index 314179a8..436ef5fb 100644 --- a/src/libcamera/pixel_format.cpp +++ b/src/libcamera/pixel_format.cpp @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const * \return DRM modifier */ +/** + * \brief Checks if \a this is a RAW pixel format + * \return True if \a this is a RAW pixel format, false otherwise + */ +bool PixelFormat::isRaw() const +{ + const PixelFormatInfo &info = PixelFormatInfo::info(*this); + + return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; +} + /** * \brief Assemble and return a string describing the pixel format * \return A string describing the pixel format
Add a isRaw() helper to the PixelFormat class, to know whether the pixel format has RAW encoding. This will used by validation and configuration code paths in pipeline handlers, to know whether a pixel format is a raw format or not. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- include/libcamera/pixel_format.h | 1 + src/libcamera/pixel_format.cpp | 11 +++++++++++ 2 files changed, 12 insertions(+)