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

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

Commit Message

Jacopo Mondi March 7, 2023, 11:48 a.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>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart March 19, 2023, 8:02 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Mar 07, 2023 at 12:48:03PM +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.

The order in which roles are requested matters, applications must
specify them in decreasing order of priority. Why is it better to
priority the still capture role even if specified last ?

> $ 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>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 60ab7380034c..cd92e99a50b3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -643,6 +643,10 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	if (roles.empty())
>  		return config;
>  
> +	/* If still capture is requested, reserve the main path for it. */
> +	bool reserveMainPath = std::find(roles.cbegin(), roles.cend(),
> +					 StreamRole::StillCapture) != roles.cend();
> +
>  	/*
>  	 * 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
> @@ -661,6 +665,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;
>  
> @@ -702,7 +709,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  
>  		RkISP1Path *path;
>  
> -		if (useMainPath) {
> +		if (useMainPath && !reserveMainPath) {
>  			path = data->mainPath_;
>  			mainPathAvailable = false;
>  		} else {
Jacopo Mondi March 20, 2023, 7:27 a.m. UTC | #2 
Hi Laurent

On Sun, Mar 19, 2023 at 10:02:38PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 07, 2023 at 12:48:03PM +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.
>
> The order in which roles are requested matters, applications must
> specify them in decreasing order of priority. Why is it better to
> priority the still capture role even if specified last ?
>
> > $ cam -c1 --stream role=viewfinder --stream role=still

It is not about prioritizing it, it's about being capable of
supporting a configuration that can the hw support and we currently
fail to implement.

Regardless of the roles order, the platform can do

                { vf: 1080p; still: full-res }

I don't see a reason why we should expose to applications a
platform-specific constraint such as the fact that the secondary
output is limited in res compard to the first one, which requires them to
take that into account when configuring roles.

TL;DR application should be able to get the same config with
[vf; still] and [still; vf] as the hw is capable of doing that, it
simply was not implemented.

> > Camera camera.cpp:969 streams configuration: (0) 1920x1080-NV12 (1) 4208x3120-NV12
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 60ab7380034c..cd92e99a50b3 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -643,6 +643,10 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  	if (roles.empty())
> >  		return config;
> >
> > +	/* If still capture is requested, reserve the main path for it. */
> > +	bool reserveMainPath = std::find(roles.cbegin(), roles.cend(),
> > +					 StreamRole::StillCapture) != roles.cend();
> > +
> >  	/*
> >  	 * 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
> > @@ -661,6 +665,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;
> >
> > @@ -702,7 +709,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >
> >  		RkISP1Path *path;
> >
> > -		if (useMainPath) {
> > +		if (useMainPath && !reserveMainPath) {
> >  			path = data->mainPath_;
> >  			mainPathAvailable = false;
> >  		} else {
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart March 20, 2023, 11:31 p.m. UTC | #3 
Hi Jacopo,

On Mon, Mar 20, 2023 at 08:27:25AM +0100, Jacopo Mondi wrote:
> On Sun, Mar 19, 2023 at 10:02:38PM +0200, Laurent Pinchart wrote:
> > On Tue, Mar 07, 2023 at 12:48:03PM +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.
> >
> > The order in which roles are requested matters, applications must
> > specify them in decreasing order of priority. Why is it better to
> > priority the still capture role even if specified last ?
> >
> > > $ cam -c1 --stream role=viewfinder --stream role=still
> 
> It is not about prioritizing it, it's about being capable of
> supporting a configuration that can the hw support and we currently
> fail to implement.
> 
> Regardless of the roles order, the platform can do
> 
>                 { vf: 1080p; still: full-res }
> 
> I don't see a reason why we should expose to applications a
> platform-specific constraint such as the fact that the secondary
> output is limited in res compard to the first one, which requires them to
> take that into account when configuring roles.
> 
> TL;DR application should be able to get the same config with
> [vf; still] and [still; vf] as the hw is capable of doing that, it
> simply was not implemented.

What if the application wants to capture the 1080p resolution in an RGB
format ? If you always assign the selfpath to the viewfinder when a
still image capture role is specified, that won't be possible. It may
even be that the application would like to capture still images in a
downscaled resolution (supported by the selfpath) in YUV format, and use
an RGB format for CPU processing. The hardware supports that, but with
this patch it won't be possible using the generate configuration.

> > > Camera camera.cpp:969 streams configuration: (0) 1920x1080-NV12 (1) 4208x3120-NV12
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 60ab7380034c..cd92e99a50b3 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -643,6 +643,10 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > >  	if (roles.empty())
> > >  		return config;
> > >
> > > +	/* If still capture is requested, reserve the main path for it. */
> > > +	bool reserveMainPath = std::find(roles.cbegin(), roles.cend(),
> > > +					 StreamRole::StillCapture) != roles.cend();
> > > +
> > >  	/*
> > >  	 * 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
> > > @@ -661,6 +665,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;
> > >
> > > @@ -702,7 +709,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > >
> > >  		RkISP1Path *path;
> > >
> > > -		if (useMainPath) {
> > > +		if (useMainPath && !reserveMainPath) {
> > >  			path = data->mainPath_;
> > >  			mainPathAvailable = false;
> > >  		} else {
Jacopo Mondi March 21, 2023, 8:22 a.m. UTC | #4 
Hi Laurent

On Tue, Mar 21, 2023 at 01:31:46AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Mar 20, 2023 at 08:27:25AM +0100, Jacopo Mondi wrote:
> > On Sun, Mar 19, 2023 at 10:02:38PM +0200, Laurent Pinchart wrote:
> > > On Tue, Mar 07, 2023 at 12:48:03PM +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.
> > >
> > > The order in which roles are requested matters, applications must
> > > specify them in decreasing order of priority. Why is it better to
> > > priority the still capture role even if specified last ?
> > >
> > > > $ cam -c1 --stream role=viewfinder --stream role=still
> >
> > It is not about prioritizing it, it's about being capable of
> > supporting a configuration that can the hw support and we currently
> > fail to implement.
> >
> > Regardless of the roles order, the platform can do
> >
> >                 { vf: 1080p; still: full-res }
> >
> > I don't see a reason why we should expose to applications a
> > platform-specific constraint such as the fact that the secondary
> > output is limited in res compard to the first one, which requires them to
> > take that into account when configuring roles.
> >
> > TL;DR application should be able to get the same config with
> > [vf; still] and [still; vf] as the hw is capable of doing that, it
> > simply was not implemented.
>
> What if the application wants to capture the 1080p resolution in an RGB
> format ? If you always assign the selfpath to the viewfinder when a

I guess we can go on the whole day listing use cases that can or
cannot be achived with or without this patch.

> still image capture role is specified, that won't be possible. It may
> even be that the application would like to capture still images in a
> downscaled resolution (supported by the selfpath) in YUV format, and use
> an RGB format for CPU processing. The hardware supports that, but with
> this patch it won't be possible using the generate configuration.
>

Let's drop this patch and 3/4

> > > > Camera camera.cpp:969 streams configuration: (0) 1920x1080-NV12 (1) 4208x3120-NV12
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 60ab7380034c..cd92e99a50b3 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -643,6 +643,10 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > > >  	if (roles.empty())
> > > >  		return config;
> > > >
> > > > +	/* If still capture is requested, reserve the main path for it. */
> > > > +	bool reserveMainPath = std::find(roles.cbegin(), roles.cend(),
> > > > +					 StreamRole::StillCapture) != roles.cend();
> > > > +
> > > >  	/*
> > > >  	 * 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
> > > > @@ -661,6 +665,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;
> > > >
> > > > @@ -702,7 +709,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > > >
> > > >  		RkISP1Path *path;
> > > >
> > > > -		if (useMainPath) {
> > > > +		if (useMainPath && !reserveMainPath) {
> > > >  			path = data->mainPath_;
> > > >  			mainPathAvailable = false;
> > > >  		} else {
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 60ab7380034c..cd92e99a50b3 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -643,6 +643,10 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 	if (roles.empty())
 		return config;
 
+	/* If still capture is requested, reserve the main path for it. */
+	bool reserveMainPath = std::find(roles.cbegin(), roles.cend(),
+					 StreamRole::StillCapture) != roles.cend();
+
 	/*
 	 * 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
@@ -661,6 +665,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;
 
@@ -702,7 +709,7 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 
 		RkISP1Path *path;
 
-		if (useMainPath) {
+		if (useMainPath && !reserveMainPath) {
 			path = data->mainPath_;
 			mainPathAvailable = false;
 		} else {