[PATCH] apps: cam: Try raw role if default viewfinder role fails

[Milan Zamazal]

Hi Paul,

thank you for the patch.

Paul Elder <paul.elder at ideasonboard.com> writes:

> cam currently defaults to the viewfinder role when no role is specified.
> This means that on platforms that only support the raw role (such as a
> raw sensor with no softISP on a simple pipeline platform),
> generateConfiguration() would return nullptr and cam would bail out.
>
> At least this is what is supposed to happen based on the little
> documentation that we have written regarding generateConfiguration(),
> specifically in the application writer's guide, which is probably the
> most influential piece of documentation:
>
>   The ``Camera::generateConfiguration()`` function accepts a list of
>   desired roles and generates a ``CameraConfiguration`` with the best
>   stream parameters configuration for each of the requested roles. If the
>   camera can handle the requested roles, it returns an initialized
>   ``CameraConfiguration`` and a null pointer if it can't.
>
> Currently the simple pipeline handler will return a raw configuration
> anyway (if it only supports raw) even if a non-raw role was requested.
> Thus cam receives a raw configuration instead of a nullptr when no role
> is specified and viewfinder is requested.
>
> However, in the near future, support for raw streams with softISP on the
> simple pipeline handler will be merged. This will notably change the
> behavior of the simple pipeline handler to return nullptr if a non-raw
> role was requested on a platform that only supports raw. This is proper
> behavior according to documentation, but changes cam's behavior as it
> used to capture fine with no parameters but will no longer be able to.
>
> Technically this is an issue with the roles API, as we are mixing
> roles in the sense of "configuration hints" (eg. viewfinder vs recording
> vs still capture) with roles in the sense of "platform capabilities"
> (raw vs everything else). In the long term the proper solution is to
> rework the roles API.
>
> In the meantime, fix cam so that it will try the raw role if the default
> viewfinder role returns no configuration. cam is an app that is capable
> of using the raw stream, so this is appropriate behavior. If roles are
> specified, then do not retry, as in this situation the user knows what
> streams they can use and what they want.

Makes sense to me and I cannot see any problem related to my work on raw
streams.

Reviewed-by: Milan Zamazal <mzamazal at redhat.com>

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/apps/cam/camera_session.cpp    | 29 +++++++++++++++++++++++++----
>  src/apps/common/stream_options.cpp |  3 +--
>  src/apps/qcam/main_window.cpp      |  3 +++
>  3 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> index 97c1ae44995e..f63fcb228519 100644
> --- a/src/apps/cam/camera_session.cpp
> +++ b/src/apps/cam/camera_session.cpp
> @@ -62,11 +62,32 @@ CameraSession::CameraSession(CameraManager *cm,
>  		return;
>  	}
>  
> -	std::vector<StreamRole> roles = StreamKeyValueParser::roles(options_[OptStream]);
> +	std::vector<StreamRole> roles =
> +		StreamKeyValueParser::roles(options_[OptStream]);
> +	std::vector<std::vector<StreamRole>> tryRoles;
> +	if (!roles.empty()) {
> +		/*
> +		 * If the roles are explicitly specified then there's no need
> +		 * to try other roles
> +		 */
> +		tryRoles.push_back(roles);
> +	} else {
> +		tryRoles.push_back({ StreamRole::Viewfinder });
> +		tryRoles.push_back({ StreamRole::Raw });
> +	}
> +
> +	std::unique_ptr<CameraConfiguration> config;
> +	bool valid = false;
> +	for (std::vector<StreamRole> &rolesIt : tryRoles) {
> +		config = camera_->generateConfiguration(rolesIt);
> +		if (config && config->size() == rolesIt.size()) {
> +			roles = rolesIt;
> +			valid = true;
> +			break;
> +		}
> +	}
>  
> -	std::unique_ptr<CameraConfiguration> config =
> -		camera_->generateConfiguration(roles);
> -	if (!config || config->size() != roles.size()) {
> +	if (!valid) {
>  		std::cerr << "Failed to get default stream configuration"
>  			  << std::endl;
>  		return;
> diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
> index 99239e07e302..288f86530351 100644
> --- a/src/apps/common/stream_options.cpp
> +++ b/src/apps/common/stream_options.cpp
> @@ -42,9 +42,8 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
>  
>  std::vector<StreamRole> StreamKeyValueParser::roles(const OptionValue &values)
>  {
> -	/* If no configuration values to examine default to viewfinder. */
>  	if (values.empty())
> -		return { StreamRole::Viewfinder };
> +		return {};
>  
>  	const std::vector<OptionValue> &streamParameters = values.toArray();
>  
> diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> index d2ccbd2318fa..224a7e5a693a 100644
> --- a/src/apps/qcam/main_window.cpp
> +++ b/src/apps/qcam/main_window.cpp
> @@ -356,6 +356,9 @@ int MainWindow::startCapture()
>  
>  	/* Verify roles are supported. */
>  	switch (roles.size()) {
> +	case 0:
> +		roles[0] = StreamRole::Viewfinder;
> +		break;
>  	case 1:
>  		if (roles[0] != StreamRole::Viewfinder) {
>  			qWarning() << "Only viewfinder supported for single stream";