[libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use parametered constructor instead of default
Kaaira Gupta
kgupta at es.iitr.ac.in
Mon Mar 23 20:10:16 CET 2020
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 at 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
More information about the libcamera-devel
mailing list