[libcamera-devel,v2,3/4] libcamera: rkisp1: Reserve main path for StillCapture
diff mbox series

Message ID 20230222151917.669526-4-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: rkisp1: Fix generateConfiguration
Related show

Commit Message

Jacopo Mondi Feb. 22, 2023, 3:19 p.m. UTC 
The main output path can produce images in higher resolution and
should be reserved for the StillCapture role when a configuration
is generated.

Before this change if StillCapture was not requested first it got
assigned to the self-path and thus down-scaled to 1920x1920.

With this change, StillCapture can be asked last and it would take
precedence over other streams for the usage of the main path.

$ cam -c1 --stream role=viewfinder --stream role=still
Camera camera.cpp:969 streams configuration: (0) 1920x1080-NV12 (1) 4208x3120-NV12

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Paul Elder March 3, 2023, 8:42 a.m. UTC | #1 
On Wed, Feb 22, 2023 at 04:19:16PM +0100, Jacopo Mondi via libcamera-devel wrote:
> The main output path can produce images in higher resolution and
> should be reserved for the StillCapture role when a configuration
> is generated.
> 
> Before this change if StillCapture was not requested first it got
> assigned to the self-path and thus down-scaled to 1920x1920.
> 
> With this change, StillCapture can be asked last and it would take
> precedence over other streams for the usage of the main path.
> 
> $ cam -c1 --stream role=viewfinder --stream role=still
> Camera camera.cpp:969 streams configuration: (0) 1920x1080-NV12 (1) 4208x3120-NV12
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 05a7ba03b2d2..23ff859e3835 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -626,6 +626,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	if (roles.empty())
>  		return config;
>  
> +	/* If still capture is requested, reserve the main path for it. */
> +	bool reserveMainPath = false;
> +	for (const StreamRole role : roles) {
> +		if (role != StreamRole::StillCapture)
> +			continue;
> +
> +		reserveMainPath = true;
> +		break;
> +	}
> +

How about...

	bool reserveMainPath =
		std::find(roles.cbegin(), roles.cend(),
			  StreamRole::StillCapture) != roles.cend();

Which doesn't actually look that nice either. Oh well. Up to you.


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

>  	/*
>  	 * As the ISP can't output different color spaces for the main and self
>  	 * path, pick a sensible default color space based on the role of the
> @@ -644,6 +654,9 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  			if (!colorSpace)
>  				colorSpace = ColorSpace::Sycc;
>  
> +			/* Unlock usage of main path which was reserved. */
> +			reserveMainPath = false;
> +
>  			size = data->sensor_->resolution();
>  			break;
>  
> @@ -685,7 +698,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  
>  		RkISP1Path *path;
>  
> -		if (useMainPath) {
> +		if (useMainPath && !reserveMainPath) {
>  			path = data->mainPath_;
>  			mainPathAvailable = false;
>  		} else {
> -- 
> 2.39.0
>
Kieran Bingham March 7, 2023, 8:59 a.m. UTC | #2 
Quoting Paul Elder via libcamera-devel (2023-03-03 08:42:33)
> On Wed, Feb 22, 2023 at 04:19:16PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > The main output path can produce images in higher resolution and
> > should be reserved for the StillCapture role when a configuration
> > is generated.
> > 
> > Before this change if StillCapture was not requested first it got
> > assigned to the self-path and thus down-scaled to 1920x1920.
> > 
> > With this change, StillCapture can be asked last and it would take
> > precedence over other streams for the usage of the main path.
> > 
> > $ cam -c1 --stream role=viewfinder --stream role=still
> > Camera camera.cpp:969 streams configuration: (0) 1920x1080-NV12 (1) 4208x3120-NV12
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 05a7ba03b2d2..23ff859e3835 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -626,6 +626,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >       if (roles.empty())
> >               return config;
> >  
> > +     /* If still capture is requested, reserve the main path for it. */
> > +     bool reserveMainPath = false;
> > +     for (const StreamRole role : roles) {
> > +             if (role != StreamRole::StillCapture)
> > +                     continue;
> > +
> > +             reserveMainPath = true;
> > +             break;
> > +     }
> > +
> 
> How about...
> 
>         bool reserveMainPath =
>                 std::find(roles.cbegin(), roles.cend(),
>                           StreamRole::StillCapture) != roles.cend();
> 
> Which doesn't actually look that nice either. Oh well. Up to you.
> 
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

Either way there should be fine. Pauls way is a bit more concise?



> 
> >       /*
> >        * As the ISP can't output different color spaces for the main and self
> >        * path, pick a sensible default color space based on the role of the
> > @@ -644,6 +654,9 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >                       if (!colorSpace)
> >                               colorSpace = ColorSpace::Sycc;
> >  
> > +                     /* Unlock usage of main path which was reserved. */
> > +                     reserveMainPath = false;
> > +

If there were more paths we might need something different here - but I
believe this works from what I see.


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

> >                       size = data->sensor_->resolution();
> >                       break;
> >  
> > @@ -685,7 +698,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  
> >               RkISP1Path *path;
> >  
> > -             if (useMainPath) {
> > +             if (useMainPath && !reserveMainPath) {
> >                       path = data->mainPath_;
> >                       mainPathAvailable = false;
> >               } else {
> > -- 
> > 2.39.0
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 05a7ba03b2d2..23ff859e3835 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -626,6 +626,16 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 	if (roles.empty())
 		return config;
 
+	/* If still capture is requested, reserve the main path for it. */
+	bool reserveMainPath = false;
+	for (const StreamRole role : roles) {
+		if (role != StreamRole::StillCapture)
+			continue;
+
+		reserveMainPath = true;
+		break;
+	}
+
 	/*
 	 * As the ISP can't output different color spaces for the main and self
 	 * path, pick a sensible default color space based on the role of the
@@ -644,6 +654,9 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 			if (!colorSpace)
 				colorSpace = ColorSpace::Sycc;
 
+			/* Unlock usage of main path which was reserved. */
+			reserveMainPath = false;
+
 			size = data->sensor_->resolution();
 			break;
 
@@ -685,7 +698,7 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 
 		RkISP1Path *path;
 
-		if (useMainPath) {
+		if (useMainPath && !reserveMainPath) {
 			path = data->mainPath_;
 			mainPathAvailable = false;
 		} else {