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

[Jacopo Mondi]

Hi Harvey

On Fri, Nov 22, 2024 at 03:02:25PM +0800, Cheng-Hao Yang wrote:
> 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
>

Yes, it stops the libcamera::Camera but from the Android's point of
view the camera configuration is not reset, it is possible to start
sending capture requests without going through a configure_streams
call.

The camera HAL will re-start the camera in processCaptureRequest() if
it has been stopped.
https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1114

Please note that if a libcamera::Camera is stopped, the configuration
is not reset, according to the Camera state-machine.

TL;DR from the Android point of view the camera configuration is not
reset and capture requests can be submitted immediately after a flush()

Does this patch steam from an error detected by CTS or a runtime error
you have noticed ?

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

I presume it will have to be a synchronous operation, that returns
all in-flights requests without going through a libcamera::Camera::stop()

However I don't think there are designs document that can be
referenced 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.
>
> 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