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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Sep 14, 2020 at 04:21:38PM +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>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> * Changes since v1
> - Use list initializers
> - Use constexpr instead of static const
> - Define constants in anonymoys namespace
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 34 +++++++++++++-----------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 009d190d3ec828f0..86a3e3c63b17762b 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -37,6 +37,21 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(RkISP1)
>  
> +namespace {
> +	constexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };
> +	constexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };
> +	constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{

Either add a space before { here, or remove it on the two lines above.
We'll have to tidy up the coding style for list initialization, it's
inconsistent at the moment.

We will also need to tidy up the coding style for naming of constants,
the existing code base uses a mix of camelCase and SNAKE_CASE. While
macros should be SNAKE_CASE, constexpr constants don't need to be. I'm
leaning for camelCase, possibly with a way to express that they're
constants (adding a 'k' prefix is often used for that purpose, even if
I'm not necessarily fond of it). The debate is open :-)

> +		formats::YUYV,
> +		formats::YVYU,
> +		formats::VYUY,
> +		formats::NV16,
> +		formats::NV61,
> +		formats::NV21,
> +		formats::NV12,
> +		/* \todo Add support for 8-bit greyscale to DRM formats */

On a side note, that's formats::R8 :-)

> +	};

No need for the extra level of indentation.

> +}

} /* namespace */

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
>  class PipelineHandlerRkISP1;
>  class RkISP1ActionQueueBuffers;
>  
> @@ -461,17 +476,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 +491,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 +529,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);
>  
>  	if (cfg.size != size) {
>  		LOG(RkISP1, Debug)

-- 
Regards,

Laurent Pinchart