[libcamera-devel,02/13] libcamera: pipeline: rkisp1: Breakout mainpath size and format constraints
diff mbox series

Message ID 20200813005246.3265807-3-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: pipeline: rkisp1: Extend to support two streams
Related show

Commit Message

Niklas Söderlund Aug. 13, 2020, 12:52 a.m. UTC 
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@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 32 +++++++++++++-----------
 1 file changed, 17 insertions(+), 15 deletions(-)

Comments

Jacopo Mondi Aug. 20, 2020, 8:11 a.m. UTC | #1 
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@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

Also, are you sure about the ALL_CAPITAL_NAME of the array of 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 */
> +};
> +
>  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();

Nits apart, the patch itself looks good!
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
>  	if (cfg.size != size) {
>  		LOG(RkISP1, Debug)
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Aug. 20, 2020, 12:14 p.m. UTC | #2 
Hi Jacopo,

On 20/08/2020 09:11, 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@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
> 
> Also, are you sure about the ALL_CAPITAL_NAME of the array of 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 */
>> +};
>> +
>>  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();
> 

That looks neat, but does it also indicate we need a
  Size::clamp(min, max) ?

Or would that not make sense given the multi-dimensional properties of a
Size...


> Nits apart, the patch itself looks good!
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

With fixups above:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Thanks
>    j
> 
>>
>>  	if (cfg.size != size) {
>>  		LOG(RkISP1, Debug)
>> --
>> 2.28.0
>>
>> _______________________________________________
>> 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
>
Niklas Söderlund Sept. 13, 2020, 12:47 p.m. UTC | #3 
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@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@jmondi.org>
> 
> Thanks
>    j
> 
> >
> >  	if (cfg.size != size) {
> >  		LOG(RkISP1, Debug)
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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 };
+static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_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 */
+};
+
 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);
 
 	if (cfg.size != size) {
 		LOG(RkISP1, Debug)