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

[Paul Elder]

On Wed, Apr 23, 2025 at 02:26:04PM +0100, Kieran Bingham wrote:
> 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 ?

It's the retain the previous behavior. If no role is specified it
defaults to viewfinder.

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

Good point, I think we can do this.

> 
> But that could be on top anyway.

Yes :)

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


Thanks,

Paul

> 
> > -- 
> > 2.47.2
> >