[PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to configure the completion order

[Cheng-Hao Yang]

On Sun, Sep 1, 2024 at 8:47 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> On Mon, Aug 26, 2024 at 02:13:22PM +0200, Cheng-Hao Yang wrote:
> > On Sun, Aug 25, 2024 at 3:15 AM Laurent Pinchart wrote:
> > > On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:
> > > > From: Han-Lin Chen <hanlinchen at chromium.org>
> > > >
> > > > Add enum RequestCompletionMode to Camera with two values:
> > > > InSubmissionOrder andImmediately. The purpose is to allow the
> > > > application to configure the order of signaling requestCompleted.
> > > > The InSubmissionOrder mode is the default mode, which signals
> according
> > > > to the request submission order.
> > > > The Immediately mode allows the pipeline handler to signal as soon
> as a
> > > > request is completed. Applications need to reconstruct the order by
> > > > themselves.
> > > >
> > > > In the real use cases, it allows a camera app to continue the preview
> > > > stream flowing, while certain ISPs/algorithms are still processing a
> > > > complex flow like a still capture request, instead of being blocked
> > > > by the still capture request.
> > >
> > > Given how this changes the request mechanism in a fundamental way, you
> > > will need to be way more convincing than this.
> > >
> > > Based on the cover letter, this is meant to implement partial metadata
> > > support in the Android camera HAL. If that's the only use case, I think
> > > a better solution is to add partial metadata support to the libcamera
> > > native API. This being said, I don't see how this change can provide
> > > partial metadata support, as metadata is still reported in one go. I
> > > assume you want to report metadata to the camera service ahead of
> > > request completion time, using the partial metadata API of the Android
> > > camera HAL, but still in one go. If that's right, that should be quite
> > > simple to implement in the PipelineHandler class by adding a signal to
> > > report metadata once a request is marked as complete.
> >
> > Although the purpose of this patch was not to implement partial metadata
> > support in the Android adapter, it's true that we don't really need it:
> > Buffers and metadata can be returned earlier to the application with
> > the partial result support, and Android adapter actually needs to ensure
> > that the requests are returned in the submission order as well [1].
> >
> > And yes, we'll implement the mechanism to report the last metadata
> > as a partial result ahead of the requestCompleted signal.
> >
> > Let's drop this patch.
> >
> > [1]:
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808
> >
> > One more question before I submit partial result support patches:
> > In mtkisp7 branch, we added `partialResultCompleted` [2] signal
> > in libcamera::Camera, while it actually duplicates the signal with
> > `bufferCompleted`. In the use cases though, we only call
> > `PipelineHandler::completeMetadata` &
> > `PipelineHandler::completeBuffer`, without calling
> > `PipelineHandler::completePartialResult` directly in the pipeline
> > handler mtkisp7's implementation. That being said, we don't need
> > to support having multiple buffers and/or metadata within the same
> > partial result in the libcamera core libraries.
> >
> > Do you think we should keep `bufferCompleted` signal and add
> > a `metadataCompleted` signal in libcamera::Camera, or remove
> > `bufferCompleted` signal and add a `partialResultCompleted`
> > signal?
>
> I think added a metadataCompleted signal is the simplest and least
> intrusive change for now (please voice your opinion if you disagree, I
> may be missing something), so it would be my preference.
>

Thanks for the reply. I think it makes sense, as both mtkisp7 and
intelipu7 doesn't seem to need a partial result that contains both
buffers and metadata.

I'll modify the patches and upload them accordingly.

BR,
Harvey


>
> > [2]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h
> > [3]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h
> >
> > > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > ---
> > > >  Documentation/guides/pipeline-handler.rst |  3 +-
> > > >  include/libcamera/camera.h                |  8 ++++
> > > >  include/libcamera/internal/camera.h       |  1 +
> > > >  src/libcamera/camera.cpp                  | 50
> +++++++++++++++++++++--
> > > >  src/libcamera/pipeline_handler.cpp        |  7 ++++
> > > >  5 files changed, 65 insertions(+), 4 deletions(-)
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240902/8770a75d/attachment.htm>