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

[Jacopo Mondi]

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 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