[libcamera-devel] libcamera: rkisp1: Generate configuration from main path if only one role

16304 diff mbox series

The current logic for generating configurations assumes that we have
multiple roles. The consequence of this is that if only one role is
requested, and it is for viewfinder or recording, then the self path's
configuration generator would be used instead of the main path's. This
is what causes the default resolution on the rkisp1 pipeline handler to
be 1920x1920 (since it's the max resolution of the self path). Note that
the main path is still used for streaming, just that it is using self
path's default configuraion (if it isn't changed by the application).

This patch skips all the logic for determining which path to assign to
which role in the event that only one role is requested. In this case,
we simply generate the configuration from the math path. This makes the
default resolution for a single stream 2592x1944.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++++++
 1 file changed, 10 insertions(+)

Quoting Paul Elder via libcamera-devel (2022-06-22 08:46:08)
> The current logic for generating configurations assumes that we have
> multiple roles. The consequence of this is that if only one role is
> requested, and it is for viewfinder or recording, then the self path's
> configuration generator would be used instead of the main path's. This
> is what causes the default resolution on the rkisp1 pipeline handler to
> be 1920x1920 (since it's the max resolution of the self path). Note that
> the main path is still used for streaming, just that it is using self
> path's default configuraion (if it isn't changed by the application).
> 
> This patch skips all the logic for determining which path to assign to
> which role in the event that only one role is requested. In this case,
> we simply generate the configuration from the math path. This makes the

s/math/main/

> default resolution for a single stream 2592x1944.

This sounds like quite a large effect, but I suspect it's ok. If a
StreamRole is set to ViewFinder, should this default resolution be
smaller?



> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7cf36524..43b76e14 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -515,6 +515,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>         if (roles.empty())
>                 return config;
>  
> +       if (roles.size() == 1) {
> +               StreamConfiguration cfg = data->mainPath_->generateConfiguration(
> +                       data->sensor_->resolution());
> +
> +               config->addConfiguration(cfg);
> +               config->validate();
> +
> +               return config;
> +       }
> +
>         bool mainPathAvailable = true;
>         bool selfPathAvailable = true;
>         for (const StreamRole role : roles) {
> -- 
> 2.30.2
>

Hi Paul,

On Wed, Jun 22, 2022 at 04:46:08PM +0900, Paul Elder via libcamera-devel wrote:
> The current logic for generating configurations assumes that we have
> multiple roles. The consequence of this is that if only one role is
> requested, and it is for viewfinder or recording, then the self path's
> configuration generator would be used instead of the main path's. This
> is what causes the default resolution on the rkisp1 pipeline handler to
> be 1920x1920 (since it's the max resolution of the self path). Note that

This sounds like a bug, RkISP1Path::generateConfiguration() should take
the sensor aspect ratio into account. Could you check what is happening
there ?

> the main path is still used for streaming, just that it is using self
> path's default configuraion (if it isn't changed by the application).
> 
> This patch skips all the logic for determining which path to assign to
> which role in the event that only one role is requested. In this case,
> we simply generate the configuration from the math path. This makes the
> default resolution for a single stream 2592x1944.

I'm a bit bothered by the rationale here. It's not so much that
generateConfiguration() assumes that there *are* multiple roles, but
that there *can* be multiple roles because it assumes there are multiple
*paths*. What we need to drop is the assumption we have a self path (and
the commit message should mention that's because the ISP8000Nano in the
i.MX8MP has a main path only). The availability of the self path should
be detected (possibly at init() time and cached, or in
generateConfiguration() if it's cheap), and generateConfiguration()
should then act accordingly, restricting the configuration to a single
stream in that case.

There's a bit more work needed on the kernel side to not register the
self path for the i.MX8MP.

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7cf36524..43b76e14 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -515,6 +515,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() == 1) {
> +		StreamConfiguration cfg = data->mainPath_->generateConfiguration(
> +			data->sensor_->resolution());
> +
> +		config->addConfiguration(cfg);
> +		config->validate();
> +
> +		return config;
> +	}
> +
>  	bool mainPathAvailable = true;
>  	bool selfPathAvailable = true;
>  	for (const StreamRole role : roles) {