[PATCH v2 2/2] pipeline: simple: Fix matching with empty media graphs
Paul Elder
paul.elder at ideasonboard.com
Mon Apr 28 18:12:10 CEST 2025
On Fri, Apr 25, 2025 at 05:52:34PM +0900, Paul Elder wrote:
> 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 at ideasonboard.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham at 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() ?
Notes from discussion (also cc'ing Laurent):
> 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.
The while loop is correct, as we still need to try other *instances* of
the media-device-name.
Which is why clearMediaDevices() should actually be inside the while
loop at the end, to clear the mediaDevices_ list when matchDevice()
fails to create a camera.
However, we realized this doesn't actually fully solve the issue.
PipelineHandler::acquireMediaDevice() uses DeviceEnumerator::search(),
which is not stateful and skips devices if they are MediaDevice::busy().
MediaDevice::busy() uses MediaDevice::acquired_, which is set via
PipelineHandler::acquireMediaDevice() (MediaDevice::acquire()), and is
not unset (MediaDevice::release()) until pipeline handler
deconstruction.
Therefore, even if we remove the media device from the mediaDevices_
list, it's not actually released. But we can't just release it either as
DeviceEnumerator::search() will just pick it up again. So I think the
only solution is to rework DeviceEnumerator::search() to either be
stateful or return/take an index for resuming.
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
> > >
More information about the libcamera-devel
mailing list