[v2,2/2] pipeline: simple: Fix matching with empty media graphs

23034 diff mbox series

The match() function currently reports that it is not possible to create
any cameras if it encounters an empty media graph.

Fix this by looping over all media graphs and only returning false when
all of them fail to create a camera.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Changes in v2:
- prevent a mix of valid and invalid media devices from ending up in
  mediaDevices_
---
 src/libcamera/pipeline/simple/simple.cpp | 52 ++++++++++++++++--------
 1 file changed, 35 insertions(+), 17 deletions(-)

Quoting Paul Elder (2025-03-26 08:47:59)
> The match() function currently reports that it is not possible to create
> any cameras if it encounters an empty media graph.
> 
> Fix this by looping over all media graphs and only returning false when
> all of them fail to create a camera.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I think that SoB only appeared because I might have sent this patch to
the list on your behalf in the past as
https://patchwork.libcamera.org/patch/20849/.

But there's no functional changes in here from me that I'm aware of.

Anyway,

Patch 1/2 in this series addresses Laurents comments on
https://patchwork.libcamera.org/patch/20849/, and I've seen this work on
a complex device so I think it's helpful to land it.


> ---
> Changes in v2:
> - prevent a mix of valid and invalid media devices from ending up in
>   mediaDevices_
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 52 ++++++++++++++++--------
>  1 file changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e039bf35fc1..a6bdd6024a99 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -373,6 +373,9 @@ private:
>                 return static_cast<SimpleCameraData *>(camera->_d());
>         }
>  
> +       bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,
> +                        DeviceEnumerator *enumerator);
> +
>         std::vector<MediaEntity *> locateSensors(MediaDevice *media);
>         static int resetRoutingTable(V4L2Subdevice *subdev);
>  
> @@ -1532,25 +1535,13 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)
>         return 0;
>  }
>  
> -bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> +bool SimplePipelineHandler::matchDevice(MediaDevice *media,
> +                                       const SimplePipelineInfo &info,
> +                                       DeviceEnumerator *enumerator)
>  {
> -       const SimplePipelineInfo *info = nullptr;
>         unsigned int numStreams = 1;
> -       MediaDevice *media;
> -
> -       for (const SimplePipelineInfo &inf : supportedDevices) {
> -               DeviceMatch dm(inf.driver);
> -               media = acquireMediaDevice(enumerator, dm);
> -               if (media) {
> -                       info = &inf;
> -                       break;
> -               }
> -       }
> -
> -       if (!media)
> -               return false;
>  
> -       for (const auto &[name, streams] : info->converters) {
> +       for (const auto &[name, streams] : info.converters) {
>                 DeviceMatch converterMatch(name);
>                 converter_ = acquireMediaDevice(enumerator, converterMatch);
>                 if (converter_) {
> @@ -1559,7 +1550,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>                 }
>         }
>  
> -       swIspEnabled_ = info->swIspEnabled;
> +       swIspEnabled_ = info.swIspEnabled;
>  
>         /* Locate the sensors. */
>         std::vector<MediaEntity *> sensors = locateSensors(media);
> @@ -1678,6 +1669,33 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>         return registered;
>  }
>  
> +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> +{
> +       MediaDevice *media;
> +
> +       for (const SimplePipelineInfo &inf : supportedDevices) {
> +               DeviceMatch dm(inf.driver);
> +               while ((media = acquireMediaDevice(enumerator, dm))) {
> +                       /*
> +                        * If match succeeds, return true to let match() be
> +                        * called again on a new instance of the pipeline
> +                        * handler. Otherwise keep looping until we do
> +                        * successfully match one (or run out).
> +                        */
> +                       if (matchDevice(media, inf, enumerator)) {
> +                               LOG(SimplePipeline, Debug)
> +                                       << "Matched on device: "
> +                                       << media->deviceNode();
> +                               return true;
> +                       }
> +               }
> +
> +               clearMediaDevices();

Hrm, I was about to add a tag, but now I'm doubting myself.

Should clearMediaDevices be here or in the loop above before the next
call to acquireMediaDevice() ?

Or does the functionality of acquireMediaDevice rely on the ones we
don't support already being acquired so we don't try to acquire them
again in an infinite loop ?

(i.e. - is the loop operator based on the enumerator, or something
imposed by the media devices?)



> +       }
> +
> +       return false;
> +}
> +
>  V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
>  {
>         auto iter = entities_.find(entity);
> -- 
> 2.47.2
>

On Thu, Apr 24, 2025 at 12:39:00PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2025-03-26 08:47:59)
> > The match() function currently reports that it is not possible to create
> > any cameras if it encounters an empty media graph.
> > 
> > Fix this by looping over all media graphs and only returning false when
> > all of them fail to create a camera.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> I think that SoB only appeared because I might have sent this patch to
> the list on your behalf in the past as
> https://patchwork.libcamera.org/patch/20849/.
> 
> But there's no functional changes in here from me that I'm aware of.
> 
> Anyway,
> 
> Patch 1/2 in this series addresses Laurents comments on
> https://patchwork.libcamera.org/patch/20849/, and I've seen this work on
> a complex device so I think it's helpful to land it.
> 
> 
> > ---
> > Changes in v2:
> > - prevent a mix of valid and invalid media devices from ending up in
> >   mediaDevices_
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 52 ++++++++++++++++--------
> >  1 file changed, 35 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 6e039bf35fc1..a6bdd6024a99 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -373,6 +373,9 @@ private:
> >                 return static_cast<SimpleCameraData *>(camera->_d());
> >         }
> >  
> > +       bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,
> > +                        DeviceEnumerator *enumerator);
> > +
> >         std::vector<MediaEntity *> locateSensors(MediaDevice *media);
> >         static int resetRoutingTable(V4L2Subdevice *subdev);
> >  
> > @@ -1532,25 +1535,13 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)
> >         return 0;
> >  }
> >  
> > -bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > +bool SimplePipelineHandler::matchDevice(MediaDevice *media,
> > +                                       const SimplePipelineInfo &info,
> > +                                       DeviceEnumerator *enumerator)
> >  {
> > -       const SimplePipelineInfo *info = nullptr;
> >         unsigned int numStreams = 1;
> > -       MediaDevice *media;
> > -
> > -       for (const SimplePipelineInfo &inf : supportedDevices) {
> > -               DeviceMatch dm(inf.driver);
> > -               media = acquireMediaDevice(enumerator, dm);
> > -               if (media) {
> > -                       info = &inf;
> > -                       break;
> > -               }
> > -       }
> > -
> > -       if (!media)
> > -               return false;
> >  
> > -       for (const auto &[name, streams] : info->converters) {
> > +       for (const auto &[name, streams] : info.converters) {
> >                 DeviceMatch converterMatch(name);
> >                 converter_ = acquireMediaDevice(enumerator, converterMatch);
> >                 if (converter_) {
> > @@ -1559,7 +1550,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >                 }
> >         }
> >  
> > -       swIspEnabled_ = info->swIspEnabled;
> > +       swIspEnabled_ = info.swIspEnabled;
> >  
> >         /* Locate the sensors. */
> >         std::vector<MediaEntity *> sensors = locateSensors(media);
> > @@ -1678,6 +1669,33 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >         return registered;
> >  }
> >  
> > +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > +{
> > +       MediaDevice *media;
> > +
> > +       for (const SimplePipelineInfo &inf : supportedDevices) {
> > +               DeviceMatch dm(inf.driver);
> > +               while ((media = acquireMediaDevice(enumerator, dm))) {
> > +                       /*
> > +                        * If match succeeds, return true to let match() be
> > +                        * called again on a new instance of the pipeline
> > +                        * handler. Otherwise keep looping until we do
> > +                        * successfully match one (or run out).
> > +                        */
> > +                       if (matchDevice(media, inf, enumerator)) {
> > +                               LOG(SimplePipeline, Debug)
> > +                                       << "Matched on device: "
> > +                                       << media->deviceNode();
> > +                               return true;
> > +                       }
> > +               }
> > +
> > +               clearMediaDevices();
> 
> Hrm, I was about to add a tag, but now I'm doubting myself.
> 
> Should clearMediaDevices be here or in the loop above before the next
> call to acquireMediaDevice() ?

The idea is that every time matchDevice() returns false, we need to
rollback all acquireMediaDevice() calls with clearMediaDevices().
Really there are only two acquireMediaDevice() calls; one here, and one
inside matchDevice() to acquire the converter media device. That's the
main one that we're concerned about imo.

If the first acquireMediaDevice() here succeeds and the
acquireMediaDevice() inside matchDevice() fails, we need to
clearMediaDevices() before the next attempt of acquireMediaDevice() here
on the next supportedDevices.

(Otherwise mediaDevices_ will have a stray media device from the last
attempt that succeeded acquire on the main device but failed acquire on
the converter)

So I think clearMediaDevices() being here is correct.

Though I'm thinking that the while loop should probably just be an if.


Paul

> 
> Or does the functionality of acquireMediaDevice rely on the ones we
> don't support already being acquired so we don't try to acquire them
> again in an infinite loop ?
> 
> (i.e. - is the loop operator based on the enumerator, or something
> imposed by the media devices?)
> 
> 
> 
> > +       }
> > +
> > +       return false;
> > +}
> > +
> >  V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
> >  {
> >         auto iter = entities_.find(entity);
> > -- 
> > 2.47.2
> >