| Message ID | 20200323084430.GA7565@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, On 23/03/2020 08:44, Kaaira Gupta wrote: > Use of default constructor StreamConfiguration() is 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/ ? If so, both here and in the subject line. > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > Also, use erase(), instead of replace() in validate() to prevent it from > creating a new instance with empty constructor. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > > Changes since v1: > - replace resize() by erase() in validate() to prevent it from > creating a new instance with empty constructor. > Changes since v2: > - Rearaange the order of declaration of friend class. > - Add constructor implementation again to stream.cpp > - Corrections in range of erase() > > include/libcamera/stream.h | 4 ++- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++-------- > src/libcamera/stream.cpp | 6 ++-- > 3 files changed, 32 insertions(+), 17 deletions(-) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index b1441f8..a379ebb 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,6 +51,9 @@ struct StreamConfiguration { > std::string toString() const; > > private: > + friend class Stream; > + StreamConfiguration(); > + Because the change to move the StreamConfiguration(); to being private causes multiple breakage, I think we should have it as a separate change, so this has certainly already become a series :-) The series should thus fix all breakages (before it breaks) then move this at the end of the series. Probably along with the removal of the \todo > Stream *stream_; > StreamFormats formats_; > }; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 2f909ce..6839e3c 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; > > @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > /* Cap the number of entries to the available streams. */ > if (config_.size() > 1) { > - config_.resize(1); > + config_.erase(config_.begin() + 1, config_.end()); Interesting, how does this differ from .resize(1)? Ah, I see - it's required because if .resize() is used to 'increase' the size, it uses the default constructor, and that is no longer available. I think it would be useful to add a comment to explain /why/ we use .erase() instead of .resize() here... > status = Adjusted; > } > > @@ -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{ Where do the values for the following SizeRange come from? A comment to indicate could be beneficial. Presumably these are the minimum and maximum capabilities of the ISP output ? Ideally these values should be determined from the hardware driver, but maybe that's not possible right now. > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > + }; > + pixelformats[pixelformat] = sizes; > + } I think a newline could be added here, > + StreamFormats format(pixelformats); > + StreamConfiguration cfg(format); And probably here to aid readability. > 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..7e41378 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > */ > > /** > - * \todo This method is deprecated and should be removed once all pipeline > - * handlers provied StreamFormats. > - */ > +* \todo This method is deprecated and should be removed once all pipeline > +* handlers provied StreamFormats. > +*/ This looks like an unintentional change (just removing some whitespace on the comment?), and shouldn't be in the patch. Once the pipeline handlers all provide StreamFormats, a patch on top should remove this comment of course. > StreamConfiguration::StreamConfiguration() > : pixelFormat(0), stream_(nullptr) > { >
On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote: > Hi Kaaira, > > On 23/03/2020 08:44, Kaaira Gupta wrote: > > Use of default constructor StreamConfiguration() is 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/ ? If so, both here and in the subject line. > > > counterpart by using StreamFormats in generateConfiguration() in rkisp1 > > > > Also, use erase(), instead of replace() in validate() to prevent it from > > creating a new instance with empty constructor. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > > > Changes since v1: > > - replace resize() by erase() in validate() to prevent it from > > creating a new instance with empty constructor. > > Changes since v2: > > - Rearaange the order of declaration of friend class. > > - Add constructor implementation again to stream.cpp > > - Corrections in range of erase() > > > > include/libcamera/stream.h | 4 ++- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++-------- > > src/libcamera/stream.cpp | 6 ++-- > > 3 files changed, 32 insertions(+), 17 deletions(-) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index b1441f8..a379ebb 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,6 +51,9 @@ struct StreamConfiguration { > > std::string toString() const; > > > > private: > > + friend class Stream; > > + StreamConfiguration(); > > + > > Because the change to move the StreamConfiguration(); to being private > causes multiple breakage, I think we should have it as a separate > change, so this has certainly already become a series :-) Exactly, it does cause a lot of breakage. But I am having a hard time figuring out what all they are because I think I haven't figured it out right how I can run tests on one thing at a time or maybe 'stop' the tests on a particular thing? Can you please help me with it? :/ > > The series should thus fix all breakages (before it breaks) then move > this at the end of the series. Probably along with the removal of the \todo > > > > Stream *stream_; > > StreamFormats formats_; > > }; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 2f909ce..6839e3c 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; > > > > @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > /* Cap the number of entries to the available streams. */ > > if (config_.size() > 1) { > > - config_.resize(1); > > + config_.erase(config_.begin() + 1, config_.end()); > > Interesting, how does this differ from .resize(1)? > > Ah, I see - it's required because if .resize() is used to 'increase' the > size, it uses the default constructor, and that is no longer available. Yes > > I think it would be useful to add a comment to explain /why/ we use > .erase() instead of .resize() here... Okay I will > > > > status = Adjusted; > > } > > > > @@ -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{ > > Where do the values for the following SizeRange come from? A comment to > indicate could be beneficial. > > Presumably these are the minimum and maximum capabilities of the ISP > output ? Yes, I took them from here in 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)); > > Ideally these values should be determined from the hardware driver, but > maybe that's not possible right now. > > > > + SizeRange{ { 32, 16 }, { 4416, 3312 } } > > + }; > > + pixelformats[pixelformat] = sizes; > > + } > > I think a newline could be added here, > > > + StreamFormats format(pixelformats); > > + StreamConfiguration cfg(format); > > And probably here to aid readability. > > > 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..7e41378 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > > */ > > > > /** > > - * \todo This method is deprecated and should be removed once all pipeline > > - * handlers provied StreamFormats. > > - */ > > +* \todo This method is deprecated and should be removed once all pipeline > > +* handlers provied StreamFormats. > > +*/ > > This looks like an unintentional change (just removing some whitespace > on the comment?), and shouldn't be in the patch. Yes, I'll fix all the whitespace errors. > > Once the pipeline handlers all provide StreamFormats, a patch on top > should remove this comment of course. > > > StreamConfiguration::StreamConfiguration() > > : pixelFormat(0), stream_(nullptr) > > { > > > > -- > Regards > -- > Kieran
Hi Kaaira, On 23/03/2020 13:36, Kaaira Gupta wrote: > On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote: >> Hi Kaaira, >> >> On 23/03/2020 08:44, Kaaira Gupta wrote: >>> Use of default constructor StreamConfiguration() is 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/ ? If so, both here and in the subject line. >> >>> counterpart by using StreamFormats in generateConfiguration() in rkisp1 >>> >>> Also, use erase(), instead of replace() in validate() to prevent it from >>> creating a new instance with empty constructor. >>> >>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >>> --- >>> >>> Changes since v1: >>> - replace resize() by erase() in validate() to prevent it from >>> creating a new instance with empty constructor. >>> Changes since v2: >>> - Rearaange the order of declaration of friend class. >>> - Add constructor implementation again to stream.cpp >>> - Corrections in range of erase() >>> >>> include/libcamera/stream.h | 4 ++- >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++-------- >>> src/libcamera/stream.cpp | 6 ++-- >>> 3 files changed, 32 insertions(+), 17 deletions(-) >>> >>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h >>> index b1441f8..a379ebb 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,6 +51,9 @@ struct StreamConfiguration { >>> std::string toString() const; >>> >>> private: >>> + friend class Stream; >>> + StreamConfiguration(); >>> + >> >> Because the change to move the StreamConfiguration(); to being private >> causes multiple breakage, I think we should have it as a separate >> change, so this has certainly already become a series :-) > > Exactly, it does cause a lot of breakage. But I am having a hard time > figuring out what all they are because I think I haven't figured it out > right how I can run tests on one thing at a time or maybe 'stop' the > tests on a particular thing? Can you please help me with it? :/ I assume here you mean that one of the tests gets broken with the change? (test/v4l2_videodevice/buffer_cache I think) The important thing to remember here is that the change you make to the rkisp1 should still work without the change to the StreamConfiguration being made private, so I would recommend splitting that part out already. You should be able to test your change to RKISP1 (by ensuring that it compiles cleanly, - you don't have hardware to test it running) entirely by *not* including the patch which makes StreamConfiguration private. And indeed, we could expect a few patches that can be tested independently before the StreamConfiguration is moved... The series would end up looking something like this: - [1/n] pipeline: Remove use of .resize() in validate() calls. - [2/n] pipeline: rkisp1: Use paramaterized StreamConfiguration - [3/n] pipeline: ipu3: Use paramaterized StreamConfiguration - [4/n] test: Fix v4l2_videodevice/buffer_cache test - [5/n] stream: Make default constructor of StreamConfiguration private Patch 1 should fix all uses of the .resize() which we know won't work (so that's vimc, uvc, rkisp1, ipu3). Patch 2 should then handle the rkisp1 changes as you've already identified. You should be able to compile cleanly and run the test suite on both of those patches independently. Patch 3 can then apply the similar changes you've made to the RKISP.. Patch 4 should then fix the issue that will be faced in the buffer_cache test. I haven't looked at that yet to determine the solution. Is this the point you are currently blocked? If there are any other fixes needed, they'd go in here, and patch 5 would come later of course... Patch 5 should handle the privatisation of the StreamConfiguration default constructor. >> The series should thus fix all breakages (before it breaks) then move >> this at the end of the series. Probably along with the removal of the \todo >> >> >>> Stream *stream_; >>> StreamFormats formats_; >>> }; >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> index 2f909ce..6839e3c 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; >>> >>> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() >>> >>> /* Cap the number of entries to the available streams. */ >>> if (config_.size() > 1) { >>> - config_.resize(1); >>> + config_.erase(config_.begin() + 1, config_.end()); >> >> Interesting, how does this differ from .resize(1)? >> >> Ah, I see - it's required because if .resize() is used to 'increase' the >> size, it uses the default constructor, and that is no longer available. > > Yes > >> >> I think it would be useful to add a comment to explain /why/ we use >> .erase() instead of .resize() here... > > Okay I will If you group the vimc/uvc element of this change, it could potentially be a single patch making this change for all of the pipeline handlers at the same time, each one doing the 'right' change and adding the same comment or such. That patch would then come before the changes to generateConfiguration in this pipeline handler. > >> >> >>> status = Adjusted; >>> } >>> >>> @@ -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{ >> >> Where do the values for the following SizeRange come from? A comment to >> indicate could be beneficial. >> >> Presumably these are the minimum and maximum capabilities of the ISP >> output ? > > Yes, I took them from here in 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)); Ah yes, it would be nice if these were in a common location - but I don't think you need to worry about that for now in this patch. >> Ideally these values should be determined from the hardware driver, but >> maybe that's not possible right now. >> >> >>> + SizeRange{ { 32, 16 }, { 4416, 3312 } } >>> + }; >>> + pixelformats[pixelformat] = sizes; >>> + } >> >> I think a newline could be added here, >> >>> + StreamFormats format(pixelformats); >>> + StreamConfiguration cfg(format); >> >> And probably here to aid readability. >> >>> 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..7e41378 100644 >>> --- a/src/libcamera/stream.cpp >>> +++ b/src/libcamera/stream.cpp >>> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const >>> */ >>> >>> /** >>> - * \todo This method is deprecated and should be removed once all pipeline >>> - * handlers provied StreamFormats. >>> - */ >>> +* \todo This method is deprecated and should be removed once all pipeline >>> +* handlers provied StreamFormats. >>> +*/ >> >> This looks like an unintentional change (just removing some whitespace >> on the comment?), and shouldn't be in the patch. > > Yes, I'll fix all the whitespace errors. > >> >> Once the pipeline handlers all provide StreamFormats, a patch on top >> should remove this comment of course. >> >>> StreamConfiguration::StreamConfiguration() >>> : pixelFormat(0), stream_(nullptr) >>> { >>> >> >> -- >> Regards >> -- >> Kieran
On Mon, Mar 23, 2020 at 04:28:24PM +0000, Kieran Bingham wrote: > Hi Kaaira, > > On 23/03/2020 13:36, Kaaira Gupta wrote: > > On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote: > >> Hi Kaaira, > >> > >> On 23/03/2020 08:44, Kaaira Gupta wrote: > >>> Use of default constructor StreamConfiguration() is 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/ ? If so, both here and in the subject line. > >> > >>> counterpart by using StreamFormats in generateConfiguration() in rkisp1 > >>> > >>> Also, use erase(), instead of replace() in validate() to prevent it from > >>> creating a new instance with empty constructor. > >>> > >>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >>> --- > >>> > >>> Changes since v1: > >>> - replace resize() by erase() in validate() to prevent it from > >>> creating a new instance with empty constructor. > >>> Changes since v2: > >>> - Rearaange the order of declaration of friend class. > >>> - Add constructor implementation again to stream.cpp > >>> - Corrections in range of erase() > >>> > >>> include/libcamera/stream.h | 4 ++- > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++-------- > >>> src/libcamera/stream.cpp | 6 ++-- > >>> 3 files changed, 32 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > >>> index b1441f8..a379ebb 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,6 +51,9 @@ struct StreamConfiguration { > >>> std::string toString() const; > >>> > >>> private: > >>> + friend class Stream; > >>> + StreamConfiguration(); > >>> + > >> > >> Because the change to move the StreamConfiguration(); to being private > >> causes multiple breakage, I think we should have it as a separate > >> change, so this has certainly already become a series :-) > > > > Exactly, it does cause a lot of breakage. But I am having a hard time > > figuring out what all they are because I think I haven't figured it out > > right how I can run tests on one thing at a time or maybe 'stop' the > > tests on a particular thing? Can you please help me with it? :/ > > I assume here you mean that one of the tests gets broken with the > change? (test/v4l2_videodevice/buffer_cache I think) > > The important thing to remember here is that the change you make to the > rkisp1 should still work without the change to the StreamConfiguration > being made private, so I would recommend splitting that part out already. > > You should be able to test your change to RKISP1 (by ensuring that it > compiles cleanly, - you don't have hardware to test it running) entirely > by *not* including the patch which makes StreamConfiguration private. > > And indeed, we could expect a few patches that can be tested > independently before the StreamConfiguration is moved... > > The series would end up looking something like this: > > > - [1/n] pipeline: Remove use of .resize() in validate() calls. > - [2/n] pipeline: rkisp1: Use paramaterized StreamConfiguration > - [3/n] pipeline: ipu3: Use paramaterized StreamConfiguration > - [4/n] test: Fix v4l2_videodevice/buffer_cache test > - [5/n] stream: Make default constructor of StreamConfiguration private > > > Patch 1 should fix all uses of the .resize() which we know won't work > (so that's vimc, uvc, rkisp1, ipu3). > > Patch 2 should then handle the rkisp1 changes as you've already identified. > > You should be able to compile cleanly and run the test suite on both of > those patches independently. > > > Patch 3 can then apply the similar changes you've made to the RKISP.. > > > Patch 4 should then fix the issue that will be faced in the buffer_cache > test. I haven't looked at that yet to determine the solution. Is this > the point you are currently blocked? Yes, I was stuck here. Your mail clears this of-course..I'll send in the three patches, and we'll come on this after that. Thanks! > > > If there are any other fixes needed, they'd go in here, and patch 5 > would come later of course... > > > Patch 5 should handle the privatisation of the StreamConfiguration > default constructor. > > > > >> The series should thus fix all breakages (before it breaks) then move > >> this at the end of the series. Probably along with the removal of the \todo > >> > >> > >>> Stream *stream_; > >>> StreamFormats formats_; > >>> }; > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> index 2f909ce..6839e3c 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; > >>> > >>> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > >>> > >>> /* Cap the number of entries to the available streams. */ > >>> if (config_.size() > 1) { > >>> - config_.resize(1); > >>> + config_.erase(config_.begin() + 1, config_.end()); > >> > >> Interesting, how does this differ from .resize(1)? > >> > >> Ah, I see - it's required because if .resize() is used to 'increase' the > >> size, it uses the default constructor, and that is no longer available. > > > > Yes > > > >> > >> I think it would be useful to add a comment to explain /why/ we use > >> .erase() instead of .resize() here... > > > > Okay I will > > If you group the vimc/uvc element of this change, it could potentially > be a single patch making this change for all of the pipeline handlers at > the same time, each one doing the 'right' change and adding the same > comment or such. Okay > > That patch would then come before the changes to generateConfiguration > in this pipeline handler. > > > > > >> > >> > >>> status = Adjusted; > >>> } > >>> > >>> @@ -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{ > >> > >> Where do the values for the following SizeRange come from? A comment to > >> indicate could be beneficial. > >> > >> Presumably these are the minimum and maximum capabilities of the ISP > >> output ? > > > > Yes, I took them from here in 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)); > > Ah yes, it would be nice if these were in a common location - but I > don't think you need to worry about that for now in this patch. > > >> Ideally these values should be determined from the hardware driver, but > >> maybe that's not possible right now. > >> > >> > >>> + SizeRange{ { 32, 16 }, { 4416, 3312 } } > >>> + }; > >>> + pixelformats[pixelformat] = sizes; > >>> + } > >> > >> I think a newline could be added here, > >> > >>> + StreamFormats format(pixelformats); > >>> + StreamConfiguration cfg(format); > >> > >> And probably here to aid readability. > >> > >>> 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..7e41378 100644 > >>> --- a/src/libcamera/stream.cpp > >>> +++ b/src/libcamera/stream.cpp > >>> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > >>> */ > >>> > >>> /** > >>> - * \todo This method is deprecated and should be removed once all pipeline > >>> - * handlers provied StreamFormats. > >>> - */ > >>> +* \todo This method is deprecated and should be removed once all pipeline > >>> +* handlers provied StreamFormats. > >>> +*/ > >> > >> This looks like an unintentional change (just removing some whitespace > >> on the comment?), and shouldn't be in the patch. > > > > Yes, I'll fix all the whitespace errors. > > > >> > >> Once the pipeline handlers all provide StreamFormats, a patch on top > >> should remove this comment of course. > >> > >>> StreamConfiguration::StreamConfiguration() > >>> : pixelFormat(0), stream_(nullptr) > >>> { > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran Thanks! -- Kaaira
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index b1441f8..a379ebb 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,6 +51,9 @@ struct StreamConfiguration { std::string toString() const; private: + friend class Stream; + StreamConfiguration(); + Stream *stream_; StreamFormats formats_; }; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 2f909ce..6839e3c 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; @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() /* Cap the number of entries to the available streams. */ if (config_.size() > 1) { - config_.resize(1); + config_.erase(config_.begin() + 1, config_.end()); status = Adjusted; } @@ -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..7e41378 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const */ /** - * \todo This method is deprecated and should be removed once all pipeline - * handlers provied StreamFormats. - */ +* \todo This method is deprecated and should be removed once all pipeline +* handlers provied StreamFormats. +*/ StreamConfiguration::StreamConfiguration() : pixelFormat(0), stream_(nullptr) {
Use of default constructor StreamConfiguration() is 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 Also, use erase(), instead of replace() in validate() to prevent it from creating a new instance with empty constructor. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- Changes since v1: - replace resize() by erase() in validate() to prevent it from creating a new instance with empty constructor. Changes since v2: - Rearaange the order of declaration of friend class. - Add constructor implementation again to stream.cpp - Corrections in range of erase() include/libcamera/stream.h | 4 ++- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++-------- src/libcamera/stream.cpp | 6 ++-- 3 files changed, 32 insertions(+), 17 deletions(-)