| Message ID | 20200320215020.GA26347@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote: > Use of default constructor StreamConfiguration() in deprecated, hence > make it private. Make Stream class a friend of StreamConfiguration as it > uses the default constructor. > > Replace default constructor StreamConfiguration() by it's parametered s/parametered/parametrized ? > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > WIP: > - It fails to build as other pipelines still use default > constructor. > - Even rkisp1 fails as it uses default constructor in its > validate() function. The compiler points to config_.resize(1); ^ ../include/libcamera/stream.h:54:2: note: declared private here StreamConfiguration(); The std::vector::resize() documentation says If the current size is greater than count, the container is reduced to its first count elements. So I guess the compiler is just worried about the possibility that the current size is smaller than the resize(n) argument and a new instance should be created, so it would need to access the default constructor. The most trivial option here is to manually delete the exceeding instances if vector.size() > 1 and create a new one using the parametrized constructor. > - I have taken care of generateConfiguration() only. > > include/libcamera/stream.h | 4 ++- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- > src/libcamera/stream.cpp | 9 ------ > 3 files changed, 28 insertions(+), 22 deletions(-) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index b1441f8..c19aed6 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -37,7 +37,6 @@ private: > }; > > struct StreamConfiguration { > - StreamConfiguration(); > StreamConfiguration(const StreamFormats &formats); > > PixelFormat pixelFormat; > @@ -52,8 +51,11 @@ struct StreamConfiguration { > std::string toString() const; > > private: > + StreamConfiguration(); > Stream *stream_; > StreamFormats formats_; > + > + friend class Stream; > }; > > enum StreamRole { > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 2f909ce..bf97b53 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -218,6 +218,21 @@ private: > Camera *activeCamera_; > }; > > +namespace { > + > +static const std::array<PixelFormat, 8> formats{ > + PixelFormat(DRM_FORMAT_YUYV), > + PixelFormat(DRM_FORMAT_YVYU), > + PixelFormat(DRM_FORMAT_VYUY), > + PixelFormat(DRM_FORMAT_NV16), > + PixelFormat(DRM_FORMAT_NV61), > + PixelFormat(DRM_FORMAT_NV21), > + PixelFormat(DRM_FORMAT_NV12), > + /* \todo Add support for 8-bit greyscale to DRM formats */ > +}; > + > +} /* namespace */ > + > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) > { > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > { > - static const std::array<PixelFormat, 8> formats{ > - PixelFormat(DRM_FORMAT_YUYV), > - PixelFormat(DRM_FORMAT_YVYU), > - PixelFormat(DRM_FORMAT_VYUY), > - PixelFormat(DRM_FORMAT_NV16), > - PixelFormat(DRM_FORMAT_NV61), > - PixelFormat(DRM_FORMAT_NV21), > - PixelFormat(DRM_FORMAT_NV12), > - /* \todo Add support for 8-bit greyscale to DRM formats */ > - }; > - > const CameraSensor *sensor = data_->sensor_; > Status status = Valid; > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > if (roles.empty()) > return config; > > - StreamConfiguration cfg{}; > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > + > + for (PixelFormat pixelformat : formats) { > + std::vector<SizeRange> sizes{ > + SizeRange{ { 32, 16 }, { 4416, 3312 } } I might have missed where these sizes come from. > + }; > + pixelformats[pixelformat] = sizes; So you have an hardcoded list of PixelFormats in the pipeline handler. And you got a map of V4L2PixelFormats from the video device. What I would do is going through all the PixelFormats, get their V4L2PixelFormat counterpart, access the map using that as key to retrive the size vector, and associated them with the PixelFormat you started with. I don't think it is necessary to make sure all the V4L2PixelFormat obtained from the PixelFormat array are valid, as the pipeline handler should know that. Thanks j > + } > + StreamFormats format(pixelformats); > + StreamConfiguration cfg(format); > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > cfg.size = data->sensor_->resolution(); > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index ea3d214..96b3dc5 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > * configured for a single video stream. > */ > > -/** > - * \todo This method is deprecated and should be removed once all pipeline > - * handlers provied StreamFormats. > - */ > -StreamConfiguration::StreamConfiguration() > - : pixelFormat(0), stream_(nullptr) > -{ > -} > - > /** > * \brief Construct a configuration with stream formats > */ > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi again, On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > Hi Kaaira, > > On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote: > > Use of default constructor StreamConfiguration() in deprecated, hence > > make it private. Make Stream class a friend of StreamConfiguration as it > > uses the default constructor. > > > > Replace default constructor StreamConfiguration() by it's parametered > > s/parametered/parametrized ? > > > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > WIP: > > - It fails to build as other pipelines still use default > > constructor. > > - Even rkisp1 fails as it uses default constructor in its > > validate() function. > > The compiler points to > > config_.resize(1); > ^ > ../include/libcamera/stream.h:54:2: note: declared private here > StreamConfiguration(); > > The std::vector::resize() documentation says > If the current size is greater than count, the container is reduced to its first count elements. > > So I guess the compiler is just worried about the possibility that the > current size is smaller than the resize(n) argument and a new instance > should be created, so it would need to access the default constructor. > > The most trivial option here is to manually delete the exceeding > instances if vector.size() > 1 and create a new one using the > parametrized constructor. Sorry this is confused. Delete the exceeding entries if the size of the vector is larger and create a new instance manually if the vector is empty (which can't happen as it is caught above :). > > > - I have taken care of generateConfiguration() only. > > > > include/libcamera/stream.h | 4 ++- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- > > src/libcamera/stream.cpp | 9 ------ > > 3 files changed, 28 insertions(+), 22 deletions(-) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index b1441f8..c19aed6 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -37,7 +37,6 @@ private: > > }; > > > > struct StreamConfiguration { > > - StreamConfiguration(); > > StreamConfiguration(const StreamFormats &formats); > > > > PixelFormat pixelFormat; > > @@ -52,8 +51,11 @@ struct StreamConfiguration { > > std::string toString() const; > > > > private: > > + StreamConfiguration(); > > Stream *stream_; > > StreamFormats formats_; > > + > > + friend class Stream; > > }; > > > > enum StreamRole { > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 2f909ce..bf97b53 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -218,6 +218,21 @@ private: > > Camera *activeCamera_; > > }; > > > > +namespace { > > + > > +static const std::array<PixelFormat, 8> formats{ > > + PixelFormat(DRM_FORMAT_YUYV), > > + PixelFormat(DRM_FORMAT_YVYU), > > + PixelFormat(DRM_FORMAT_VYUY), > > + PixelFormat(DRM_FORMAT_NV16), > > + PixelFormat(DRM_FORMAT_NV61), > > + PixelFormat(DRM_FORMAT_NV21), > > + PixelFormat(DRM_FORMAT_NV12), > > + /* \todo Add support for 8-bit greyscale to DRM formats */ > > +}; > > + > > +} /* namespace */ > > + > > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > > : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) > > { > > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > { > > - static const std::array<PixelFormat, 8> formats{ > > - PixelFormat(DRM_FORMAT_YUYV), > > - PixelFormat(DRM_FORMAT_YVYU), > > - PixelFormat(DRM_FORMAT_VYUY), > > - PixelFormat(DRM_FORMAT_NV16), > > - PixelFormat(DRM_FORMAT_NV61), > > - PixelFormat(DRM_FORMAT_NV21), > > - PixelFormat(DRM_FORMAT_NV12), > > - /* \todo Add support for 8-bit greyscale to DRM formats */ > > - }; > > - > > const CameraSensor *sensor = data_->sensor_; > > Status status = Valid; > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > if (roles.empty()) > > return config; > > > > - StreamConfiguration cfg{}; > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > + > > + for (PixelFormat pixelformat : formats) { > > + std::vector<SizeRange> sizes{ > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > I might have missed where these sizes come from. > > > + }; > > + pixelformats[pixelformat] = sizes; > > So you have an hardcoded list of PixelFormats in the pipeline handler. > And you got a map of V4L2PixelFormats from the video device. > What I would do is going through all the PixelFormats, get their > V4L2PixelFormat counterpart, access the map using that as key to > retrive the size vector, and associated them with the PixelFormat you > started with. > > I don't think it is necessary to make sure all the V4L2PixelFormat > obtained from the PixelFormat array are valid, as the pipeline handler > should know that. > > Thanks > j > > > > + } > > + StreamFormats format(pixelformats); > > + StreamConfiguration cfg(format); > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > cfg.size = data->sensor_->resolution(); > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > index ea3d214..96b3dc5 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > * configured for a single video stream. > > */ > > > > -/** > > - * \todo This method is deprecated and should be removed once all pipeline > > - * handlers provied StreamFormats. > > - */ > > -StreamConfiguration::StreamConfiguration() > > - : pixelFormat(0), stream_(nullptr) > > -{ > > -} > > - > > /** > > * \brief Construct a configuration with stream formats > > */ > > -- > > 2.17.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Sat, Mar 21, 2020 at 01:02:46PM +0100, Jacopo Mondi wrote: > Hi again, > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > Hi Kaaira, > > > > On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote: > > > Use of default constructor StreamConfiguration() in deprecated, hence > > > make it private. Make Stream class a friend of StreamConfiguration as it > > > uses the default constructor. > > > > > > Replace default constructor StreamConfiguration() by it's parametered > > > > s/parametered/parametrized ? > > > > > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > --- > > > WIP: > > > - It fails to build as other pipelines still use default > > > constructor. > > > - Even rkisp1 fails as it uses default constructor in its > > > validate() function. > > > > The compiler points to > > > > config_.resize(1); > > ^ > > ../include/libcamera/stream.h:54:2: note: declared private here > > StreamConfiguration(); > > > > The std::vector::resize() documentation says > > If the current size is greater than count, the container is reduced to its first count elements. > > > > So I guess the compiler is just worried about the possibility that the > > current size is smaller than the resize(n) argument and a new instance > > should be created, so it would need to access the default constructor. > > > > The most trivial option here is to manually delete the exceeding > > instances if vector.size() > 1 and create a new one using the > > parametrized constructor. > > Sorry this is confused. > > Delete the exceeding entries if the size of the vector is larger and > create a new instance manually if the vector is empty (which can't > happen as it is caught above :). Can you please elaborate it a bit more? I don't catch it when you say that "which can't happen as it is caught above", what exactly are you talking about? Sorry if this seems trivial but I am confused of what you are trying to get me to do here. thanks! > > > > > > - I have taken care of generateConfiguration() only. > > > > > > include/libcamera/stream.h | 4 ++- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- > > > src/libcamera/stream.cpp | 9 ------ > > > 3 files changed, 28 insertions(+), 22 deletions(-) > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > index b1441f8..c19aed6 100644 > > > --- a/include/libcamera/stream.h > > > +++ b/include/libcamera/stream.h > > > @@ -37,7 +37,6 @@ private: > > > }; > > > > > > struct StreamConfiguration { > > > - StreamConfiguration(); > > > StreamConfiguration(const StreamFormats &formats); > > > > > > PixelFormat pixelFormat; > > > @@ -52,8 +51,11 @@ struct StreamConfiguration { > > > std::string toString() const; > > > > > > private: > > > + StreamConfiguration(); > > > Stream *stream_; > > > StreamFormats formats_; > > > + > > > + friend class Stream; > > > }; > > > > > > enum StreamRole { > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 2f909ce..bf97b53 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -218,6 +218,21 @@ private: > > > Camera *activeCamera_; > > > }; > > > > > > +namespace { > > > + > > > +static const std::array<PixelFormat, 8> formats{ > > > + PixelFormat(DRM_FORMAT_YUYV), > > > + PixelFormat(DRM_FORMAT_YVYU), > > > + PixelFormat(DRM_FORMAT_VYUY), > > > + PixelFormat(DRM_FORMAT_NV16), > > > + PixelFormat(DRM_FORMAT_NV61), > > > + PixelFormat(DRM_FORMAT_NV21), > > > + PixelFormat(DRM_FORMAT_NV12), > > > + /* \todo Add support for 8-bit greyscale to DRM formats */ > > > +}; > > > + > > > +} /* namespace */ > > > + > > > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > > > : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) > > > { > > > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > { > > > - static const std::array<PixelFormat, 8> formats{ > > > - PixelFormat(DRM_FORMAT_YUYV), > > > - PixelFormat(DRM_FORMAT_YVYU), > > > - PixelFormat(DRM_FORMAT_VYUY), > > > - PixelFormat(DRM_FORMAT_NV16), > > > - PixelFormat(DRM_FORMAT_NV61), > > > - PixelFormat(DRM_FORMAT_NV21), > > > - PixelFormat(DRM_FORMAT_NV12), > > > - /* \todo Add support for 8-bit greyscale to DRM formats */ > > > - }; > > > - > > > const CameraSensor *sensor = data_->sensor_; > > > Status status = Valid; > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > if (roles.empty()) > > > return config; > > > > > > - StreamConfiguration cfg{}; > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > + > > > + for (PixelFormat pixelformat : formats) { > > > + std::vector<SizeRange> sizes{ > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > I might have missed where these sizes come from. > > > > > + }; > > > + pixelformats[pixelformat] = sizes; > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > And you got a map of V4L2PixelFormats from the video device. > > What I would do is going through all the PixelFormats, get their > > V4L2PixelFormat counterpart, access the map using that as key to > > retrive the size vector, and associated them with the PixelFormat you > > started with. > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > obtained from the PixelFormat array are valid, as the pipeline handler > > should know that. > > > > Thanks > > j > > > > > > > + } > > > + StreamFormats format(pixelformats); > > > + StreamConfiguration cfg(format); > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > cfg.size = data->sensor_->resolution(); > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > index ea3d214..96b3dc5 100644 > > > --- a/src/libcamera/stream.cpp > > > +++ b/src/libcamera/stream.cpp > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > * configured for a single video stream. > > > */ > > > > > > -/** > > > - * \todo This method is deprecated and should be removed once all pipeline > > > - * handlers provied StreamFormats. > > > - */ > > > -StreamConfiguration::StreamConfiguration() > > > - : pixelFormat(0), stream_(nullptr) > > > -{ > > > -} > > > - > > > /** > > > * \brief Construct a configuration with stream formats > > > */ > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > Hi Kaaira, > > On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote: > > Use of default constructor StreamConfiguration() in deprecated, hence > > make it private. Make Stream class a friend of StreamConfiguration as it > > uses the default constructor. > > > > Replace default constructor StreamConfiguration() by it's parametered > > s/parametered/parametrized ? > > > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > WIP: > > - It fails to build as other pipelines still use default > > constructor. > > - Even rkisp1 fails as it uses default constructor in its > > validate() function. > > The compiler points to > > config_.resize(1); > ^ > ../include/libcamera/stream.h:54:2: note: declared private here > StreamConfiguration(); > > The std::vector::resize() documentation says > If the current size is greater than count, the container is reduced to its first count elements. > > So I guess the compiler is just worried about the possibility that the > current size is smaller than the resize(n) argument and a new instance > should be created, so it would need to access the default constructor. > > The most trivial option here is to manually delete the exceeding > instances if vector.size() > 1 and create a new one using the > parametrized constructor. > > > - I have taken care of generateConfiguration() only. > > > > include/libcamera/stream.h | 4 ++- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- > > src/libcamera/stream.cpp | 9 ------ > > 3 files changed, 28 insertions(+), 22 deletions(-) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index b1441f8..c19aed6 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -37,7 +37,6 @@ private: > > }; > > > > struct StreamConfiguration { > > - StreamConfiguration(); > > StreamConfiguration(const StreamFormats &formats); > > > > PixelFormat pixelFormat; > > @@ -52,8 +51,11 @@ struct StreamConfiguration { > > std::string toString() const; > > > > private: > > + StreamConfiguration(); > > Stream *stream_; > > StreamFormats formats_; > > + > > + friend class Stream; > > }; > > > > enum StreamRole { > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 2f909ce..bf97b53 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -218,6 +218,21 @@ private: > > Camera *activeCamera_; > > }; > > > > +namespace { > > + > > +static const std::array<PixelFormat, 8> formats{ > > + PixelFormat(DRM_FORMAT_YUYV), > > + PixelFormat(DRM_FORMAT_YVYU), > > + PixelFormat(DRM_FORMAT_VYUY), > > + PixelFormat(DRM_FORMAT_NV16), > > + PixelFormat(DRM_FORMAT_NV61), > > + PixelFormat(DRM_FORMAT_NV21), > > + PixelFormat(DRM_FORMAT_NV12), > > + /* \todo Add support for 8-bit greyscale to DRM formats */ > > +}; > > + > > +} /* namespace */ > > + > > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > > : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) > > { > > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > { > > - static const std::array<PixelFormat, 8> formats{ > > - PixelFormat(DRM_FORMAT_YUYV), > > - PixelFormat(DRM_FORMAT_YVYU), > > - PixelFormat(DRM_FORMAT_VYUY), > > - PixelFormat(DRM_FORMAT_NV16), > > - PixelFormat(DRM_FORMAT_NV61), > > - PixelFormat(DRM_FORMAT_NV21), > > - PixelFormat(DRM_FORMAT_NV12), > > - /* \todo Add support for 8-bit greyscale to DRM formats */ > > - }; > > - > > const CameraSensor *sensor = data_->sensor_; > > Status status = Valid; > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > if (roles.empty()) > > return config; > > > > - StreamConfiguration cfg{}; > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > + > > + for (PixelFormat pixelformat : formats) { > > + std::vector<SizeRange> sizes{ > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > I might have missed where these sizes come from. I got these values from here, in the handler() cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width)); cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height)); That is how I defined them. > > > + }; > > + pixelformats[pixelformat] = sizes; > > So you have an hardcoded list of PixelFormats in the pipeline handler. > And you got a map of V4L2PixelFormats from the video device. > What I would do is going through all the PixelFormats, get their > V4L2PixelFormat counterpart, access the map using that as key to > retrive the size vector, and associated them with the PixelFormat you > started with. Please read the above logic behind the hardcoded values once and let me know if I still have to map the sizes this way? Thanks! > > I don't think it is necessary to make sure all the V4L2PixelFormat > obtained from the PixelFormat array are valid, as the pipeline handler > should know that. > > Thanks > j > > > > + } > > + StreamFormats format(pixelformats); > > + StreamConfiguration cfg(format); > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > cfg.size = data->sensor_->resolution(); > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > index ea3d214..96b3dc5 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > * configured for a single video stream. > > */ > > > > -/** > > - * \todo This method is deprecated and should be removed once all pipeline > > - * handlers provied StreamFormats. > > - */ > > -StreamConfiguration::StreamConfiguration() > > - : pixelFormat(0), stream_(nullptr) > > -{ > > -} > > - > > /** > > * \brief Construct a configuration with stream formats > > */ > > -- > > 2.17.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Sat, Mar 21, 2020 at 01:02:46PM +0100, Jacopo Mondi wrote: > Hi again, > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > Hi Kaaira, > > > > On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote: > > > Use of default constructor StreamConfiguration() in deprecated, hence > > > make it private. Make Stream class a friend of StreamConfiguration as it > > > uses the default constructor. > > > > > > Replace default constructor StreamConfiguration() by it's parametered > > > > s/parametered/parametrized ? > > > > > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > --- > > > WIP: > > > - It fails to build as other pipelines still use default > > > constructor. > > > - Even rkisp1 fails as it uses default constructor in its > > > validate() function. > > > > The compiler points to > > > > config_.resize(1); > > ^ > > ../include/libcamera/stream.h:54:2: note: declared private here > > StreamConfiguration(); > > > > The std::vector::resize() documentation says > > If the current size is greater than count, the container is reduced to its first count elements. > > > > So I guess the compiler is just worried about the possibility that the > > current size is smaller than the resize(n) argument and a new instance > > should be created, so it would need to access the default constructor. I don't think this is the case. We are already checking if the size>1, and only if it is, we are resizing it. I think the problem is with ```StreamConfiguration &cfg = config_[0];``` where we are calling the default constructor. &Format must be passed here. > > > > The most trivial option here is to manually delete the exceeding > > instances if vector.size() > 1 and create a new one using the > > parametrized constructor. > > Sorry this is confused. > > Delete the exceeding entries if the size of the vector is larger and > create a new instance manually if the vector is empty (which can't > happen as it is caught above :). > > > > > > - I have taken care of generateConfiguration() only. > > > > > > include/libcamera/stream.h | 4 ++- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- > > > src/libcamera/stream.cpp | 9 ------ > > > 3 files changed, 28 insertions(+), 22 deletions(-) > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > index b1441f8..c19aed6 100644 > > > --- a/include/libcamera/stream.h > > > +++ b/include/libcamera/stream.h > > > @@ -37,7 +37,6 @@ private: > > > }; > > > > > > struct StreamConfiguration { > > > - StreamConfiguration(); > > > StreamConfiguration(const StreamFormats &formats); > > > > > > PixelFormat pixelFormat; > > > @@ -52,8 +51,11 @@ struct StreamConfiguration { > > > std::string toString() const; > > > > > > private: > > > + StreamConfiguration(); > > > Stream *stream_; > > > StreamFormats formats_; > > > + > > > + friend class Stream; > > > }; > > > > > > enum StreamRole { > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 2f909ce..bf97b53 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -218,6 +218,21 @@ private: > > > Camera *activeCamera_; > > > }; > > > > > > +namespace { > > > + > > > +static const std::array<PixelFormat, 8> formats{ > > > + PixelFormat(DRM_FORMAT_YUYV), > > > + PixelFormat(DRM_FORMAT_YVYU), > > > + PixelFormat(DRM_FORMAT_VYUY), > > > + PixelFormat(DRM_FORMAT_NV16), > > > + PixelFormat(DRM_FORMAT_NV61), > > > + PixelFormat(DRM_FORMAT_NV21), > > > + PixelFormat(DRM_FORMAT_NV12), > > > + /* \todo Add support for 8-bit greyscale to DRM formats */ > > > +}; > > > + > > > +} /* namespace */ > > > + > > > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > > > : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) > > > { > > > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > { > > > - static const std::array<PixelFormat, 8> formats{ > > > - PixelFormat(DRM_FORMAT_YUYV), > > > - PixelFormat(DRM_FORMAT_YVYU), > > > - PixelFormat(DRM_FORMAT_VYUY), > > > - PixelFormat(DRM_FORMAT_NV16), > > > - PixelFormat(DRM_FORMAT_NV61), > > > - PixelFormat(DRM_FORMAT_NV21), > > > - PixelFormat(DRM_FORMAT_NV12), > > > - /* \todo Add support for 8-bit greyscale to DRM formats */ > > > - }; > > > - > > > const CameraSensor *sensor = data_->sensor_; > > > Status status = Valid; > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > if (roles.empty()) > > > return config; > > > > > > - StreamConfiguration cfg{}; > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > + > > > + for (PixelFormat pixelformat : formats) { > > > + std::vector<SizeRange> sizes{ > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > I might have missed where these sizes come from. > > > > > + }; > > > + pixelformats[pixelformat] = sizes; > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > And you got a map of V4L2PixelFormats from the video device. > > What I would do is going through all the PixelFormats, get their > > V4L2PixelFormat counterpart, access the map using that as key to > > retrive the size vector, and associated them with the PixelFormat you > > started with. > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > obtained from the PixelFormat array are valid, as the pipeline handler > > should know that. > > > > Thanks > > j > > > > > > > + } > > > + StreamFormats format(pixelformats); > > > + StreamConfiguration cfg(format); > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > cfg.size = data->sensor_->resolution(); > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > index ea3d214..96b3dc5 100644 > > > --- a/src/libcamera/stream.cpp > > > +++ b/src/libcamera/stream.cpp > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > * configured for a single video stream. > > > */ > > > > > > -/** > > > - * \todo This method is deprecated and should be removed once all pipeline > > > - * handlers provied StreamFormats. > > > - */ > > > -StreamConfiguration::StreamConfiguration() > > > - : pixelFormat(0), stream_(nullptr) > > > -{ > > > -} > > > - > > > /** > > > * \brief Construct a configuration with stream formats > > > */ > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kaaira, On Sat, Mar 21, 2020 at 07:39:23PM +0530, Kaaira Gupta wrote: > On Sat, Mar 21, 2020 at 01:02:46PM +0100, Jacopo Mondi wrote: > > Hi again, > > > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > > Hi Kaaira, > > > > > > On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote: > > > > Use of default constructor StreamConfiguration() in deprecated, hence > > > > make it private. Make Stream class a friend of StreamConfiguration as it > > > > uses the default constructor. > > > > > > > > Replace default constructor StreamConfiguration() by it's parametered > > > > > > s/parametered/parametrized ? > > > > > > > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > > --- > > > > WIP: > > > > - It fails to build as other pipelines still use default > > > > constructor. > > > > - Even rkisp1 fails as it uses default constructor in its > > > > validate() function. > > > > > > The compiler points to > > > > > > config_.resize(1); > > > ^ > > > ../include/libcamera/stream.h:54:2: note: declared private here > > > StreamConfiguration(); > > > > > > The std::vector::resize() documentation says > > > If the current size is greater than count, the container is reduced to its first count elements. > > > > > > So I guess the compiler is just worried about the possibility that the > > > current size is smaller than the resize(n) argument and a new instance > > > should be created, so it would need to access the default constructor. > > > > > > The most trivial option here is to manually delete the exceeding > > > instances if vector.size() > 1 and create a new one using the > > > parametrized constructor. > > > > Sorry this is confused. > > > > Delete the exceeding entries if the size of the vector is larger and > > create a new instance manually if the vector is empty (which can't > > happen as it is caught above :). > > Can you please elaborate it a bit more? I don't catch it when you say > that "which can't happen as it is caught above", what exactly are you > talking about? > Sorry if this seems trivial but I am confused of what you are trying to > get me to do here. No worries, I have indeed confused you. My point was that the compiler points to std::vector::resize() and, even if it can't happen that in this code path that a new element has to be created, I assume the resize implementation calls the StreamConfiguration default constructor in case the vector has to be enlarged, hence the error. That call to default constructor can't happen here, as we check for the vector not to be empty just before calling resize, and as we resize to 1, the only option left to enter the here below second if branch is that (config_.size() > 1) returns true. if (config_.empty()) return Invalid; /* Cap the number of entries to the available streams. */ if (config_.size() > 1) { config_.resize(1); status = Adjusted; } As long as this pipeline handler only support a single stream and its valide() implementation only accept a single StreamConfiguration, I would just erase all the entries beyond the first one to shrink the vector size to 1 and release the memory containing the exceeding entries. To sum it up I would got for if (config_.empty()) return Invalid; /* Cap the number of entries to the available streams. */ if (config_.size() > 1) { config_.erase(config_.begin() + 1, config_.end()); status = Adjusted; } That should make sure there are no calls to the class default constructor in any code path. Also, please note you removed the default constructor implementation in stream.cpp, and you will likely get a linking error. You rightfully changed the method visibility to private, but its implementation has to be defined somewhere. Hope it clears it up Thanks j > > thanks! > > > > > > > > > > - I have taken care of generateConfiguration() only. > > > > > > > > include/libcamera/stream.h | 4 ++- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- > > > > src/libcamera/stream.cpp | 9 ------ > > > > 3 files changed, 28 insertions(+), 22 deletions(-) > > > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > > index b1441f8..c19aed6 100644 > > > > --- a/include/libcamera/stream.h > > > > +++ b/include/libcamera/stream.h > > > > @@ -37,7 +37,6 @@ private: > > > > }; > > > > > > > > struct StreamConfiguration { > > > > - StreamConfiguration(); > > > > StreamConfiguration(const StreamFormats &formats); > > > > > > > > PixelFormat pixelFormat; > > > > @@ -52,8 +51,11 @@ struct StreamConfiguration { > > > > std::string toString() const; > > > > > > > > private: > > > > + StreamConfiguration(); > > > > Stream *stream_; > > > > StreamFormats formats_; > > > > + > > > > + friend class Stream; > > > > }; > > > > > > > > enum StreamRole { > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index 2f909ce..bf97b53 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -218,6 +218,21 @@ private: > > > > Camera *activeCamera_; > > > > }; > > > > > > > > +namespace { > > > > + > > > > +static const std::array<PixelFormat, 8> formats{ > > > > + PixelFormat(DRM_FORMAT_YUYV), > > > > + PixelFormat(DRM_FORMAT_YVYU), > > > > + PixelFormat(DRM_FORMAT_VYUY), > > > > + PixelFormat(DRM_FORMAT_NV16), > > > > + PixelFormat(DRM_FORMAT_NV61), > > > > + PixelFormat(DRM_FORMAT_NV21), > > > > + PixelFormat(DRM_FORMAT_NV12), > > > > + /* \todo Add support for 8-bit greyscale to DRM formats */ > > > > +}; > > > > + > > > > +} /* namespace */ > > > > + > > > > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > > > > : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) > > > > { > > > > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > { > > > > - static const std::array<PixelFormat, 8> formats{ > > > > - PixelFormat(DRM_FORMAT_YUYV), > > > > - PixelFormat(DRM_FORMAT_YVYU), > > > > - PixelFormat(DRM_FORMAT_VYUY), > > > > - PixelFormat(DRM_FORMAT_NV16), > > > > - PixelFormat(DRM_FORMAT_NV61), > > > > - PixelFormat(DRM_FORMAT_NV21), > > > > - PixelFormat(DRM_FORMAT_NV12), > > > > - /* \todo Add support for 8-bit greyscale to DRM formats */ > > > > - }; > > > > - > > > > const CameraSensor *sensor = data_->sensor_; > > > > Status status = Valid; > > > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > if (roles.empty()) > > > > return config; > > > > > > > > - StreamConfiguration cfg{}; > > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > > + > > > > + for (PixelFormat pixelformat : formats) { > > > > + std::vector<SizeRange> sizes{ > > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > > > I might have missed where these sizes come from. > > > > > > > + }; > > > > + pixelformats[pixelformat] = sizes; > > > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > > And you got a map of V4L2PixelFormats from the video device. > > > What I would do is going through all the PixelFormats, get their > > > V4L2PixelFormat counterpart, access the map using that as key to > > > retrive the size vector, and associated them with the PixelFormat you > > > started with. > > > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > > obtained from the PixelFormat array are valid, as the pipeline handler > > > should know that. > > > > > > Thanks > > > j > > > > > > > > > > + } > > > > + StreamFormats format(pixelformats); > > > > + StreamConfiguration cfg(format); > > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > > cfg.size = data->sensor_->resolution(); > > > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > > index ea3d214..96b3dc5 100644 > > > > --- a/src/libcamera/stream.cpp > > > > +++ b/src/libcamera/stream.cpp > > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > > * configured for a single video stream. > > > > */ > > > > > > > > -/** > > > > - * \todo This method is deprecated and should be removed once all pipeline > > > > - * handlers provied StreamFormats. > > > > - */ > > > > -StreamConfiguration::StreamConfiguration() > > > > - : pixelFormat(0), stream_(nullptr) > > > > -{ > > > > -} > > > > - > > > > /** > > > > * \brief Construct a configuration with stream formats > > > > */ > > > > -- > > > > 2.17.1 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kaaira, On Sat, Mar 21, 2020 at 08:05:38PM +0530, Kaaira Gupta wrote: > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: [snip] > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > if (roles.empty()) > > > return config; > > > > > > - StreamConfiguration cfg{}; > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > + > > > + for (PixelFormat pixelformat : formats) { > > > + std::vector<SizeRange> sizes{ > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > I might have missed where these sizes come from. > > I got these values from here, in the handler() validate() ? > cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width)); > cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height)); > > That is how I defined them. > Oh, sorry missed those. I'm not an expert of the platform, but from that piece of code I get it's lecit to assume for all pixel formats the valid size range is the one you reported. > > > > > + }; > > > + pixelformats[pixelformat] = sizes; > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > And you got a map of V4L2PixelFormats from the video device. > > What I would do is going through all the PixelFormats, get their > > V4L2PixelFormat counterpart, access the map using that as key to > > retrive the size vector, and associated them with the PixelFormat you > > started with. > > Please read the above logic behind the hardcoded values once and let me > know if I still have to map the sizes this way? No, I think it's fine the way it is, but I would wait for Niklas or Laurent to confirm this as they know this platform better. In the meantime, and that's for exercize, not because it's so critical at this point, be aware that this goes through 2 copies, if I'm not mistaken std::vector<SizeRange> sizes{ SizeRange{ { 32, 16 }, { 4416, 3312 } } }; pixelformats[pixelformat] = sizes; You construct a SizeRange, then copy it inside the vector, than copy the vector inside the map. The compiler might optimize the fist copy, but I think the second one stays. I think this could be reduced to SizeRange range({32, 16}, {4416, 3312}); pixelformats.emplace(std::piecewise_construct, std::forward_as_tuple(pixelformat), std::forward_as_tuple(1, range)); And someone which is more fluent in C++-voodoo than me could even be able to construct the Sizerange in place in the vector instead having to create an instance explicitly and copy it as I'm doing. This saves you the copy of of the vector in the map, by constructing the element in the map directly by forwarding the (1, range) arguments to the vector constructor. The gain is minumum but not so long ago the first time I saw a similar construct I was a bit puzzled, so I hope this could help decoding other parts of the library where we use it. Thanks j > > Thanks! > > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > obtained from the PixelFormat array are valid, as the pipeline handler > > should know that. > > > > Thanks > > j > > > > > > > + } > > > + StreamFormats format(pixelformats); > > > + StreamConfiguration cfg(format); > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > cfg.size = data->sensor_->resolution(); > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > index ea3d214..96b3dc5 100644 > > > --- a/src/libcamera/stream.cpp > > > +++ b/src/libcamera/stream.cpp > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > * configured for a single video stream. > > > */ > > > > > > -/** > > > - * \todo This method is deprecated and should be removed once all pipeline > > > - * handlers provied StreamFormats. > > > - */ > > > -StreamConfiguration::StreamConfiguration() > > > - : pixelFormat(0), stream_(nullptr) > > > -{ > > > -} > > > - > > > /** > > > * \brief Construct a configuration with stream formats > > > */ > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
On Sat, Mar 21, 2020 at 05:28:56PM +0100, Jacopo Mondi wrote: > Hi Kaaira, > > On Sat, Mar 21, 2020 at 08:05:38PM +0530, Kaaira Gupta wrote: > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > [snip] > > > > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > if (roles.empty()) > > > > return config; > > > > > > > > - StreamConfiguration cfg{}; > > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > > + > > > > + for (PixelFormat pixelformat : formats) { > > > > + std::vector<SizeRange> sizes{ > > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > > > I might have missed where these sizes come from. > > > > I got these values from here, in the handler() > > validate() ? Yes, sorry. > > > cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width)); > > cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height)); > > > > That is how I defined them. > > > > Oh, sorry missed those. I'm not an expert of the platform, but from > that piece of code I get it's lecit to assume for all pixel formats > the valid size range is the one you reported. > > > > > > > > > + }; > > > > + pixelformats[pixelformat] = sizes; > > > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > > And you got a map of V4L2PixelFormats from the video device. > > > What I would do is going through all the PixelFormats, get their > > > V4L2PixelFormat counterpart, access the map using that as key to > > > retrive the size vector, and associated them with the PixelFormat you > > > started with. > > > > Please read the above logic behind the hardcoded values once and let me > > know if I still have to map the sizes this way? > > No, I think it's fine the way it is, but I would wait for Niklas or > Laurent to confirm this as they know this platform better. Okay! > > In the meantime, and that's for exercize, not because it's so critical > at this point, be aware that this goes through 2 copies, if I'm not > mistaken > > std::vector<SizeRange> sizes{ > SizeRange{ { 32, 16 }, { 4416, 3312 } } > }; > pixelformats[pixelformat] = sizes; > > You construct a SizeRange, then copy it inside the vector, than copy > the vector inside the map. The compiler might optimize the fist copy, > but I think the second one stays. > > I think this could be reduced to > > SizeRange range({32, 16}, {4416, 3312}); > pixelformats.emplace(std::piecewise_construct, > std::forward_as_tuple(pixelformat), > std::forward_as_tuple(1, range)); > > And someone which is more fluent in C++-voodoo than me could even be > able to construct the Sizerange in place in the vector instead > having to create an instance explicitly and copy it as I'm doing. I'll try that for sure > > This saves you the copy of of the vector in the map, by constructing the > element in the map directly by forwarding the (1, range) arguments to the > vector constructor. The gain is minumum but not so long ago the first > time I saw a similar construct I was a bit puzzled, so I hope this > could help decoding other parts of the library where we use it. Thankyou so much for this help, it would surely have taken me time to decode it myself as I am more fluent with Java and Python. Had C++ as a course in just one sem.. Also, as for the error the compiler throws in validate(), if you get time please check my mails regarding that. Thanks! > > Thanks > j > > > > > Thanks! > > > > > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > > obtained from the PixelFormat array are valid, as the pipeline handler > > > should know that. > > > > > > Thanks > > > j > > > > > > > > > > + } > > > > + StreamFormats format(pixelformats); > > > > + StreamConfiguration cfg(format); > > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > > cfg.size = data->sensor_->resolution(); > > > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > > index ea3d214..96b3dc5 100644 > > > > --- a/src/libcamera/stream.cpp > > > > +++ b/src/libcamera/stream.cpp > > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > > * configured for a single video stream. > > > > */ > > > > > > > > -/** > > > > - * \todo This method is deprecated and should be removed once all pipeline > > > > - * handlers provied StreamFormats. > > > > - */ > > > > -StreamConfiguration::StreamConfiguration() > > > > - : pixelFormat(0), stream_(nullptr) > > > > -{ > > > > -} > > > > - > > > > /** > > > > * \brief Construct a configuration with stream formats > > > > */ > > > > -- > > > > 2.17.1 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kaaira, On Sat, Mar 21, 2020 at 10:40:08PM +0530, Kaaira Gupta wrote: > On Sat, Mar 21, 2020 at 05:28:56PM +0100, Jacopo Mondi wrote: > > Hi Kaaira, > > > > On Sat, Mar 21, 2020 at 08:05:38PM +0530, Kaaira Gupta wrote: > > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > > > [snip] > > > > > > > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > > if (roles.empty()) > > > > > return config; > > > > > > > > > > - StreamConfiguration cfg{}; > > > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > > > + > > > > > + for (PixelFormat pixelformat : formats) { > > > > > + std::vector<SizeRange> sizes{ > > > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > > > > > I might have missed where these sizes come from. > > > > > > I got these values from here, in the handler() > > > > validate() ? > > Yes, sorry. > > > > > > cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width)); > > > cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height)); > > > > > > That is how I defined them. > > > > > > > Oh, sorry missed those. I'm not an expert of the platform, but from > > that piece of code I get it's lecit to assume for all pixel formats > > the valid size range is the one you reported. > > > > > > > > > > > > > + }; > > > > > + pixelformats[pixelformat] = sizes; > > > > > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > > > And you got a map of V4L2PixelFormats from the video device. > > > > What I would do is going through all the PixelFormats, get their > > > > V4L2PixelFormat counterpart, access the map using that as key to > > > > retrive the size vector, and associated them with the PixelFormat you > > > > started with. > > > > > > Please read the above logic behind the hardcoded values once and let me > > > know if I still have to map the sizes this way? > > > > No, I think it's fine the way it is, but I would wait for Niklas or > > Laurent to confirm this as they know this platform better. > > Okay! > > > > > In the meantime, and that's for exercize, not because it's so critical > > at this point, be aware that this goes through 2 copies, if I'm not > > mistaken > > > > std::vector<SizeRange> sizes{ > > SizeRange{ { 32, 16 }, { 4416, 3312 } } > > }; > > pixelformats[pixelformat] = sizes; > > > > You construct a SizeRange, then copy it inside the vector, than copy > > the vector inside the map. The compiler might optimize the fist copy, > > but I think the second one stays. > > > > I think this could be reduced to > > > > SizeRange range({32, 16}, {4416, 3312}); > > pixelformats.emplace(std::piecewise_construct, > > std::forward_as_tuple(pixelformat), > > std::forward_as_tuple(1, range)); > > > > And someone which is more fluent in C++-voodoo than me could even be > > able to construct the Sizerange in place in the vector instead > > having to create an instance explicitly and copy it as I'm doing. > > I'll try that for sure > > > > > This saves you the copy of of the vector in the map, by constructing the > > element in the map directly by forwarding the (1, range) arguments to the > > vector constructor. The gain is minumum but not so long ago the first > > time I saw a similar construct I was a bit puzzled, so I hope this > > could help decoding other parts of the library where we use it. > > Thankyou so much for this help, it would surely have taken me time to > decode it myself as I am more fluent with Java and Python. Had C++ as a > course in just one sem.. > > Also, as for the error the compiler throws in validate(), if you get > time please check my mails regarding that. > I did, I'm not sure you have read the clarification were I suggested to replace resize() with erase() :) > Thanks! > > > > > Thanks > > j > > > > > > > > Thanks! > > > > > > > > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > > > obtained from the PixelFormat array are valid, as the pipeline handler > > > > should know that. > > > > > > > > Thanks > > > > j > > > > > > > > > > > > > + } > > > > > + StreamFormats format(pixelformats); > > > > > + StreamConfiguration cfg(format); > > > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > > > cfg.size = data->sensor_->resolution(); > > > > > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > > > index ea3d214..96b3dc5 100644 > > > > > --- a/src/libcamera/stream.cpp > > > > > +++ b/src/libcamera/stream.cpp > > > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > > > * configured for a single video stream. > > > > > */ > > > > > > > > > > -/** > > > > > - * \todo This method is deprecated and should be removed once all pipeline > > > > > - * handlers provied StreamFormats. > > > > > - */ > > > > > -StreamConfiguration::StreamConfiguration() > > > > > - : pixelFormat(0), stream_(nullptr) > > > > > -{ > > > > > -} > > > > > - > > > > > /** > > > > > * \brief Construct a configuration with stream formats > > > > > */ > > > > > -- > > > > > 2.17.1 > > > > > > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
On Sat, Mar 21, 2020 at 06:27:54PM +0100, Jacopo Mondi wrote: > Hi Kaaira, > > On Sat, Mar 21, 2020 at 10:40:08PM +0530, Kaaira Gupta wrote: > > On Sat, Mar 21, 2020 at 05:28:56PM +0100, Jacopo Mondi wrote: > > > Hi Kaaira, > > > > > > On Sat, Mar 21, 2020 at 08:05:38PM +0530, Kaaira Gupta wrote: > > > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > > > > > [snip] > > > > > > > > > > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > > > if (roles.empty()) > > > > > > return config; > > > > > > > > > > > > - StreamConfiguration cfg{}; > > > > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > > > > + > > > > > > + for (PixelFormat pixelformat : formats) { > > > > > > + std::vector<SizeRange> sizes{ > > > > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > > > > > > > I might have missed where these sizes come from. > > > > > > > > I got these values from here, in the handler() > > > > > > validate() ? > > > > Yes, sorry. > > > > > > > > > cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width)); > > > > cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height)); > > > > > > > > That is how I defined them. > > > > > > > > > > Oh, sorry missed those. I'm not an expert of the platform, but from > > > that piece of code I get it's lecit to assume for all pixel formats > > > the valid size range is the one you reported. > > > > > > > > > > > > > > > > > + }; > > > > > > + pixelformats[pixelformat] = sizes; > > > > > > > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > > > > And you got a map of V4L2PixelFormats from the video device. > > > > > What I would do is going through all the PixelFormats, get their > > > > > V4L2PixelFormat counterpart, access the map using that as key to > > > > > retrive the size vector, and associated them with the PixelFormat you > > > > > started with. > > > > > > > > Please read the above logic behind the hardcoded values once and let me > > > > know if I still have to map the sizes this way? > > > > > > No, I think it's fine the way it is, but I would wait for Niklas or > > > Laurent to confirm this as they know this platform better. > > > > Okay! > > > > > > > > In the meantime, and that's for exercize, not because it's so critical > > > at this point, be aware that this goes through 2 copies, if I'm not > > > mistaken > > > > > > std::vector<SizeRange> sizes{ > > > SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > }; > > > pixelformats[pixelformat] = sizes; > > > > > > You construct a SizeRange, then copy it inside the vector, than copy > > > the vector inside the map. The compiler might optimize the fist copy, > > > but I think the second one stays. > > > > > > I think this could be reduced to > > > > > > SizeRange range({32, 16}, {4416, 3312}); > > > pixelformats.emplace(std::piecewise_construct, > > > std::forward_as_tuple(pixelformat), > > > std::forward_as_tuple(1, range)); > > > > > > And someone which is more fluent in C++-voodoo than me could even be > > > able to construct the Sizerange in place in the vector instead > > > having to create an instance explicitly and copy it as I'm doing. > > > > I'll try that for sure > > > > > > > > This saves you the copy of of the vector in the map, by constructing the > > > element in the map directly by forwarding the (1, range) arguments to the > > > vector constructor. The gain is minumum but not so long ago the first > > > time I saw a similar construct I was a bit puzzled, so I hope this > > > could help decoding other parts of the library where we use it. > > > > Thankyou so much for this help, it would surely have taken me time to > > decode it myself as I am more fluent with Java and Python. Had C++ as a > > course in just one sem.. > > > > Also, as for the error the compiler throws in validate(), if you get > > time please check my mails regarding that. > > > > I did, I'm not sure you have read the clarification were I suggested to > replace resize() with erase() :) Yes I did, and yes using clear() worked. But I was confused as to why the compiler would worry about the size being less than the resize size when we already check for it before sending it to resize. But now I understand that it's the property of resize() to check for both larger and smaller size whether or not we check beforehand. Hence, it's clear to me now. Thanks > > > Thanks! > > > > > > > > Thanks > > > j > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > > > > obtained from the PixelFormat array are valid, as the pipeline handler > > > > > should know that. > > > > > > > > > > Thanks > > > > > j > > > > > > > > > > > > > > > > + } > > > > > > + StreamFormats format(pixelformats); > > > > > > + StreamConfiguration cfg(format); > > > > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > > > > cfg.size = data->sensor_->resolution(); > > > > > > > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > > > > index ea3d214..96b3dc5 100644 > > > > > > --- a/src/libcamera/stream.cpp > > > > > > +++ b/src/libcamera/stream.cpp > > > > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > > > > * configured for a single video stream. > > > > > > */ > > > > > > > > > > > > -/** > > > > > > - * \todo This method is deprecated and should be removed once all pipeline > > > > > > - * handlers provied StreamFormats. > > > > > > - */ > > > > > > -StreamConfiguration::StreamConfiguration() > > > > > > - : pixelFormat(0), stream_(nullptr) > > > > > > -{ > > > > > > -} > > > > > > - > > > > > > /** > > > > > > * \brief Construct a configuration with stream formats > > > > > > */ > > > > > > -- > > > > > > 2.17.1 > > > > > > > > > > > > _______________________________________________ > > > > > > libcamera-devel mailing list > > > > > > libcamera-devel@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kaaira, On Sat, Mar 21, 2020 at 11:29:48PM +0530, Kaaira Gupta wrote: > On Sat, Mar 21, 2020 at 06:27:54PM +0100, Jacopo Mondi wrote: > > Hi Kaaira, > > > > On Sat, Mar 21, 2020 at 10:40:08PM +0530, Kaaira Gupta wrote: > > > On Sat, Mar 21, 2020 at 05:28:56PM +0100, Jacopo Mondi wrote: > > > > Hi Kaaira, > > > > > > > > On Sat, Mar 21, 2020 at 08:05:38PM +0530, Kaaira Gupta wrote: > > > > > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote: > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > > > > if (roles.empty()) > > > > > > > return config; > > > > > > > > > > > > > > - StreamConfiguration cfg{}; > > > > > > > + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; > > > > > > > + > > > > > > > + for (PixelFormat pixelformat : formats) { > > > > > > > + std::vector<SizeRange> sizes{ > > > > > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > > > > > > > > > I might have missed where these sizes come from. > > > > > > > > > > I got these values from here, in the handler() > > > > > > > > validate() ? > > > > > > Yes, sorry. > > > > > > > > > > > > cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width)); > > > > > cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height)); > > > > > > > > > > That is how I defined them. > > > > > > > > > > > > > Oh, sorry missed those. I'm not an expert of the platform, but from > > > > that piece of code I get it's lecit to assume for all pixel formats > > > > the valid size range is the one you reported. > > > > > > > > > > > > > > > > > > > > > + }; > > > > > > > + pixelformats[pixelformat] = sizes; > > > > > > > > > > > > So you have an hardcoded list of PixelFormats in the pipeline handler. > > > > > > And you got a map of V4L2PixelFormats from the video device. > > > > > > What I would do is going through all the PixelFormats, get their > > > > > > V4L2PixelFormat counterpart, access the map using that as key to > > > > > > retrive the size vector, and associated them with the PixelFormat you > > > > > > started with. > > > > > > > > > > Please read the above logic behind the hardcoded values once and let me > > > > > know if I still have to map the sizes this way? > > > > > > > > No, I think it's fine the way it is, but I would wait for Niklas or > > > > Laurent to confirm this as they know this platform better. > > > > > > Okay! > > > > > > > > > > > In the meantime, and that's for exercize, not because it's so critical > > > > at this point, be aware that this goes through 2 copies, if I'm not > > > > mistaken > > > > > > > > std::vector<SizeRange> sizes{ > > > > SizeRange{ { 32, 16 }, { 4416, 3312 } } > > > > }; > > > > pixelformats[pixelformat] = sizes; > > > > > > > > You construct a SizeRange, then copy it inside the vector, than copy > > > > the vector inside the map. The compiler might optimize the fist copy, > > > > but I think the second one stays. > > > > > > > > I think this could be reduced to > > > > > > > > SizeRange range({32, 16}, {4416, 3312}); > > > > pixelformats.emplace(std::piecewise_construct, > > > > std::forward_as_tuple(pixelformat), > > > > std::forward_as_tuple(1, range)); > > > > > > > > And someone which is more fluent in C++-voodoo than me could even be > > > > able to construct the Sizerange in place in the vector instead > > > > having to create an instance explicitly and copy it as I'm doing. > > > > > > I'll try that for sure > > > > > > > > > > > This saves you the copy of of the vector in the map, by constructing the > > > > element in the map directly by forwarding the (1, range) arguments to the > > > > vector constructor. The gain is minumum but not so long ago the first > > > > time I saw a similar construct I was a bit puzzled, so I hope this > > > > could help decoding other parts of the library where we use it. > > > > > > Thankyou so much for this help, it would surely have taken me time to > > > decode it myself as I am more fluent with Java and Python. Had C++ as a > > > course in just one sem.. > > > > > > Also, as for the error the compiler throws in validate(), if you get > > > time please check my mails regarding that. > > > > > > > I did, I'm not sure you have read the clarification were I suggested to > > replace resize() with erase() :) > > Yes I did, and yes using clear() worked. But I was confused as to why the compiler > would worry about the size being less than the resize size when we > already check for it before sending it to resize. But now I understand > that it's the property of resize() to check for both larger and smaller > size whether or not we check beforehand. Hence, it's clear to me now. > Great! be aware that clear() != erase() void clear(); (until C++11) Erases all elements from the container. After this call, size() returns zero. This is not what you want, I'm sure :) Thanks j > Thanks > > > > > > Thanks! > > > > > > > > > > > Thanks > > > > j > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > I don't think it is necessary to make sure all the V4L2PixelFormat > > > > > > obtained from the PixelFormat array are valid, as the pipeline handler > > > > > > should know that. > > > > > > > > > > > > Thanks > > > > > > j > > > > > > > > > > > > > > > > > > > + } > > > > > > > + StreamFormats format(pixelformats); > > > > > > > + StreamConfiguration cfg(format); > > > > > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > > > > > cfg.size = data->sensor_->resolution(); > > > > > > > > > > > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > > > > > > index ea3d214..96b3dc5 100644 > > > > > > > --- a/src/libcamera/stream.cpp > > > > > > > +++ b/src/libcamera/stream.cpp > > > > > > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > > > > > > * configured for a single video stream. > > > > > > > */ > > > > > > > > > > > > > > -/** > > > > > > > - * \todo This method is deprecated and should be removed once all pipeline > > > > > > > - * handlers provied StreamFormats. > > > > > > > - */ > > > > > > > -StreamConfiguration::StreamConfiguration() > > > > > > > - : pixelFormat(0), stream_(nullptr) > > > > > > > -{ > > > > > > > -} > > > > > > > - > > > > > > > /** > > > > > > > * \brief Construct a configuration with stream formats > > > > > > > */ > > > > > > > -- > > > > > > > 2.17.1 > > > > > > > > > > > > > > _______________________________________________ > > > > > > > libcamera-devel mailing list > > > > > > > libcamera-devel@lists.libcamera.org > > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index b1441f8..c19aed6 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -37,7 +37,6 @@ private: }; struct StreamConfiguration { - StreamConfiguration(); StreamConfiguration(const StreamFormats &formats); PixelFormat pixelFormat; @@ -52,8 +51,11 @@ struct StreamConfiguration { std::string toString() const; private: + StreamConfiguration(); Stream *stream_; StreamFormats formats_; + + friend class Stream; }; enum StreamRole { diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 2f909ce..bf97b53 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -218,6 +218,21 @@ private: Camera *activeCamera_; }; +namespace { + +static const std::array<PixelFormat, 8> formats{ + PixelFormat(DRM_FORMAT_YUYV), + PixelFormat(DRM_FORMAT_YVYU), + PixelFormat(DRM_FORMAT_VYUY), + PixelFormat(DRM_FORMAT_NV16), + PixelFormat(DRM_FORMAT_NV61), + PixelFormat(DRM_FORMAT_NV21), + PixelFormat(DRM_FORMAT_NV12), + /* \todo Add support for 8-bit greyscale to DRM formats */ +}; + +} /* namespace */ + RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) : pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe)) { @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, CameraConfiguration::Status RkISP1CameraConfiguration::validate() { - static const std::array<PixelFormat, 8> formats{ - PixelFormat(DRM_FORMAT_YUYV), - PixelFormat(DRM_FORMAT_YVYU), - PixelFormat(DRM_FORMAT_VYUY), - PixelFormat(DRM_FORMAT_NV16), - PixelFormat(DRM_FORMAT_NV61), - PixelFormat(DRM_FORMAT_NV21), - PixelFormat(DRM_FORMAT_NV12), - /* \todo Add support for 8-bit greyscale to DRM formats */ - }; - const CameraSensor *sensor = data_->sensor_; Status status = Valid; @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera if (roles.empty()) return config; - StreamConfiguration cfg{}; + std::map<PixelFormat, std::vector<SizeRange>> pixelformats; + + for (PixelFormat pixelformat : formats) { + std::vector<SizeRange> sizes{ + SizeRange{ { 32, 16 }, { 4416, 3312 } } + }; + pixelformats[pixelformat] = sizes; + } + StreamFormats format(pixelformats); + StreamConfiguration cfg(format); cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); cfg.size = data->sensor_->resolution(); diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index ea3d214..96b3dc5 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const * configured for a single video stream. */ -/** - * \todo This method is deprecated and should be removed once all pipeline - * handlers provied StreamFormats. - */ -StreamConfiguration::StreamConfiguration() - : pixelFormat(0), stream_(nullptr) -{ -} - /** * \brief Construct a configuration with stream formats */
Use of default constructor StreamConfiguration() in deprecated, hence make it private. Make Stream class a friend of StreamConfiguration as it uses the default constructor. Replace default constructor StreamConfiguration() by it's parametered counterpart by using StreamFormats in generateConfiguration() in rkisp1 Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- WIP: - It fails to build as other pipelines still use default constructor. - Even rkisp1 fails as it uses default constructor in its validate() function. - I have taken care of generateConfiguration() only. include/libcamera/stream.h | 4 ++- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++-------- src/libcamera/stream.cpp | 9 ------ 3 files changed, 28 insertions(+), 22 deletions(-)