[RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent dependency

[Laurent Pinchart]

On Fri, Jan 10, 2025 at 10:44:07AM +0000, Barnabás Pőcze wrote:
> 2025. január 10., péntek 11:28 keltezéssel, Barnabás Pőcze írta:
> > 2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart írta:
> > > On Tue, Jan 07, 2025 at 06:06:04PM +0100, Jacopo Mondi wrote:
> > > > On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:
> > > > > libevent is only used as a way to notify the main thread from
> > > > > the CameraManager thread when the capture should be terminated.
> > >
> > > True, but will that always be the case ? The current tests are fairly
> > > simple, but what will happen when we'll have more complex tests that
> > > will need to perform multiple actions through a capture session ? I'm
> > > sure we could still handle that manually with condition variables, but I
> > > think that will become complex, and it feels like reinventing the wheel.
> > 
> > The current libevent usage is not quite correct, so it needs to be fixed in any case.
> > This patch implements probably the simplest option that works for the current
> > requirements. Alternatively, I believe manually triggered events of libevent could
> > be used to achieve the same, but that would easily be more code and touch more
> > components (e.g. `event_loop.{cpp,h}`). But I'm not trying to suggest that libevent
> > should not be used anymore. It could be reintroduced in the future easily in my opinion.
> > So which one should it be?
> 
> It would appear that `EventLoop::callLater()` might be an option as well.

I suppose the issue isn't just that loop_ could be null when the request
completion handler calls loop_->exit(), but also the fact that exit()
could be called before exec(). Using callLater() as done in the cam
application should work I think, it the fix should then be simple and
not too intrusive. Unless I'm missing something :-) Could you try it out
?

> > > > > Not only is libevent not really necessary and instead can be
> > > > > replaced with a simple combination of C++ STL parts, the current
> > > > > usage is prone to race conditions.
> > > > >
> > > > > Specifically, the camera's `queueRequest()` might complete the
> > > > > request before the main thread could set the `loop_` member
> > > > > variable and synchronize with the thread. This is a data race,
> > > > > practically resulting in a nullptr or dangling pointer dereference.
> > > > >
> > > > > This can easily be triggered by inserting a sufficiently large
> > > > > timeout before `loop_ = new EventLoop;`:
> > > >
> > > > To be frank this is kind of hard to trigger, as it can only happens
> > > > if the main thread does not execute for longer than the frame
> > > > interval. But yes, who knows, if there is a chance for this to happen,
> > > > it will happen sooner or later, so it's indeed worth addressing it.
> > > >
> > > > >   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)
> > > > >   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'
> > > > >     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140
> > > > >
> > > > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > > > ---
> > > > > [...]

-- 
Regards,

Laurent Pinchart