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

[Jacopo Mondi]

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