| Message ID | 20210803133205.6599-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote: > In CameraSensor, the mbusCodes() and sizes() accessor functions > retrieves all the supported media bus codes and the supported sizes > respectively. However, this is quite limiting since the caller > probably isn't in a position to match which range of sizes are > supported for a particular mbusCode. > > Hence, the caller is most likely interested to know about the sizes > supported for a particular media bus code. This patch transforms the > existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to > achieve that goal. > > To know all the frame sizes of the CameraSensor as required in IPU3 > case Cio2Device::sizes(), one would now require to enumerate all the > media bus codes (can be retrieved by CameraSensor::mbusCodes()) with > CameraSensor::size(mbusCode). The result can be inserted in a > std::set<> to avoid duplicates. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/internal/camera_sensor.h | 2 +- > src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------ > src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++--- > test/camera-sensor.cpp | 2 +- > 4 files changed, 34 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index db12b07e..d25a1165 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -38,7 +38,7 @@ public: > const std::string &id() const { return id_; } > const MediaEntity *entity() const { return entity_; } > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > - const std::vector<Size> &sizes() const { return sizes_; } > + const std::vector<Size> sizes(unsigned int mbusCode) const; > Size resolution() const; > const std::vector<int32_t> &testPatternModes() const > { > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index cde431cc..3c3ceff3 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -471,16 +471,6 @@ int CameraSensor::initProperties() > * \return The supported media bus codes sorted in increasing order > */ > > -/** > - * \fn CameraSensor::sizes() > - * \brief Retrieve the frame sizes supported by the camera sensor > - * > - * The reported sizes span all media bus codes supported by the camera sensor. > - * Not all sizes may be supported by all media bus codes. > - * > - * \return The supported frame sizes sorted in increasing order > - */ > - > /** > * \brief Retrieve the camera sensor resolution > * > @@ -594,6 +584,32 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu > return format; > } > > +/** > + * \brief Retrieve the supported frame sizes for a media bus code > + * \param[in] mbusCode The media bus code for which sizes are requested > + * > + * The reported sizes for a particular \a mbusCode are supported by the camera > + * sensor. I would drop this > + * > + * \return The supported frame sizes for \a mbusCode sorted in increasing order > + */ > +const std::vector<Size> CameraSensor::sizes(unsigned int mbusCode) const Why have you moved this ? Let's respect the declaration order > +{ > + std::vector<Size> sizes; > + > + const auto format = formats_.find(mbusCode); &format > + if (format == formats_.end()) > + return sizes; > + > + const std::vector<SizeRange> &ranges = format->second; > + std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes), > + [](const SizeRange &range) { return range.max; }); > + > + std::sort(sizes.begin(), sizes.end()); > + > + return sizes; > +} > + > /** > * \brief Set the sensor output format > * \param[in] format The desired sensor output format > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 1bcd580e..6f2ef321 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -69,11 +69,13 @@ std::vector<SizeRange> CIO2Device::sizes() const > if (!sensor_) > return {}; > > - std::vector<SizeRange> sizes; > - for (const Size &size : sensor_->sizes()) > - sizes.emplace_back(size, size); > + std::set<Size> allSizes; > + for (unsigned int code : sensor_->mbusCodes()) { > + for (Size sz : sensor_->sizes(code)) const Size &sz > + allSizes.insert(sz); > + } > > - return sizes; > + return std::vector<SizeRange>(allSizes.begin(), allSizes.end()); Looking at how CIO2::sizes() is used in IPU3::generateConfiguration() I wonder if we shouldn't pass the desired PixelFormat to CIO2::sizes() so that we can actually enumerate the per-format sizes for real (and avoid going through the set->vector conversion). > } > > /** > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > index a8dcad82..372ee4af 100644 > --- a/test/camera-sensor.cpp > +++ b/test/camera-sensor.cpp > @@ -76,7 +76,7 @@ protected: > return TestFail; > } > > - const std::vector<Size> &sizes = sensor_->sizes(); > + const std::vector<Size> &sizes = sensor_->sizes(*iter); This changes the semantic of the test from "let's check if a format supports 4096x2160" to "let's check if ARGB8888_1X32 supports 4096x2160". I think it's better now, and as long as the test passes, since it uses VIMC, we should be good! Thanks j > auto iter2 = std::find(sizes.begin(), sizes.end(), > Size(4096, 2160)); > if (iter2 == sizes.end()) { > -- > 2.31.0 >
Hi Jacopo On 8/9/21 9:19 PM, Jacopo Mondi wrote: > Hi Umang, > > On Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote: >> In CameraSensor, the mbusCodes() and sizes() accessor functions >> retrieves all the supported media bus codes and the supported sizes >> respectively. However, this is quite limiting since the caller >> probably isn't in a position to match which range of sizes are >> supported for a particular mbusCode. >> >> Hence, the caller is most likely interested to know about the sizes >> supported for a particular media bus code. This patch transforms the >> existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to >> achieve that goal. >> >> To know all the frame sizes of the CameraSensor as required in IPU3 >> case Cio2Device::sizes(), one would now require to enumerate all the >> media bus codes (can be retrieved by CameraSensor::mbusCodes()) with >> CameraSensor::size(mbusCode). The result can be inserted in a >> std::set<> to avoid duplicates. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> include/libcamera/internal/camera_sensor.h | 2 +- >> src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------ >> src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++--- >> test/camera-sensor.cpp | 2 +- >> 4 files changed, 34 insertions(+), 16 deletions(-) >> <snip> >> + allSizes.insert(sz); >> + } >> >> - return sizes; >> + return std::vector<SizeRange>(allSizes.begin(), allSizes.end()); > Looking at how CIO2::sizes() is used in IPU3::generateConfiguration() > I wonder if we shouldn't pass the desired PixelFormat to I think you meant s/shouldn't/should here? So essentially do you want std::vector<SizeRange> CIO2Device::sizes(const PixelFormat &format) const ? (which looks okay to me) > CIO2::sizes() so that we can actually enumerate the per-format sizes > for real (and avoid going through the set->vector conversion). > >> } >> >> /** >> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp >> index a8dcad82..372ee4af 100644 >> --- a/test/camera-sensor.cpp >> +++ b/test/camera-sensor.cpp >> @@ -76,7 +76,7 @@ protected: >> return TestFail; >> } >> >> - const std::vector<Size> &sizes = sensor_->sizes(); >> + const std::vector<Size> &sizes = sensor_->sizes(*iter); > This changes the semantic of the test from "let's check if a format > supports 4096x2160" to "let's check if ARGB8888_1X32 supports > 4096x2160". I think it's better now, and as long as the test passes, > since it uses VIMC, we should be good! I'll check! > > Thanks > j > >> auto iter2 = std::find(sizes.begin(), sizes.end(), >> Size(4096, 2160)); >> if (iter2 == sizes.end()) { >> -- >> 2.31.0 >>
Hi Umang, On Tue, Aug 10, 2021 at 11:13:35AM +0530, Umang Jain wrote: > Hi Jacopo > > On 8/9/21 9:19 PM, Jacopo Mondi wrote: > > Hi Umang, > > > > On Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote: > > > In CameraSensor, the mbusCodes() and sizes() accessor functions > > > retrieves all the supported media bus codes and the supported sizes > > > respectively. However, this is quite limiting since the caller > > > probably isn't in a position to match which range of sizes are > > > supported for a particular mbusCode. > > > > > > Hence, the caller is most likely interested to know about the sizes > > > supported for a particular media bus code. This patch transforms the > > > existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to > > > achieve that goal. > > > > > > To know all the frame sizes of the CameraSensor as required in IPU3 > > > case Cio2Device::sizes(), one would now require to enumerate all the > > > media bus codes (can be retrieved by CameraSensor::mbusCodes()) with > > > CameraSensor::size(mbusCode). The result can be inserted in a > > > std::set<> to avoid duplicates. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > include/libcamera/internal/camera_sensor.h | 2 +- > > > src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------ > > > src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++--- > > > test/camera-sensor.cpp | 2 +- > > > 4 files changed, 34 insertions(+), 16 deletions(-) > > > > <snip> > > > > + allSizes.insert(sz); > > > + } > > > > > > - return sizes; > > > + return std::vector<SizeRange>(allSizes.begin(), allSizes.end()); > > Looking at how CIO2::sizes() is used in IPU3::generateConfiguration() > > I wonder if we shouldn't pass the desired PixelFormat to > > I think you meant s/shouldn't/should here? So essentially do you want > > std::vector<SizeRange> CIO2Device::sizes(const PixelFormat &format) > const > > ? > > (which looks okay to me) Looking at how it is used to generate the raw stream sizes I think it would be better > > > CIO2::sizes() so that we can actually enumerate the per-format sizes > > for real (and avoid going through the set->vector conversion). > > > > > } > > > > > > /** > > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > > > index a8dcad82..372ee4af 100644 > > > --- a/test/camera-sensor.cpp > > > +++ b/test/camera-sensor.cpp > > > @@ -76,7 +76,7 @@ protected: > > > return TestFail; > > > } > > > > > > - const std::vector<Size> &sizes = sensor_->sizes(); > > > + const std::vector<Size> &sizes = sensor_->sizes(*iter); > > This changes the semantic of the test from "let's check if a format > > supports 4096x2160" to "let's check if ARGB8888_1X32 supports > > 4096x2160". I think it's better now, and as long as the test passes, > > since it uses VIMC, we should be good! > I'll check! Yeah please make sure tests still pass in full. This one shouldn't be a problem... Thanks j > > > > Thanks > > j > > > > > auto iter2 = std::find(sizes.begin(), sizes.end(), > > > Size(4096, 2160)); > > > if (iter2 == sizes.end()) { > > > -- > > > 2.31.0 > > >
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index db12b07e..d25a1165 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -38,7 +38,7 @@ public: const std::string &id() const { return id_; } const MediaEntity *entity() const { return entity_; } const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } - const std::vector<Size> &sizes() const { return sizes_; } + const std::vector<Size> sizes(unsigned int mbusCode) const; Size resolution() const; const std::vector<int32_t> &testPatternModes() const { diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index cde431cc..3c3ceff3 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -471,16 +471,6 @@ int CameraSensor::initProperties() * \return The supported media bus codes sorted in increasing order */ -/** - * \fn CameraSensor::sizes() - * \brief Retrieve the frame sizes supported by the camera sensor - * - * The reported sizes span all media bus codes supported by the camera sensor. - * Not all sizes may be supported by all media bus codes. - * - * \return The supported frame sizes sorted in increasing order - */ - /** * \brief Retrieve the camera sensor resolution * @@ -594,6 +584,32 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu return format; } +/** + * \brief Retrieve the supported frame sizes for a media bus code + * \param[in] mbusCode The media bus code for which sizes are requested + * + * The reported sizes for a particular \a mbusCode are supported by the camera + * sensor. + * + * \return The supported frame sizes for \a mbusCode sorted in increasing order + */ +const std::vector<Size> CameraSensor::sizes(unsigned int mbusCode) const +{ + std::vector<Size> sizes; + + const auto format = formats_.find(mbusCode); + if (format == formats_.end()) + return sizes; + + const std::vector<SizeRange> &ranges = format->second; + std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes), + [](const SizeRange &range) { return range.max; }); + + std::sort(sizes.begin(), sizes.end()); + + return sizes; +} + /** * \brief Set the sensor output format * \param[in] format The desired sensor output format diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 1bcd580e..6f2ef321 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -69,11 +69,13 @@ std::vector<SizeRange> CIO2Device::sizes() const if (!sensor_) return {}; - std::vector<SizeRange> sizes; - for (const Size &size : sensor_->sizes()) - sizes.emplace_back(size, size); + std::set<Size> allSizes; + for (unsigned int code : sensor_->mbusCodes()) { + for (Size sz : sensor_->sizes(code)) + allSizes.insert(sz); + } - return sizes; + return std::vector<SizeRange>(allSizes.begin(), allSizes.end()); } /** diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp index a8dcad82..372ee4af 100644 --- a/test/camera-sensor.cpp +++ b/test/camera-sensor.cpp @@ -76,7 +76,7 @@ protected: return TestFail; } - const std::vector<Size> &sizes = sensor_->sizes(); + const std::vector<Size> &sizes = sensor_->sizes(*iter); auto iter2 = std::find(sizes.begin(), sizes.end(), Size(4096, 2160)); if (iter2 == sizes.end()) {
In CameraSensor, the mbusCodes() and sizes() accessor functions retrieves all the supported media bus codes and the supported sizes respectively. However, this is quite limiting since the caller probably isn't in a position to match which range of sizes are supported for a particular mbusCode. Hence, the caller is most likely interested to know about the sizes supported for a particular media bus code. This patch transforms the existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to achieve that goal. To know all the frame sizes of the CameraSensor as required in IPU3 case Cio2Device::sizes(), one would now require to enumerate all the media bus codes (can be retrieved by CameraSensor::mbusCodes()) with CameraSensor::size(mbusCode). The result can be inserted in a std::set<> to avoid duplicates. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- include/libcamera/internal/camera_sensor.h | 2 +- src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------ src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++--- test/camera-sensor.cpp | 2 +- 4 files changed, 34 insertions(+), 16 deletions(-)