[PATCH 00/12] libcamera: Hardening against thread race conditions

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 02:52:04AM +0300, Andrei Konovalov wrote:
> On 22.01.2024 19:30, Milan Zamazal wrote:
> > Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > 
> >> Even if V4L2VideoDevice isn't addressed directly by this series, and the
> >> bug report is related to usage of the V4L2VideoDevice class, I believe
> >> the issue in the soft ISP code will be caught by the assertions added to
> >> the EventNotifier class. Milan, would you be able to test this, and
> >> confirm that libcamera now complains loudly ?
> > 
> > Nice cleanup, thank you!
> 
> +1, thanks Laurent!
> 
> > Yes, this complains immediately and loudly in the
> > given case.
> 
> I was also able to get the assertion on my much faster board.
> This required to use the highest resolution for capture, and it can take some
> time (tens of seconds) before the assertion is triggered.
> 
> > Now a related question is how to deal with threads and instances of classes that
> > don't inherit Object.
> > 
> > I think V4L2VideoDevice::queueBuffer is a good example.  V4L2VideoDevice is
> > thread-bound but doesn't inherit Object.  So the caller, unless it has created
> > the given instance itself (and being thread-bound itself or tracking its initial
> > thread), has no idea about the thread a V4L2VideoDevice instance is bound too,
> > even less how to call it there, and even less without worrying about
> > implications of asynchronous calls.
> 
> I can create a proxy Object (to call V4L2VideoDevice::queueBuffer from) in the pipeline
> handler thread and connect it to the Soft ISP's signal, but it doesn't look like a
> nice solution...

Agreed, it's not very nice :-)

> > This means ensuring most things run in the same thread.  This may be easy in
> > some cases and less fine in other ones.  It discourages using multiple threads,
> > not sure whether this is a good thing or not.  It's not robust in any case.
> > 
> > A possible alternative would be making less things thread-bound.  I suppose
> > there are reasons why this is not happening in libcamera.

Yes, complexity. It's error-prone enough as it is, making more functions
callable from different threads will mean more race conditions as all
the callers will need to handle the thread-safety requirements. As much
as possible of the code base is single-threaded in the sense that
developers don't need to think about threads, and thread boundaries are
handled with strict rules that need to be considered only when adding a
new thread, which should be a rare case.

> > Another alternative would be to use Object (as a direct base class or in
> > wrappers) or Thread (together with initial thread tracking) to call the right
> > thread in more places.  It would be limited to situations where possibly
> > asynchronous invocations don't harm if nothing else.
> 
> ...yes, something along these lines.

A good solution may be to model the soft ISP object like if it was a
real hardware device. Give it functions to perform actions, and signals
to report events. Internally, create a proxy to the thread to relay the
function calls with invokeMethod(). The signals should work out of the
box, if you emit them from the ISP thread, and connect the pipeline
handler slots to them, you should get queued signal behaviour
automatically as PipelineHandler inherits from Object.

I think this will also make it easier to implement a GPU-based ISP
later.

> > The call chain in the software ISP branch, DebayerCpu -> SwIspSimple ->
> > SimpleCameraData -> V4L2VideoDevice (thread-bound) -> EventNotifier (inheriting
> > Object), must be changed to eventually invoke V4L2VideoDevice::queueBuffer in
> > the right thread.  Is the recommended practice to examine thoroughly what's
> > running where or are there any simpler options?

The soft ISP should not deal with V4L2 at all. It should be modelled as
a processing component that takes incoming calls and generates events
when processing is complete, like a V4L2VideoDevice does. It's then the
job of the pipeline handler to connect the building blocks to create a
pipeline.

-- 
Regards,

Laurent Pinchart