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

[Andrei Konovalov]

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...

> 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.
> 
> 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.

Thanks,
Andrei

> 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?
> 
> Thanks,
> Milan
>