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

[Cheng-Hao Yang]

Hi Jacopo,


On Tue, Nov 19, 2024 at 10:20 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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

Right, sorry, I didn't notice that V4L2VideoDevice::streamOff() triggers
bufferReady signal, and therefore the pipeline handler implementations
should complete buffers and requests accordingly.



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

Sorry, the previous link [4] limits the diff in camera_device.cpp.
Please check this one:
https://git.libcamera.org/libcamera/libcamera.git/commit/?id=b1cefe38f360915ad597ab2934c009bd1e46d10d


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

Correct. That was one of my concerns that the android HAL's flush
is not being implemented correctly now.
The current implementation [7] stops the camera, while I believe
Android's flush doesn't mean to do that, yet.

[7]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n428

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

That's good to know. What would be the expected behavior though?
Is it expected to only signal flushing, or is it going to be blocked until
all requests/buffers completed?

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

True, it's an interesting question. A random idea is that a pipeline handler
implementation can digest from `waitingRequests_` based on the pipeline
depth, or add a signal to trigger the processing in the base class.

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

If there's some discussion / agreement that I can take a look first,
I can polish my patch before submitting :)

BR,
Harvey

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