Discussion of libcamera::Camera::stop() and flush

[Jacopo Mondi]

Hi Harvey

On Tue, Nov 19, 2024 at 01:42:30PM +0800, Cheng-Hao Yang wrote:
> Hi upstream developers,
>
> During the review of the pending mtkisp7 patches to be upstreamed, I
> found one [1] that I don't think you'd like.
>
> I don't think Android adapter's flush() is correctly implemented now,
> as it requires all requests & buffers to be returned from the HAL [2].
>
> In the current implementation [3] though, it only stops processing new
> requests and stops the camera, which basically stops all processing
> and does nothing else.
>

mmm, this shouldn't happen. In facts Camera::stop() should

 * This function stops capturing and processing requests immediately. All
 * pending requests are cancelled and complete synchronously in an error state.

so it is expected that calling

        camera_->stop()

completes all pending Requests and Buffers

> IIUC, in V4L2, stopping wouldn't trigger any returning signal
> emission. However, Android Camera HAL v3 (function flush()) still
> expects requests and buffers being returned with success/failure
> signals.
>

There are several aspects at play here

1) Calling VIDIOC_STREAMOFF on a capture device should return all
queued buffers in error state.
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html

You should check if this is correctly implemented in the mtkisp7
kernel driver, which apparently happens here ?
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1/drivers/media/platform/mediatek/isp/isp_7x/imgsys/mtk_imgsys-v4l2.c#444

2) The V4L2VideoDevice class completes all the queued FrameBuffers
with an error condition in its streamOff() implementation. The
pipeline handler should complete a Request when all buffers part of it
are completed, even if in error state

3) The pipeline handler should make sure all pending requests have
been completed before returning from PipelineHandler::stopDevice()

In example the RkISP1 pipeline handler does

	ASSERT(data->queuedRequests_.empty());

You could try adding the same to your pipeline to verify if the
V4L2/kernel device behaves correctly.

> I also found Umang's patch [4] that adds a flushing mechanism in post
> processing, while it's never enabled. Without using it, the current
> mechanism is problematic that a flush call would drop buffers that are
> still being post-processed.

I'm puzzled, the commit message says:

This patch also implements a flush() for the PostProcessorWorker
class which is responsible to purge post-processing requests
queued up while a camera is stopping/flushing. It is hooked with
CameraStream::flush(), which isn't used currently but will be
used when we handle flush/stop scenarios in greater detail
subsequently (in a different patchset).

however I don't see it being implemented ?

Also please note that, if I got this right, this change is only about
buffers being post-processed in the android HAL (for example to
duplicate YUV streams using libyuv or for SW JPEG encoding) and
doesn't impact the libcamera API.

>
> In mtkisp7, we chose to wait for the completion of requests/buffers
> before the return of the flush() function [1]. Also, the current
> design of libcamera::Camera::stop() doesn't quite work with our Task &
> Scheduler [5], so we have WIP CLs [6] to add
> libcamera::Camera::flush() that hints a pipeline handler to abort
> requests before calling stop().

I think there's general agreement we want Camera::flush()
implemented eventually.

However I'm not sure how if we need support in V4L2 or not for that,
as the idea would be to return all queued buffers in error state
without calling VIDIOC_STREAMOFF on the capture device, something I'm
not sure we can do at the moment.

Ideally, we should queue buffers to the video devices as late as
possible (while in most pipeline handler we currently queue them as
soon as queueRequestDevice is called) minimizing the number of
pre-queued buffers which can't be returned without stopping the
capture device. However I'm not sure how hard that can be enforced as
both the pipeline handler and the kernel driver should be carefully
implemented to support this.

I think [6] might be a useful starting point to clarify this if you
want to send it to the list.

Thanks
   j

>
> Please check and share your thoughts about supporting Android
> adapter's flushing mechanism. Thanks!
>
> BR,
> Harvey
>
> [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674665
> [2]: https://android.googlesource.com/platform/hardware/interfaces/+/main/camera/device/3.2/ICameraDeviceSession.hal#283
> [3]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n418
> [4]: https://git.libcamera.org/libcamera/libcamera.git/commit/src/android/camera_device.cpp?id=b1cefe38f360915ad597ab2934c009bd1e46d10d
> [5]: https://patchwork.libcamera.org/patch/21029/
> [6]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5553372/8