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

[Kieran Bingham]

Quoting Paul Elder (2025-04-23 10:12:08)
> 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.
> 
> 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;

Is this necessary? Doesn't it stop the raw stream being handled in qcam
which could be supported with Milans' series ? Or perhaps this just
continues default behaviour ?

>         case 1:
>                 if (roles[0] != StreamRole::Viewfinder) {
>                         qWarning() << "Only viewfinder supported for single stream";

This all looks good - except here - do we need to change this ?

qcam /is/ capable of displaying RAW streams as a single stream for quite
some time now.

But that could be on top anyway.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> -- 
> 2.47.2
>