[libcamera-devel] [PATCH 02/13] libcamera: pipeline: rkisp1: Breakout mainpath size and format constraints

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2020-08-20 10:11:35 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Aug 13, 2020 at 02:52:35AM +0200, Niklas Söderlund wrote:
> > Breakout the mainpath size and format constrains as it will be used in
> > more places then just validate(). While at it use the new helpers to
> > validate Size.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 32 +++++++++++++-----------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index e2b703fc09f7afda..a0e36a44d8d91260 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -37,6 +37,19 @@ namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(RkISP1)
> >
> > +static const Size RKISP1_RSZ_MP_SRC_MIN = { 32, 16 };
> > +static const Size RKISP1_RSZ_MP_SRC_MAX = { 4416, 3312 };
> 
> You could list-initialize these
> 
> > +static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> 
> All of these can be made constexprs
> Also, we usually prefer an anonymous namespace in place of static

I like both suggestions above.

> 
> Also, are you sure about the ALL_CAPITAL_NAME of the array of formats ?

I have picked the names directly from the kernel driver sources to make 
it easier to map between the two. I'm open to pick something else but I 
still find value in keeping a clear mapping between the two so I think I 
will keep it as is for now.

> 
> > +	formats::YUYV,
> > +	formats::YVYU,
> > +	formats::VYUY,
> > +	formats::NV16,
> > +	formats::NV61,
> > +	formats::NV21,
> > +	formats::NV12,
> > +	/* \todo Add support for 8-bit greyscale to DRM formats */
> > +};
> > +
> >  class PipelineHandlerRkISP1;
> >  class RkISP1ActionQueueBuffers;
> >
> > @@ -461,17 +474,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >
> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  {
> > -	static const std::array<PixelFormat, 7> formats{
> > -		formats::YUYV,
> > -		formats::YVYU,
> > -		formats::VYUY,
> > -		formats::NV16,
> > -		formats::NV61,
> > -		formats::NV21,
> > -		formats::NV12,
> > -		/* \todo Add support for 8-bit greyscale to DRM formats */
> > -	};
> > -
> >  	const CameraSensor *sensor = data_->sensor_;
> >  	Status status = Valid;
> >
> > @@ -487,8 +489,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  	StreamConfiguration &cfg = config_[0];
> >
> >  	/* Adjust the pixel format. */
> > -	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> > -	    formats.end()) {
> > +	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> > +		      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> >  		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> >  		cfg.pixelFormat = formats::NV12,
> >  		status = Adjusted;
> > @@ -525,8 +527,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  				/ sensorFormat_.size.width;
> >  	}
> >
> > -	cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> > -	cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> > +	cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> > +	cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> 
> You could concatenate the two
>         cfg.size.boundTo().expandTo();

I could but I find the above more readable ;-)

> 
> Nits apart, the patch itself looks good!
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
>    j
> 
> >
> >  	if (cfg.size != size) {
> >  		LOG(RkISP1, Debug)
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund