| Message ID | 20240924071341.24424-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | e85c7ddd38ce8456ab01c2a73baf9e788f6a462e |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2024-09-24 08:13:41) > The resizer limits are dynamically queried in populateFormats() and > assigned to minResolution_ and maxResolution_. Therefore, initializing > these values in the constructor is unnecessary. > > This change allows us to remove the hard-coded max/min resolution limits > from RkISP1Path, simplifying its constructor further. > This looks reasonable to me. I can see that the sizes are set correctly at PopulateFormats, and as the types are a Size, there is no possibility of use of uninitialized data, so this all looks good to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 ++++++------------- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-- > 2 files changed, 7 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index c49017d1..8015c58c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -54,11 +54,8 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { > > } /* namespace */ > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > - const Size &minResolution, const Size &maxResolution) > - : name_(name), running_(false), formats_(formats), > - minResolution_(minResolution), maxResolution_(maxResolution), > - link_(nullptr) > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) > + : name_(name), running_(false), formats_(formats), link_(nullptr) > { > } > > @@ -435,12 +432,10 @@ void RkISP1Path::stop() > } > > /* > - * \todo Remove the hardcoded resolutions and formats once all users will have > - * migrated to a recent enough kernel. > + * \todo Remove the hardcoded formats once all users will have migrated to a > + * recent enough kernel. > */ > namespace { > -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -462,8 +457,6 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::SRGGB12, > }; > > -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -477,14 +470,12 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > } /* namespace */ > > RkISP1MainPath::RkISP1MainPath() > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > - RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) > { > } > > RkISP1SelfPath::RkISP1SelfPath() > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > - RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) > { > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 08edefec..d33471e4 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -32,8 +32,7 @@ struct V4L2SubdeviceFormat; > class RkISP1Path > { > public: > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > - const Size &minResolution, const Size &maxResolution); > + RkISP1Path(const char *name, const Span<const PixelFormat> &formats); > > bool init(MediaDevice *media); > > -- > 2.45.2 >
Hi Umang On Tue, Sep 24, 2024 at 12:43:41PM GMT, Umang Jain wrote: > The resizer limits are dynamically queried in populateFormats() and Is this the resizer ? minResolution_ and maxResolution_ are populated from the list of formats from video_ and if I recall correctly RkISP1 has an explicit resizer entity, right ? > assigned to minResolution_ and maxResolution_. Therefore, initializing > these values in the constructor is unnecessary. > > This change allows us to remove the hard-coded max/min resolution limits > from RkISP1Path, simplifying its constructor further. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 ++++++------------- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-- > 2 files changed, 7 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index c49017d1..8015c58c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -54,11 +54,8 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { > > } /* namespace */ > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > - const Size &minResolution, const Size &maxResolution) > - : name_(name), running_(false), formats_(formats), > - minResolution_(minResolution), maxResolution_(maxResolution), > - link_(nullptr) > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) > + : name_(name), running_(false), formats_(formats), link_(nullptr) > { > } > > @@ -435,12 +432,10 @@ void RkISP1Path::stop() > } > > /* > - * \todo Remove the hardcoded resolutions and formats once all users will have > - * migrated to a recent enough kernel. > + * \todo Remove the hardcoded formats once all users will have migrated to a > + * recent enough kernel. > */ > namespace { > -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -462,8 +457,6 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::SRGGB12, > }; > > -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -477,14 +470,12 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > } /* namespace */ > > RkISP1MainPath::RkISP1MainPath() > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > - RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) > { > } > > RkISP1SelfPath::RkISP1SelfPath() > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > - RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) > { > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 08edefec..d33471e4 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -32,8 +32,7 @@ struct V4L2SubdeviceFormat; > class RkISP1Path > { > public: > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > - const Size &minResolution, const Size &maxResolution); > + RkISP1Path(const char *name, const Span<const PixelFormat> &formats); However I think this is safe. minResolution_ and maxResolution_ are populated at ::init() time and the class is not usable before that function has been called. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > bool init(MediaDevice *media); > > -- > 2.45.2 >
Hi Jacopo On 26/09/24 2:51 pm, Jacopo Mondi wrote: > Hi Umang > > On Tue, Sep 24, 2024 at 12:43:41PM GMT, Umang Jain wrote: >> The resizer limits are dynamically queried in populateFormats() and > Is this the resizer ? > > minResolution_ and maxResolution_ are populated from the list of > formats from video_ and if I recall correctly RkISP1 has an explicit > resizer entity, right ? Ah yes, you are correct. I will need to update the commit message. The minResolution_ and maxResolution_ are populated from (main|self)_path video nodes. I would go with: ``` libcamera: rkisp1: Eliminate hard-coded resizer limits The minResolution_ and maxResolution_ limits are dynamically queried in populateFormats() from the RkISP1Path video node. Therefore, initializing these limits with the resizer limits in the constructor is unnecessary. This change allows us to remove the hard-coded max/min resolution limits of the resizer from RkISP1Path, simplifying its constructor further. ``` > >> assigned to minResolution_ and maxResolution_. Therefore, initializing >> these values in the constructor is unnecessary. >> >> This change allows us to remove the hard-coded max/min resolution limits >> from RkISP1Path, simplifying its constructor further. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 ++++++------------- >> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-- >> 2 files changed, 7 insertions(+), 17 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> index c49017d1..8015c58c 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> @@ -54,11 +54,8 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { >> >> } /* namespace */ >> >> -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, >> - const Size &minResolution, const Size &maxResolution) >> - : name_(name), running_(false), formats_(formats), >> - minResolution_(minResolution), maxResolution_(maxResolution), >> - link_(nullptr) >> +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) >> + : name_(name), running_(false), formats_(formats), link_(nullptr) >> { >> } >> >> @@ -435,12 +432,10 @@ void RkISP1Path::stop() >> } >> >> /* >> - * \todo Remove the hardcoded resolutions and formats once all users will have >> - * migrated to a recent enough kernel. >> + * \todo Remove the hardcoded formats once all users will have migrated to a >> + * recent enough kernel. >> */ >> namespace { >> -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; >> -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; >> constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ >> formats::YUYV, >> formats::NV16, >> @@ -462,8 +457,6 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ >> formats::SRGGB12, >> }; >> >> -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; >> -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; >> constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ >> formats::YUYV, >> formats::NV16, >> @@ -477,14 +470,12 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ >> } /* namespace */ >> >> RkISP1MainPath::RkISP1MainPath() >> - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, >> - RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) >> + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) >> { >> } >> >> RkISP1SelfPath::RkISP1SelfPath() >> - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, >> - RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) >> + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) >> { >> } >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >> index 08edefec..d33471e4 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >> @@ -32,8 +32,7 @@ struct V4L2SubdeviceFormat; >> class RkISP1Path >> { >> public: >> - RkISP1Path(const char *name, const Span<const PixelFormat> &formats, >> - const Size &minResolution, const Size &maxResolution); >> + RkISP1Path(const char *name, const Span<const PixelFormat> &formats); > However I think this is safe. minResolution_ and maxResolution_ are > populated at ::init() time and the class is not usable before that > function has been called. > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> bool init(MediaDevice *media); >> >> -- >> 2.45.2 >>
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index c49017d1..8015c58c 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -54,11 +54,8 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { } /* namespace */ -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, - const Size &minResolution, const Size &maxResolution) - : name_(name), running_(false), formats_(formats), - minResolution_(minResolution), maxResolution_(maxResolution), - link_(nullptr) +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) + : name_(name), running_(false), formats_(formats), link_(nullptr) { } @@ -435,12 +432,10 @@ void RkISP1Path::stop() } /* - * \todo Remove the hardcoded resolutions and formats once all users will have - * migrated to a recent enough kernel. + * \todo Remove the hardcoded formats once all users will have migrated to a + * recent enough kernel. */ namespace { -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ formats::YUYV, formats::NV16, @@ -462,8 +457,6 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ formats::SRGGB12, }; -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ formats::YUYV, formats::NV16, @@ -477,14 +470,12 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ } /* namespace */ RkISP1MainPath::RkISP1MainPath() - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, - RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) { } RkISP1SelfPath::RkISP1SelfPath() - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, - RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) { } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 08edefec..d33471e4 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -32,8 +32,7 @@ struct V4L2SubdeviceFormat; class RkISP1Path { public: - RkISP1Path(const char *name, const Span<const PixelFormat> &formats, - const Size &minResolution, const Size &maxResolution); + RkISP1Path(const char *name, const Span<const PixelFormat> &formats); bool init(MediaDevice *media);
The resizer limits are dynamically queried in populateFormats() and assigned to minResolution_ and maxResolution_. Therefore, initializing these values in the constructor is unnecessary. This change allows us to remove the hard-coded max/min resolution limits from RkISP1Path, simplifying its constructor further. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 ++++++------------- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-- 2 files changed, 7 insertions(+), 17 deletions(-)