[libcamera-devel] [PATCH v3 3/4] libcamera: rkisp1: Reserve main path for StillCapture

[Laurent Pinchart]

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 at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at 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