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

[Laurent Pinchart]

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.

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