apps: cam: Try raw role if default viewfinder role fails
diff mbox series

Message ID 20250423091208.2935632-1-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • apps: cam: Try raw role if default viewfinder role fails
Related show

Commit Message

Paul Elder April 23, 2025, 9:12 a.m. UTC 
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(-)

Comments

Kieran Bingham April 23, 2025, 1:26 p.m. UTC | #1 
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
>
Paul Elder April 24, 2025, 8:56 a.m. UTC | #2 
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@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@ideasonboard.com>


Thanks,

Paul

> 
> > -- 
> > 2.47.2
> >
Milan Zamazal April 26, 2025, 5:35 p.m. UTC | #3 
Hi Paul,

thank you for the patch.

Paul Elder <paul.elder@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@redhat.com>

> 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;
>  	case 1:
>  		if (roles[0] != StreamRole::Viewfinder) {
>  			qWarning() << "Only viewfinder supported for single stream";
Niklas Söderlund May 7, 2025, 7:34 p.m. UTC | #4 
Hi Paul,

Thanks for your work that is now now commit ee2b011b65c6 ("apps: cam: 
Try raw role if default viewfinder role fails").

I'm afraid this seems to breaks qcam for me.

    arm64 ~ # ./ktool/build/src/apps/qcam/qcam -c 'imx219 1-0010'
    qt.qpa.xcb: XKeyboard extension not present on the X server
    There is no XRandR 1.2 and later version available. There will be only fake screen(s) to use.
    [0:31:31.233222393] [794]  INFO IPAManager ipa_manager.cpp:137 libcamera is not installed. Adding '/root/ktool/build/src/ipa' to the IPA search path
    [0:31:31.237485661] [794]  WARN IPAManager ipa_manager.cpp:148 No IPA found in '/usr/local/lib/libcamera'
    [0:31:31.238603349] [794]  INFO Camera camera_manager.cpp:326 libcamera v0.5.0
    /usr/include/c++/14.2.1/bits/stl_vector.h:1130: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = libcamera::StreamRole; _Alloc = std::allocator<libcamera::StreamRole>; reference = libcamera::StreamRole&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
    Aborted (core dumped)

The gdb tracenack points to MainWindow::startCapture() and if I revert 
the commit it seems to solve the problem.

    arm64 ~ # ./ktool/build/src/apps/qcam/qcam -c 'imx219 1-0010'
    qt.qpa.xcb: XKeyboard extension not present on the X server
    There is no XRandR 1.2 and later version available. There will be only fake screen(s) to use.
    [0:28:16.505731015] [746]  INFO IPAManager ipa_manager.cpp:137 libcamera is not installed. Adding '/root/ktool/build/src/ipa' to the IPA search path
    [0:28:16.509698783] [746]  WARN IPAManager ipa_manager.cpp:148 No IPA found in '/usr/local/lib/libcamera'
    [0:28:16.510769671] [746]  INFO Camera camera_manager.cpp:326 libcamera v0.5.0
    [0:28:16.689332683] [746]  INFO Camera camera.cpp:1205 configuring streams: (0) 3280x2464-XRGB8888/Rec709/Rec709/None/Full
    Zero-copy enabled

      .... viewfinder displays live images ....

I have tested without the -c 'camera' option too, I then get the GUI to 
select a camera. And then the same crash as reported above with -c 
'camera' provided.

If I however force a viewfinder role as such,

    arm64 ~ # ./ktool/build/src/apps/qcam/qcam -c 'imx219 1-0010' -s role=viewfinder

I get a functional viewfinder again.

On 2025-04-23 18:12:08 +0900, Paul Elder wrote:
> 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;
>  	case 1:
>  		if (roles[0] != StreamRole::Viewfinder) {
>  			qWarning() << "Only viewfinder supported for single stream";
> -- 
> 2.47.2
>
Kieran Bingham May 7, 2025, 8:42 p.m. UTC | #5 
Quoting Niklas Söderlund (2025-05-07 20:34:16)
> Hi Paul,
> 
> Thanks for your work that is now now commit ee2b011b65c6 ("apps: cam: 
> Try raw role if default viewfinder role fails").
> 
> I'm afraid this seems to breaks qcam for me.

Eeep - I just tested here and found the same too.

There's an easy fix that I've just tested.

I'll write a commit message and send after dinner ;-)

--
Kieran


> 
>     arm64 ~ # ./ktool/build/src/apps/qcam/qcam -c 'imx219 1-0010'
>     qt.qpa.xcb: XKeyboard extension not present on the X server
>     There is no XRandR 1.2 and later version available. There will be only fake screen(s) to use.
>     [0:31:31.233222393] [794]  INFO IPAManager ipa_manager.cpp:137 libcamera is not installed. Adding '/root/ktool/build/src/ipa' to the IPA search path
>     [0:31:31.237485661] [794]  WARN IPAManager ipa_manager.cpp:148 No IPA found in '/usr/local/lib/libcamera'
>     [0:31:31.238603349] [794]  INFO Camera camera_manager.cpp:326 libcamera v0.5.0
>     /usr/include/c++/14.2.1/bits/stl_vector.h:1130: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = libcamera::StreamRole; _Alloc = std::allocator<libcamera::StreamRole>; reference = libcamera::StreamRole&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
>     Aborted (core dumped)
> 
> The gdb tracenack points to MainWindow::startCapture() and if I revert 
> the commit it seems to solve the problem.
> 
>     arm64 ~ # ./ktool/build/src/apps/qcam/qcam -c 'imx219 1-0010'
>     qt.qpa.xcb: XKeyboard extension not present on the X server
>     There is no XRandR 1.2 and later version available. There will be only fake screen(s) to use.
>     [0:28:16.505731015] [746]  INFO IPAManager ipa_manager.cpp:137 libcamera is not installed. Adding '/root/ktool/build/src/ipa' to the IPA search path
>     [0:28:16.509698783] [746]  WARN IPAManager ipa_manager.cpp:148 No IPA found in '/usr/local/lib/libcamera'
>     [0:28:16.510769671] [746]  INFO Camera camera_manager.cpp:326 libcamera v0.5.0
>     [0:28:16.689332683] [746]  INFO Camera camera.cpp:1205 configuring streams: (0) 3280x2464-XRGB8888/Rec709/Rec709/None/Full
>     Zero-copy enabled
> 
>       .... viewfinder displays live images ....
> 
> I have tested without the -c 'camera' option too, I then get the GUI to 
> select a camera. And then the same crash as reported above with -c 
> 'camera' provided.
> 
> If I however force a viewfinder role as such,
> 
>     arm64 ~ # ./ktool/build/src/apps/qcam/qcam -c 'imx219 1-0010' -s role=viewfinder
> 
> I get a functional viewfinder again.
> 
> On 2025-04-23 18:12:08 +0900, Paul Elder wrote:
> > 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;
> >       case 1:
> >               if (roles[0] != StreamRole::Viewfinder) {
> >                       qWarning() << "Only viewfinder supported for single stream";
> > -- 
> > 2.47.2
> > 
> 
> -- 
> Kind Regards,
> Niklas Söderlund

Patch
diff mbox series 

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";