<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Sep 1, 2024 at 8:47 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Aug 26, 2024 at 02:13:22PM +0200, Cheng-Hao Yang wrote:<br>
> On Sun, Aug 25, 2024 at 3:15 AM Laurent Pinchart wrote:<br>
> > On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:<br>
> > > From: Han-Lin Chen <<a href="mailto:hanlinchen@chromium.org" target="_blank">hanlinchen@chromium.org</a>><br>
> > ><br>
> > > Add enum RequestCompletionMode to Camera with two values:<br>
> > > InSubmissionOrder andImmediately. The purpose is to allow the<br>
> > > application to configure the order of signaling requestCompleted.<br>
> > > The InSubmissionOrder mode is the default mode, which signals according<br>
> > > to the request submission order.<br>
> > > The Immediately mode allows the pipeline handler to signal as soon as a<br>
> > > request is completed. Applications need to reconstruct the order by<br>
> > > themselves.<br>
> > ><br>
> > > In the real use cases, it allows a camera app to continue the preview<br>
> > > stream flowing, while certain ISPs/algorithms are still processing a<br>
> > > complex flow like a still capture request, instead of being blocked<br>
> > > by the still capture request.<br>
> ><br>
> > Given how this changes the request mechanism in a fundamental way, you<br>
> > will need to be way more convincing than this.<br>
> ><br>
> > Based on the cover letter, this is meant to implement partial metadata<br>
> > support in the Android camera HAL. If that's the only use case, I think<br>
> > a better solution is to add partial metadata support to the libcamera<br>
> > native API. This being said, I don't see how this change can provide<br>
> > partial metadata support, as metadata is still reported in one go. I<br>
> > assume you want to report metadata to the camera service ahead of<br>
> > request completion time, using the partial metadata API of the Android<br>
> > camera HAL, but still in one go. If that's right, that should be quite<br>
> > simple to implement in the PipelineHandler class by adding a signal to<br>
> > report metadata once a request is marked as complete.<br>
><br>
> Although the purpose of this patch was not to implement partial metadata<br>
> support in the Android adapter, it's true that we don't really need it:<br>
> Buffers and metadata can be returned earlier to the application with<br>
> the partial result support, and Android adapter actually needs to ensure<br>
> that the requests are returned in the submission order as well [1].<br>
> <br>
> And yes, we'll implement the mechanism to report the last metadata<br>
> as a partial result ahead of the requestCompleted signal.<br>
> <br>
> Let's drop this patch.<br>
> <br>
> [1]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808" rel="noreferrer" target="_blank">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808</a><br>
> <br>
> One more question before I submit partial result support patches:<br>
> In mtkisp7 branch, we added `partialResultCompleted` [2] signal<br>
> in libcamera::Camera, while it actually duplicates the signal with<br>
> `bufferCompleted`. In the use cases though, we only call<br>
> `PipelineHandler::completeMetadata` &<br>
> `PipelineHandler::completeBuffer`, without calling<br>
> `PipelineHandler::completePartialResult` directly in the pipeline<br>
> handler mtkisp7's implementation. That being said, we don't need<br>
> to support having multiple buffers and/or metadata within the same<br>
> partial result in the libcamera core libraries.<br>
> <br>
> Do you think we should keep `bufferCompleted` signal and add<br>
> a `metadataCompleted` signal in libcamera::Camera, or remove<br>
> `bufferCompleted` signal and add a `partialResultCompleted`<br>
> signal?<br>
<br>
I think added a metadataCompleted signal is the simplest and least<br>
intrusive change for now (please voice your opinion if you disagree, I<br>
may be missing something), so it would be my preference.<br></blockquote><div><br></div><div>Thanks for the reply. I think it makes sense, as both mtkisp7 and</div><div>intelipu7 doesn't seem to need a partial result that contains both</div><div>buffers and metadata.</div><div><br></div><div>I'll modify the patches and upload them accordingly.</div><div><br></div><div>BR,<br>Harvey</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> [2]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h</a><br>
> [3]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h</a><br>
> <br>
> > > Signed-off-by: Han-Lin Chen <<a href="mailto:hanlinchen@chromium.org" target="_blank">hanlinchen@chromium.org</a>><br>
> > > Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > > ---<br>
> > >  Documentation/guides/pipeline-handler.rst |  3 +-<br>
> > >  include/libcamera/camera.h                |  8 ++++<br>
> > >  include/libcamera/internal/camera.h       |  1 +<br>
> > >  src/libcamera/camera.cpp                  | 50 +++++++++++++++++++++--<br>
> > >  src/libcamera/pipeline_handler.cpp        |  7 ++++<br>
> > >  5 files changed, 65 insertions(+), 4 deletions(-)<br>
<br>
[snip]<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>