apps: cam: Try raw role if default viewfinder role fails

23241 diff mbox series

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@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(-)

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@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@ideasonboard.com>

> -- 
> 2.47.2
>