| Message ID | 20240912045703.3446748-2-chenghaoyang@google.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Gentle ping: May we review this patch recently, so that I can continue uploading the corresponding patches in Android Adapter? Thanks :) BR, Harvey On Thu, Sep 12, 2024 at 12:57 PM Harvey Yang <chenghaoyang@chromium.org> wrote: > > From: Han-Lin Chen <hanlinchen@chromium.org> > > Allows pipeline handler to signal metadata completion by adding the > following signals to Camera: > > Signal<Request *, const ControlList &> metadataCompleted; > > Together with the bufferCompleted signal, the pipeline handler is allowed to > return buffers and partial metadata at any stage of processing. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > --- > include/libcamera/camera.h | 1 + > include/libcamera/internal/pipeline_handler.h | 1 + > include/libcamera/request.h | 5 +++ > src/libcamera/camera.cpp | 6 +++ > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > src/libcamera/request.cpp | 20 ++++++++++ > 6 files changed, 70 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 94cee7bd..08c5c58c 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -122,6 +122,7 @@ public: > > const std::string &id() const; > > + Signal<Request *, const ControlList &> metadataCompleted; > Signal<Request *, FrameBuffer *> bufferCompleted; > Signal<Request *> requestCompleted; > Signal<> disconnected; > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 0d380803..1af63a23 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -58,6 +58,7 @@ public: > void registerRequest(Request *request); > void queueRequest(Request *request); > > + void completeMetadata(Request *request, const ControlList &metadata); > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index e214a9d1..0200f4a1 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -12,6 +12,7 @@ > #include <ostream> > #include <stdint.h> > #include <string> > +#include <unordered_set> > > #include <libcamera/base/class.h> > #include <libcamera/base/signal.h> > @@ -64,6 +65,8 @@ public: > > std::string toString() const; > > + ControlList addCompletedMetadata(const ControlList &metadata); > + > private: > LIBCAMERA_DISABLE_COPY(Request) > > @@ -73,6 +76,8 @@ private: > > const uint64_t cookie_; > Status status_; > + > + std::unordered_set<unsigned int> completedMetadata_; > }; > > std::ostream &operator<<(std::ostream &out, const Request &r); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index a86f552a..5ffae23e 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > * completed > */ > > +/** > + * \var Camera::metadataCompleted > + * \brief Signal emitted when some metadata for a request is completed as a > + * partial result > + */ > + > /** > * \var Camera::requestCompleted > * \brief Signal emitted when a request queued to the camera has completed > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index e5940469..5d2999cb 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > return request->_d()->completeBuffer(buffer); > } > > +/** > + * \brief Complete part of metadata for a request > + * \param[in] request The request the buffer belongs to > + * \param[in] metadata The partial metadata that has completed > + * > + * This function could be called by pipeline handlers to signal completion of > + * the \a metadata part of the \a request. It notifies applications of metadata > + * completion. > + * > + * \context This function shall be called from the CameraManager thread. > + */ > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > +{ > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); > + if (!validMetadata.empty()) { > + request->metadata().merge(validMetadata); > + > + Camera *camera = request->_d()->camera(); > + camera->metadataCompleted.emit(request, validMetadata); > + } > +} > + > /** > * \brief Signal request completion > * \param[in] request The request that has completed > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > Camera::Private *data = camera->_d(); > > + /* > + * Collect metadata which is not yet completed by the Camera, and > + * create one partial result to cover the missing metadata before > + * completing the whole request. This guarantees the aggregation of > + * metadata in completed partial results equals to the global metadata > + * in the request. > + * > + * \todo: Forbid merging metadata into request.metadata() directly and > + * force calling completeMetadata() to report metadata. > + */ > + const ControlList &validMetadata = request->addCompletedMetadata( > + request->metadata()); > + if (!validMetadata.empty()) > + camera->metadataCompleted.emit(request, validMetadata); > + > while (!data->queuedRequests_.empty()) { > Request *req = data->queuedRequests_.front(); > if (req->status() == Request::RequestPending) > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 8c56ed30..3ce23a8e 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -598,6 +598,26 @@ std::string Request::toString() const > return ss.str(); > } > > +/** > + * \brief Add completed metadata, as a partial result > + * \param[in] metadata The metadata completed > + * > + * Request will record the entries that has been sent to the application, to > + * prevent duplicated controls. > + * > + * \return ControlList that hasn't been completed before > + */ > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > +{ > + ControlList resultMetadata; > + for (auto &[id, value] : metadata) { > + if (!completedMetadata_.count(id)) > + resultMetadata.set(id, value); > + } > + > + return resultMetadata; > +} > + > /** > * \brief Insert a text representation of a Request into an output stream > * \param[in] out The output stream > -- > 2.46.0.598.g6f2099f65c-goog >
Hi Harvey, sorry for the late feedback On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote: > From: Han-Lin Chen <hanlinchen@chromium.org> > > Allows pipeline handler to signal metadata completion by adding the > following signals to Camera: > > Signal<Request *, const ControlList &> metadataCompleted; Bikeshedding on the name a bit, as this is allowed to be called multiple times, with partial metadata, I would call it 'metadataAvailable'. > > Together with the bufferCompleted signal, the pipeline handler is allowed to > return buffers and partial metadata at any stage of processing. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > --- > include/libcamera/camera.h | 1 + > include/libcamera/internal/pipeline_handler.h | 1 + > include/libcamera/request.h | 5 +++ > src/libcamera/camera.cpp | 6 +++ > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > src/libcamera/request.cpp | 20 ++++++++++ > 6 files changed, 70 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 94cee7bd..08c5c58c 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -122,6 +122,7 @@ public: > > const std::string &id() const; > > + Signal<Request *, const ControlList &> metadataCompleted; > Signal<Request *, FrameBuffer *> bufferCompleted; > Signal<Request *> requestCompleted; > Signal<> disconnected; > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 0d380803..1af63a23 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -58,6 +58,7 @@ public: > void registerRequest(Request *request); > void queueRequest(Request *request); > > + void completeMetadata(Request *request, const ControlList &metadata); > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index e214a9d1..0200f4a1 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -12,6 +12,7 @@ > #include <ostream> > #include <stdint.h> > #include <string> > +#include <unordered_set> > > #include <libcamera/base/class.h> > #include <libcamera/base/signal.h> > @@ -64,6 +65,8 @@ public: > > std::string toString() const; > > + ControlList addCompletedMetadata(const ControlList &metadata); > + > private: > LIBCAMERA_DISABLE_COPY(Request) > > @@ -73,6 +76,8 @@ private: > > const uint64_t cookie_; > Status status_; > + > + std::unordered_set<unsigned int> completedMetadata_; > }; > > std::ostream &operator<<(std::ostream &out, const Request &r); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index a86f552a..5ffae23e 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > * completed > */ > > +/** > + * \var Camera::metadataCompleted > + * \brief Signal emitted when some metadata for a request is completed as a > + * partial result Currently, we return metadata as part of a Request signalled with Camera::requestCompleted. Existing applications do not handle the new signal and I don't think we should make it mandatory to do so. According to the implementation of this patch, the same application running on a pipeline handler that calls PipelineHandler::completeMetadata and on a pipeline handler that doesn't will receive different sets of metadata. - If a pipeline handler calls PipelineHandler::completeMetadata() it will send metadata in chunks and only the ones not previously signalled with be part of Request::metadata(). - If a pipeline handler doesn't call PipelineHandler::completeMetadata() the full metadata buffer will be in Request::metadata(). How does an application know what to expect ? Imo we should advertise if the camera supports partial metadata or not (maybe through a property). If it does applications should opt-in to receive partial metadata. This could even be done per-configuration by adding a field to CameraConfiguration ? If applications enable early metdata completion they agree to handle the new signal, and the behaviour will be the one implemented in this patch. If they do not enable early metadata completion then PipelineHandler::completeMetadata() should simply merge the partial metadata in Request::metadata() and not emit the new signal but return the full metadata buffer as part of the Request in requestCompleted, as they currently do so that pipelines that use early completion and pipelines that do not will behave identically from an application perspective. What do you think ? > + */ > + > /** > * \var Camera::requestCompleted > * \brief Signal emitted when a request queued to the camera has completed > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index e5940469..5d2999cb 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > return request->_d()->completeBuffer(buffer); > } > > +/** > + * \brief Complete part of metadata for a request > + * \param[in] request The request the buffer belongs to * \param[in] request The Request the metadata buffer belongs to > + * \param[in] metadata The partial metadata that has completed > + * > + * This function could be called by pipeline handlers to signal completion of > + * the \a metadata part of the \a request. It notifies applications of metadata > + * completion. I think a little more details are needed. * This function could be called by pipeline handlers to signal * availability of \a metadata before \a request completes. Early * metadata completion allows to notify applications about the * availability of a partial metadata buffer before the associated * Request has completed. 1) According to this implementation metadata can be completed out-of-order (partial metadata for Request X+1 can be signalled before Request X completes). Is this desirable ? > + * > + * \context This function shall be called from the CameraManager thread. > + */ > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > +{ > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); Request::addCompleteMetadata constructs and returns a ControlList, I'm surprised the compiler doesn't complain you're taking a reference to a temporary object (it's apparently legal for const references only). However, as a temporary will be constructed anyway this doesn't buy you any gain, possibily the contrary, as this will disallow C++ copy elision from happening (afaiu). I think you should drop & > + if (!validMetadata.empty()) { > + request->metadata().merge(validMetadata); > + > + Camera *camera = request->_d()->camera(); > + camera->metadataCompleted.emit(request, validMetadata); > + } > +} > + > /** > * \brief Signal request completion > * \param[in] request The request that has completed > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > Camera::Private *data = camera->_d(); > > + /* > + * Collect metadata which is not yet completed by the Camera, and > + * create one partial result to cover the missing metadata before > + * completing the whole request. This guarantees the aggregation of > + * metadata in completed partial results equals to the global metadata > + * in the request. > + * > + * \todo: Forbid merging metadata into request.metadata() directly and > + * force calling completeMetadata() to report metadata. > + */ > + const ControlList &validMetadata = request->addCompletedMetadata( > + request->metadata()); > + if (!validMetadata.empty()) > + camera->metadataCompleted.emit(request, validMetadata); > + > while (!data->queuedRequests_.empty()) { > Request *req = data->queuedRequests_.front(); > if (req->status() == Request::RequestPending) > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 8c56ed30..3ce23a8e 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -598,6 +598,26 @@ std::string Request::toString() const > return ss.str(); > } > > +/** > + * \brief Add completed metadata, as a partial result > + * \param[in] metadata The metadata completed > + * > + * Request will record the entries that has been sent to the application, to > + * prevent duplicated controls. > + * > + * \return ControlList that hasn't been completed before > + */ > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > +{ > + ControlList resultMetadata; > + for (auto &[id, value] : metadata) { > + if (!completedMetadata_.count(id)) completedMetadata_ should be cleared in Request::reset(). I wonder if we should actually prevent the same metadata to be returned multiple times by the pipeline handler or not. > + resultMetadata.set(id, value); > + } > + > + return resultMetadata; > +} > + > /** > * \brief Insert a text representation of a Request into an output stream > * \param[in] out The output stream > -- > 2.46.0.598.g6f2099f65c-goog >
Hi Jacopo, I'll upload a new version when all discussion is done. On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey, > sorry for the late feedback > > On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote: > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > Allows pipeline handler to signal metadata completion by adding the > > following signals to Camera: > > > > Signal<Request *, const ControlList &> metadataCompleted; > > Bikeshedding on the name a bit, as this is allowed to be called > multiple times, with partial metadata, I would call it > 'metadataAvailable'. Sure, 'metadataAvailable' does make more sense. Thanks! > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to > > return buffers and partial metadata at any stage of processing. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > include/libcamera/camera.h | 1 + > > include/libcamera/internal/pipeline_handler.h | 1 + > > include/libcamera/request.h | 5 +++ > > src/libcamera/camera.cpp | 6 +++ > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > > src/libcamera/request.cpp | 20 ++++++++++ > > 6 files changed, 70 insertions(+) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 94cee7bd..08c5c58c 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -122,6 +122,7 @@ public: > > > > const std::string &id() const; > > > > + Signal<Request *, const ControlList &> metadataCompleted; > > Signal<Request *, FrameBuffer *> bufferCompleted; > > Signal<Request *> requestCompleted; > > Signal<> disconnected; > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index 0d380803..1af63a23 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -58,6 +58,7 @@ public: > > void registerRequest(Request *request); > > void queueRequest(Request *request); > > > > + void completeMetadata(Request *request, const ControlList &metadata); > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > void completeRequest(Request *request); > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index e214a9d1..0200f4a1 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -12,6 +12,7 @@ > > #include <ostream> > > #include <stdint.h> > > #include <string> > > +#include <unordered_set> > > > > #include <libcamera/base/class.h> > > #include <libcamera/base/signal.h> > > @@ -64,6 +65,8 @@ public: > > > > std::string toString() const; > > > > + ControlList addCompletedMetadata(const ControlList &metadata); > > + > > private: > > LIBCAMERA_DISABLE_COPY(Request) > > > > @@ -73,6 +76,8 @@ private: > > > > const uint64_t cookie_; > > Status status_; > > + > > + std::unordered_set<unsigned int> completedMetadata_; > > }; > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index a86f552a..5ffae23e 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > > * completed > > */ > > > > +/** > > + * \var Camera::metadataCompleted > > + * \brief Signal emitted when some metadata for a request is completed as a > > + * partial result > > Currently, we return metadata as part of a Request signalled with > Camera::requestCompleted. Existing applications do not handle the new > signal and I don't think we should make it mandatory to do so. > According to the implementation of this patch, the same application > running on a pipeline handler that calls > PipelineHandler::completeMetadata and on a pipeline handler that > doesn't will receive different sets of metadata. > > - If a pipeline handler calls PipelineHandler::completeMetadata() it > will send metadata in chunks and only the ones not previously > signalled with be part of Request::metadata(). Actually, it's not true. In this patch, `PipelineHandler::completeMetadata` will merge the partially available metadata into Request: `request->metadata().merge(validMetadata);` Therefore, whether an application handles the new signal, it can still get access to all the metadata in a request via `Request.metadata()`. This should ease your concern of the old and new implementations of applications. > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata() > the full metadata buffer will be in Request::metadata(). > > How does an application know what to expect ? > > Imo we should advertise if the camera supports partial metadata or not > (maybe through a property). If it does applications should opt-in to > receive partial metadata. This could even be done per-configuration by > adding a field to CameraConfiguration ? > > If applications enable early metdata completion they agree to handle > the new signal, and the behaviour will be the one implemented in this > patch. If they do not enable early metadata completion then > PipelineHandler::completeMetadata() should simply merge the partial > metadata in Request::metadata() and not emit the new signal but return > the full metadata buffer as part of the Request in requestCompleted, > as they currently do so that pipelines that use early completion and > pipelines that do not will behave identically from an application > perspective. > > What do you think ? > > > + */ > > + > > /** > > * \var Camera::requestCompleted > > * \brief Signal emitted when a request queued to the camera has completed > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index e5940469..5d2999cb 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > > return request->_d()->completeBuffer(buffer); > > } > > > > +/** > > + * \brief Complete part of metadata for a request > > + * \param[in] request The request the buffer belongs to > > * \param[in] request The Request the metadata buffer belongs to Perhaps: `* \param[in] request The Request the metadata belongs to` ? > > > > + * \param[in] metadata The partial metadata that has completed > > + * > > + * This function could be called by pipeline handlers to signal completion of > > + * the \a metadata part of the \a request. It notifies applications of metadata > > + * completion. > > I think a little more details are needed. > > * This function could be called by pipeline handlers to signal > * availability of \a metadata before \a request completes. Early > * metadata completion allows to notify applications about the > * availability of a partial metadata buffer before the associated > * Request has completed. Thanks! Updated. > > 1) According to this implementation metadata can be completed > out-of-order (partial metadata for Request X+1 can be signalled before > Request X completes). Is this desirable ? Yes, this is what we expect, and Android also allows that. WDYT? > > > + * > > + * \context This function shall be called from the CameraManager thread. > > + */ > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > > +{ > > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); > > Request::addCompleteMetadata constructs and returns a ControlList, > I'm surprised the compiler doesn't complain you're taking a reference > to a temporary object (it's apparently legal for const references only). > > However, as a temporary will be constructed anyway this doesn't > buy you any gain, possibily the contrary, as this will disallow C++ > copy elision from happening (afaiu). I think you should drop & Right, it's definitely a mistake. Removed. > > > > + if (!validMetadata.empty()) { > > + request->metadata().merge(validMetadata); > > + > > + Camera *camera = request->_d()->camera(); > > + camera->metadataCompleted.emit(request, validMetadata); > > + } > > +} > > + > > /** > > * \brief Signal request completion > > * \param[in] request The request that has completed > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > > > Camera::Private *data = camera->_d(); > > > > + /* > > + * Collect metadata which is not yet completed by the Camera, and > > + * create one partial result to cover the missing metadata before > > + * completing the whole request. This guarantees the aggregation of > > + * metadata in completed partial results equals to the global metadata > > + * in the request. > > + * > > + * \todo: Forbid merging metadata into request.metadata() directly and > > + * force calling completeMetadata() to report metadata. > > + */ > > + const ControlList &validMetadata = request->addCompletedMetadata( > > + request->metadata()); > > + if (!validMetadata.empty()) > > + camera->metadataCompleted.emit(request, validMetadata); > > + > > while (!data->queuedRequests_.empty()) { > > Request *req = data->queuedRequests_.front(); > > if (req->status() == Request::RequestPending) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index 8c56ed30..3ce23a8e 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -598,6 +598,26 @@ std::string Request::toString() const > > return ss.str(); > > } > > > > +/** > > + * \brief Add completed metadata, as a partial result > > + * \param[in] metadata The metadata completed > > + * > > + * Request will record the entries that has been sent to the application, to > > + * prevent duplicated controls. > > + * > > + * \return ControlList that hasn't been completed before > > + */ > > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > > +{ > > + ControlList resultMetadata; > > + for (auto &[id, value] : metadata) { > > + if (!completedMetadata_.count(id)) > > completedMetadata_ should be cleared in Request::reset(). Nice catch! Then how about we move the member variable and the member function to `Request::Private` instead? > > I wonder if we should actually prevent the same metadata to be > returned multiple times by the pipeline handler or not. This is actually prohibited by Android camera hal [1]: `Partial metadata submitted should not include any metadata key returned in a previous partial result for a given frame.` We can certainly prevent that in Android Adapter only, while I think having this restriction in libcamera core library also makes sense. WDYT? I can also update the description of the new signal. [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL. BR, Harvey > > > + resultMetadata.set(id, value); > > + } > > + > > + return resultMetadata; > > +} > > + > > /** > > * \brief Insert a text representation of a Request into an output stream > > * \param[in] out The output stream > > -- > > 2.46.0.598.g6f2099f65c-goog > >
Hi Harvey On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo, > > I'll upload a new version when all discussion is done. > > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Harvey, > > sorry for the late feedback > > > > On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote: > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > Allows pipeline handler to signal metadata completion by adding the > > > following signals to Camera: > > > > > > Signal<Request *, const ControlList &> metadataCompleted; > > > > Bikeshedding on the name a bit, as this is allowed to be called > > multiple times, with partial metadata, I would call it > > 'metadataAvailable'. > > Sure, 'metadataAvailable' does make more sense. > Thanks! > > > > > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to > > > return buffers and partial metadata at any stage of processing. > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > include/libcamera/camera.h | 1 + > > > include/libcamera/internal/pipeline_handler.h | 1 + > > > include/libcamera/request.h | 5 +++ > > > src/libcamera/camera.cpp | 6 +++ > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > > > src/libcamera/request.cpp | 20 ++++++++++ > > > 6 files changed, 70 insertions(+) > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > index 94cee7bd..08c5c58c 100644 > > > --- a/include/libcamera/camera.h > > > +++ b/include/libcamera/camera.h > > > @@ -122,6 +122,7 @@ public: > > > > > > const std::string &id() const; > > > > > > + Signal<Request *, const ControlList &> metadataCompleted; > > > Signal<Request *, FrameBuffer *> bufferCompleted; > > > Signal<Request *> requestCompleted; > > > Signal<> disconnected; > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > index 0d380803..1af63a23 100644 > > > --- a/include/libcamera/internal/pipeline_handler.h > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > @@ -58,6 +58,7 @@ public: > > > void registerRequest(Request *request); > > > void queueRequest(Request *request); > > > > > > + void completeMetadata(Request *request, const ControlList &metadata); > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > > void completeRequest(Request *request); > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > index e214a9d1..0200f4a1 100644 > > > --- a/include/libcamera/request.h > > > +++ b/include/libcamera/request.h > > > @@ -12,6 +12,7 @@ > > > #include <ostream> > > > #include <stdint.h> > > > #include <string> > > > +#include <unordered_set> > > > > > > #include <libcamera/base/class.h> > > > #include <libcamera/base/signal.h> > > > @@ -64,6 +65,8 @@ public: > > > > > > std::string toString() const; > > > > > > + ControlList addCompletedMetadata(const ControlList &metadata); > > > + > > > private: > > > LIBCAMERA_DISABLE_COPY(Request) > > > > > > @@ -73,6 +76,8 @@ private: > > > > > > const uint64_t cookie_; > > > Status status_; > > > + > > > + std::unordered_set<unsigned int> completedMetadata_; > > > }; > > > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index a86f552a..5ffae23e 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > > > * completed > > > */ > > > > > > +/** > > > + * \var Camera::metadataCompleted > > > + * \brief Signal emitted when some metadata for a request is completed as a > > > + * partial result > > > > Currently, we return metadata as part of a Request signalled with > > Camera::requestCompleted. Existing applications do not handle the new > > signal and I don't think we should make it mandatory to do so. > > According to the implementation of this patch, the same application > > running on a pipeline handler that calls > > PipelineHandler::completeMetadata and on a pipeline handler that > > doesn't will receive different sets of metadata. > > > > - If a pipeline handler calls PipelineHandler::completeMetadata() it > > will send metadata in chunks and only the ones not previously > > signalled with be part of Request::metadata(). > > Actually, it's not true. > In this patch, `PipelineHandler::completeMetadata` will merge the > partially available metadata into Request: > `request->metadata().merge(validMetadata);` > Ups, sorry I missed it! > Therefore, whether an application handles the new signal, it can > still get access to all the metadata in a request via > `Request.metadata()`. This should ease your concern of the old > and new implementations of applications. > Yes, if this time I got this right, the full metadata pack will be available in Request::metadata() at requestCompleted time, but it will be completed in chunks if the pipeline handler calls completeMetadata() earlier. There's only one thing left that I'm not sure about: in requestCompleted() const ControlList &validMetadata = request->addCompletedMetadata( request->metadata()); if (!validMetadata.empty()) camera->metadataCompleted.emit(request, validMetadata); If the pipeline handler never calls completeMetadata() then validMetadata == request->metadata(). So that applications will receive the same metadata buffer in metadataCompleted and requestCompleted. I wonder if we should emit the metadataCompleted signal at all if the pipeline handler never calls completeMetadata() (it would be easy to check, just make sure that Request:completedMetadata_ is empty with an helper function). > > > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata() > > the full metadata buffer will be in Request::metadata(). > > > > How does an application know what to expect ? > > > > Imo we should advertise if the camera supports partial metadata or not > > (maybe through a property). If it does applications should opt-in to > > receive partial metadata. This could even be done per-configuration by > > adding a field to CameraConfiguration ? > > > > If applications enable early metdata completion they agree to handle > > the new signal, and the behaviour will be the one implemented in this > > patch. If they do not enable early metadata completion then > > PipelineHandler::completeMetadata() should simply merge the partial > > metadata in Request::metadata() and not emit the new signal but return > > the full metadata buffer as part of the Request in requestCompleted, > > as they currently do so that pipelines that use early completion and > > pipelines that do not will behave identically from an application > > perspective. > > > > What do you think ? > > > > > + */ > > > + > > > /** > > > * \var Camera::requestCompleted > > > * \brief Signal emitted when a request queued to the camera has completed > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index e5940469..5d2999cb 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > > > return request->_d()->completeBuffer(buffer); > > > } > > > > > > +/** > > > + * \brief Complete part of metadata for a request > > > + * \param[in] request The request the buffer belongs to > > > > * \param[in] request The Request the metadata buffer belongs to > > Perhaps: > `* \param[in] request The Request the metadata belongs to` > ? > ack > > > > > > > + * \param[in] metadata The partial metadata that has completed > > > + * > > > + * This function could be called by pipeline handlers to signal completion of > > > + * the \a metadata part of the \a request. It notifies applications of metadata > > > + * completion. > > > > I think a little more details are needed. > > > > * This function could be called by pipeline handlers to signal > > * availability of \a metadata before \a request completes. Early > > * metadata completion allows to notify applications about the > > * availability of a partial metadata buffer before the associated > > * Request has completed. > > Thanks! Updated. > > > > > 1) According to this implementation metadata can be completed > > out-of-order (partial metadata for Request X+1 can be signalled before > > Request X completes). Is this desirable ? > > Yes, this is what we expect, and Android also allows that. > WDYT? > I'll ask others what to they think > > > > > + * > > > + * \context This function shall be called from the CameraManager thread. > > > + */ > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > > > +{ > > > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); > > > > Request::addCompleteMetadata constructs and returns a ControlList, > > I'm surprised the compiler doesn't complain you're taking a reference > > to a temporary object (it's apparently legal for const references only). > > > > However, as a temporary will be constructed anyway this doesn't > > buy you any gain, possibily the contrary, as this will disallow C++ > > copy elision from happening (afaiu). I think you should drop & > > Right, it's definitely a mistake. Removed. > > > > > > > > + if (!validMetadata.empty()) { > > > + request->metadata().merge(validMetadata); > > > + > > > + Camera *camera = request->_d()->camera(); > > > + camera->metadataCompleted.emit(request, validMetadata); > > > + } > > > +} > > > + > > > /** > > > * \brief Signal request completion > > > * \param[in] request The request that has completed > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > > > > > Camera::Private *data = camera->_d(); > > > > > > + /* > > > + * Collect metadata which is not yet completed by the Camera, and > > > + * create one partial result to cover the missing metadata before > > > + * completing the whole request. This guarantees the aggregation of > > > + * metadata in completed partial results equals to the global metadata > > > + * in the request. > > > + * > > > + * \todo: Forbid merging metadata into request.metadata() directly and > > > + * force calling completeMetadata() to report metadata. > > > + */ Could you please clarify me what this \todo means ? > > > + const ControlList &validMetadata = request->addCompletedMetadata( > > > + request->metadata()); > > > + if (!validMetadata.empty()) > > > + camera->metadataCompleted.emit(request, validMetadata); > > > + > > > while (!data->queuedRequests_.empty()) { > > > Request *req = data->queuedRequests_.front(); > > > if (req->status() == Request::RequestPending) > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index 8c56ed30..3ce23a8e 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -598,6 +598,26 @@ std::string Request::toString() const > > > return ss.str(); > > > } > > > > > > +/** > > > + * \brief Add completed metadata, as a partial result > > > + * \param[in] metadata The metadata completed > > > + * > > > + * Request will record the entries that has been sent to the application, to > > > + * prevent duplicated controls. > > > + * > > > + * \return ControlList that hasn't been completed before > > > + */ > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > > > +{ > > > + ControlList resultMetadata; > > > + for (auto &[id, value] : metadata) { > > > + if (!completedMetadata_.count(id)) > > > > completedMetadata_ should be cleared in Request::reset(). > > Nice catch! Then how about we move the member variable and the member > function to `Request::Private` instead? > Yes, they both do not need to be exposed to applications indeed. > > > > I wonder if we should actually prevent the same metadata to be > > returned multiple times by the pipeline handler or not. > > This is actually prohibited by Android camera hal [1]: > `Partial metadata submitted should not include any metadata key > returned in a previous partial result for a given frame.` > > We can certainly prevent that in Android Adapter only, while I think > having this restriction in libcamera core library also makes sense. > WDYT? I can also update the description of the new signal. > > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL. I can't find in the text where this is forbidden, however I'm not pushing for the other option, I think it's actually saner to only signal a metadata once per Request. I wonder however if this shouldn't be a WARN as it means the pipeline handler is doing something unexpected. Also, the expectations that a metadata is returned once per Request has to be documented in the documentation of PipelineHandler::completeMetadata() Thanks j > > BR, > Harvey > > > > > + resultMetadata.set(id, value); > > > + } > > > + > > > + return resultMetadata; > > > +} > > > + > > > /** > > > * \brief Insert a text representation of a Request into an output stream > > > * \param[in] out The output stream > > > -- > > > 2.46.0.598.g6f2099f65c-goog > > >
Hi Jacopo, On Thu, Nov 14, 2024 at 4:36 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > I'll upload a new version when all discussion is done. > > > > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Harvey, > > > sorry for the late feedback > > > > > > On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote: > > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > Allows pipeline handler to signal metadata completion by adding the > > > > following signals to Camera: > > > > > > > > Signal<Request *, const ControlList &> metadataCompleted; > > > > > > Bikeshedding on the name a bit, as this is allowed to be called > > > multiple times, with partial metadata, I would call it > > > 'metadataAvailable'. > > > > Sure, 'metadataAvailable' does make more sense. > > Thanks! > > > > > > > > > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to > > > > return buffers and partial metadata at any stage of processing. > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > --- > > > > include/libcamera/camera.h | 1 + > > > > include/libcamera/internal/pipeline_handler.h | 1 + > > > > include/libcamera/request.h | 5 +++ > > > > src/libcamera/camera.cpp | 6 +++ > > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > > > > src/libcamera/request.cpp | 20 ++++++++++ > > > > 6 files changed, 70 insertions(+) > > > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > > index 94cee7bd..08c5c58c 100644 > > > > --- a/include/libcamera/camera.h > > > > +++ b/include/libcamera/camera.h > > > > @@ -122,6 +122,7 @@ public: > > > > > > > > const std::string &id() const; > > > > > > > > + Signal<Request *, const ControlList &> metadataCompleted; > > > > Signal<Request *, FrameBuffer *> bufferCompleted; > > > > Signal<Request *> requestCompleted; > > > > Signal<> disconnected; > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > > index 0d380803..1af63a23 100644 > > > > --- a/include/libcamera/internal/pipeline_handler.h > > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > > @@ -58,6 +58,7 @@ public: > > > > void registerRequest(Request *request); > > > > void queueRequest(Request *request); > > > > > > > > + void completeMetadata(Request *request, const ControlList &metadata); > > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > > > void completeRequest(Request *request); > > > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > > index e214a9d1..0200f4a1 100644 > > > > --- a/include/libcamera/request.h > > > > +++ b/include/libcamera/request.h > > > > @@ -12,6 +12,7 @@ > > > > #include <ostream> > > > > #include <stdint.h> > > > > #include <string> > > > > +#include <unordered_set> > > > > > > > > #include <libcamera/base/class.h> > > > > #include <libcamera/base/signal.h> > > > > @@ -64,6 +65,8 @@ public: > > > > > > > > std::string toString() const; > > > > > > > > + ControlList addCompletedMetadata(const ControlList &metadata); > > > > + > > > > private: > > > > LIBCAMERA_DISABLE_COPY(Request) > > > > > > > > @@ -73,6 +76,8 @@ private: > > > > > > > > const uint64_t cookie_; > > > > Status status_; > > > > + > > > > + std::unordered_set<unsigned int> completedMetadata_; > > > > }; > > > > > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > index a86f552a..5ffae23e 100644 > > > > --- a/src/libcamera/camera.cpp > > > > +++ b/src/libcamera/camera.cpp > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > > > > * completed > > > > */ > > > > > > > > +/** > > > > + * \var Camera::metadataCompleted > > > > + * \brief Signal emitted when some metadata for a request is completed as a > > > > + * partial result > > > > > > Currently, we return metadata as part of a Request signalled with > > > Camera::requestCompleted. Existing applications do not handle the new > > > signal and I don't think we should make it mandatory to do so. > > > According to the implementation of this patch, the same application > > > running on a pipeline handler that calls > > > PipelineHandler::completeMetadata and on a pipeline handler that > > > doesn't will receive different sets of metadata. > > > > > > - If a pipeline handler calls PipelineHandler::completeMetadata() it > > > will send metadata in chunks and only the ones not previously > > > signalled with be part of Request::metadata(). > > > > Actually, it's not true. > > In this patch, `PipelineHandler::completeMetadata` will merge the > > partially available metadata into Request: > > `request->metadata().merge(validMetadata);` > > > > Ups, sorry I missed it! > > > Therefore, whether an application handles the new signal, it can > > still get access to all the metadata in a request via > > `Request.metadata()`. This should ease your concern of the old > > and new implementations of applications. > > > > Yes, if this time I got this right, the full metadata pack will be > available in Request::metadata() at requestCompleted time, but it will > be completed in chunks if the pipeline handler calls > completeMetadata() earlier. > > There's only one thing left that I'm not sure about: in > requestCompleted() > > const ControlList &validMetadata = request->addCompletedMetadata( > request->metadata()); > if (!validMetadata.empty()) > camera->metadataCompleted.emit(request, validMetadata); > > If the pipeline handler never calls completeMetadata() then > validMetadata == request->metadata(). So that applications will > receive the same metadata buffer in metadataCompleted and > requestCompleted. I wonder if we should emit the metadataCompleted > signal at all if the pipeline handler never calls completeMetadata() > (it would be easy to check, just make sure that Request:completedMetadata_ > is empty with an helper function). I think this is how we expect a pipeline handler to behave and how an application to handle signals. In the current implementation, it's safer for applications that whether a pipeline handler calls completeMetadata, applications still get full metadata set with the new signal, and they don't need to rely on signal requestCompleted. In other words, I want to guarantee that applications can choose to use either metadataAvailable or requestCompleted. Triggering the new signal if there's any metadata left is just a safer option to support all pipeline handlers. Of course we can choose to check if `Request::Private::completedMetadata_` is empty, and only handle this case, and expect the proper implementations of pipeline handlers call completeMetadata properly without missing any metadata. I just feel that it's unnecessary. Also, I'm not sure if there's a case that pipeline handler doesn't want to report some metadata earlier, and would rather send them altogether when a request is completed. In this case, your approach would require a pipeline handler to record those unsent metadata on its own. WDYT? > > > > > > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata() > > > the full metadata buffer will be in Request::metadata(). > > > > > > How does an application know what to expect ? > > > > > > Imo we should advertise if the camera supports partial metadata or not > > > (maybe through a property). If it does applications should opt-in to > > > receive partial metadata. This could even be done per-configuration by > > > adding a field to CameraConfiguration ? > > > > > > If applications enable early metdata completion they agree to handle > > > the new signal, and the behaviour will be the one implemented in this > > > patch. If they do not enable early metadata completion then > > > PipelineHandler::completeMetadata() should simply merge the partial > > > metadata in Request::metadata() and not emit the new signal but return > > > the full metadata buffer as part of the Request in requestCompleted, > > > as they currently do so that pipelines that use early completion and > > > pipelines that do not will behave identically from an application > > > perspective. > > > > > > What do you think ? > > > > > > > + */ > > > > + > > > > /** > > > > * \var Camera::requestCompleted > > > > * \brief Signal emitted when a request queued to the camera has completed > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > index e5940469..5d2999cb 100644 > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > > > > return request->_d()->completeBuffer(buffer); > > > > } > > > > > > > > +/** > > > > + * \brief Complete part of metadata for a request > > > > + * \param[in] request The request the buffer belongs to > > > > > > * \param[in] request The Request the metadata buffer belongs to > > > > Perhaps: > > `* \param[in] request The Request the metadata belongs to` > > ? > > > > ack > > > > > > > > > > > + * \param[in] metadata The partial metadata that has completed > > > > + * > > > > + * This function could be called by pipeline handlers to signal completion of > > > > + * the \a metadata part of the \a request. It notifies applications of metadata > > > > + * completion. > > > > > > I think a little more details are needed. > > > > > > * This function could be called by pipeline handlers to signal > > > * availability of \a metadata before \a request completes. Early > > > * metadata completion allows to notify applications about the > > > * availability of a partial metadata buffer before the associated > > > * Request has completed. > > > > Thanks! Updated. > > > > > > > > 1) According to this implementation metadata can be completed > > > out-of-order (partial metadata for Request X+1 can be signalled before > > > Request X completes). Is this desirable ? > > > > Yes, this is what we expect, and Android also allows that. > > WDYT? > > > > I'll ask others what to they think > > > > > > > > + * > > > > + * \context This function shall be called from the CameraManager thread. > > > > + */ > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > > > > +{ > > > > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); > > > > > > Request::addCompleteMetadata constructs and returns a ControlList, > > > I'm surprised the compiler doesn't complain you're taking a reference > > > to a temporary object (it's apparently legal for const references only). > > > > > > However, as a temporary will be constructed anyway this doesn't > > > buy you any gain, possibily the contrary, as this will disallow C++ > > > copy elision from happening (afaiu). I think you should drop & > > > > Right, it's definitely a mistake. Removed. > > > > > > > > > > > > + if (!validMetadata.empty()) { > > > > + request->metadata().merge(validMetadata); > > > > + > > > > + Camera *camera = request->_d()->camera(); > > > > + camera->metadataCompleted.emit(request, validMetadata); > > > > + } > > > > +} > > > > + > > > > /** > > > > * \brief Signal request completion > > > > * \param[in] request The request that has completed > > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > > > > > > > Camera::Private *data = camera->_d(); > > > > > > > > + /* > > > > + * Collect metadata which is not yet completed by the Camera, and > > > > + * create one partial result to cover the missing metadata before > > > > + * completing the whole request. This guarantees the aggregation of > > > > + * metadata in completed partial results equals to the global metadata > > > > + * in the request. > > > > + * > > > > + * \todo: Forbid merging metadata into request.metadata() directly and > > > > + * force calling completeMetadata() to report metadata. > > > > + */ > > Could you please clarify me what this \todo means ? Similar to the above reasons: Currently all pipeline handlers merge metadata into request.metadata() directly, except for the upcoming mtkisp7. We were expecting every pipeline handler to migrate to `PipelineHandler::completeMetadata` eventually. Let me know if you think this todo doesn't make sense, as we should still allow the previous way to merge into request.metadata() directly, or there's a case that a pipeline handler doesn't want to report some metadata earlier. > > > > > + const ControlList &validMetadata = request->addCompletedMetadata( > > > > + request->metadata()); > > > > + if (!validMetadata.empty()) > > > > + camera->metadataCompleted.emit(request, validMetadata); > > > > + > > > > while (!data->queuedRequests_.empty()) { > > > > Request *req = data->queuedRequests_.front(); > > > > if (req->status() == Request::RequestPending) > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > index 8c56ed30..3ce23a8e 100644 > > > > --- a/src/libcamera/request.cpp > > > > +++ b/src/libcamera/request.cpp > > > > @@ -598,6 +598,26 @@ std::string Request::toString() const > > > > return ss.str(); > > > > } > > > > > > > > +/** > > > > + * \brief Add completed metadata, as a partial result > > > > + * \param[in] metadata The metadata completed > > > > + * > > > > + * Request will record the entries that has been sent to the application, to > > > > + * prevent duplicated controls. > > > > + * > > > > + * \return ControlList that hasn't been completed before > > > > + */ > > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > > > > +{ > > > > + ControlList resultMetadata; > > > > + for (auto &[id, value] : metadata) { > > > > + if (!completedMetadata_.count(id)) > > > > > > completedMetadata_ should be cleared in Request::reset(). > > > > Nice catch! Then how about we move the member variable and the member > > function to `Request::Private` instead? > > > > Yes, they both do not need to be exposed to applications indeed. > > > > > > > I wonder if we should actually prevent the same metadata to be > > > returned multiple times by the pipeline handler or not. > > > > This is actually prohibited by Android camera hal [1]: > > `Partial metadata submitted should not include any metadata key > > returned in a previous partial result for a given frame.` > > > > We can certainly prevent that in Android Adapter only, while I think > > having this restriction in libcamera core library also makes sense. > > WDYT? I can also update the description of the new signal. > > > > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL. > > I can't find in the text where this is forbidden, however I'm not > pushing for the other option, I think it's actually saner to only > signal a metadata once per Request. I wonder however if this shouldn't > be a WARN as it means the pipeline handler is doing something > unexpected. True that a WARN is an option. I would prefer the current approach though, otherwise I need to implement the logic in Android Adapter instead. Please let me know if anyone insists on the other approach. > Also, the expectations that a metadata is returned once > per Request has to be documented in the documentation of > PipelineHandler::completeMetadata() I'll update the doc in the next version :) BR, Harvey > > Thanks > j > > > > > BR, > > Harvey > > > > > > > + resultMetadata.set(id, value); > > > > + } > > > > + > > > > + return resultMetadata; > > > > +} > > > > + > > > > /** > > > > * \brief Insert a text representation of a Request into an output stream > > > > * \param[in] out The output stream > > > > -- > > > > 2.46.0.598.g6f2099f65c-goog > > > >
Hi Harvey On Thu, Nov 14, 2024 at 05:11:42PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo, > > On Thu, Nov 14, 2024 at 4:36 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Harvey > > > > On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote: > > > Hi Jacopo, > > > > > > I'll upload a new version when all discussion is done. > > > > > > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi Harvey, > > > > sorry for the late feedback > > > > > > > > On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote: > > > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > > > Allows pipeline handler to signal metadata completion by adding the > > > > > following signals to Camera: > > > > > > > > > > Signal<Request *, const ControlList &> metadataCompleted; > > > > > > > > Bikeshedding on the name a bit, as this is allowed to be called > > > > multiple times, with partial metadata, I would call it > > > > 'metadataAvailable'. > > > > > > Sure, 'metadataAvailable' does make more sense. > > > Thanks! > > > > > > > > > > > > > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to > > > > > return buffers and partial metadata at any stage of processing. > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > --- > > > > > include/libcamera/camera.h | 1 + > > > > > include/libcamera/internal/pipeline_handler.h | 1 + > > > > > include/libcamera/request.h | 5 +++ > > > > > src/libcamera/camera.cpp | 6 +++ > > > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > > > > > src/libcamera/request.cpp | 20 ++++++++++ > > > > > 6 files changed, 70 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > > > index 94cee7bd..08c5c58c 100644 > > > > > --- a/include/libcamera/camera.h > > > > > +++ b/include/libcamera/camera.h > > > > > @@ -122,6 +122,7 @@ public: > > > > > > > > > > const std::string &id() const; > > > > > > > > > > + Signal<Request *, const ControlList &> metadataCompleted; > > > > > Signal<Request *, FrameBuffer *> bufferCompleted; > > > > > Signal<Request *> requestCompleted; > > > > > Signal<> disconnected; > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > > > index 0d380803..1af63a23 100644 > > > > > --- a/include/libcamera/internal/pipeline_handler.h > > > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > > > @@ -58,6 +58,7 @@ public: > > > > > void registerRequest(Request *request); > > > > > void queueRequest(Request *request); > > > > > > > > > > + void completeMetadata(Request *request, const ControlList &metadata); > > > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > > > > void completeRequest(Request *request); > > > > > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > > > index e214a9d1..0200f4a1 100644 > > > > > --- a/include/libcamera/request.h > > > > > +++ b/include/libcamera/request.h > > > > > @@ -12,6 +12,7 @@ > > > > > #include <ostream> > > > > > #include <stdint.h> > > > > > #include <string> > > > > > +#include <unordered_set> > > > > > > > > > > #include <libcamera/base/class.h> > > > > > #include <libcamera/base/signal.h> > > > > > @@ -64,6 +65,8 @@ public: > > > > > > > > > > std::string toString() const; > > > > > > > > > > + ControlList addCompletedMetadata(const ControlList &metadata); > > > > > + > > > > > private: > > > > > LIBCAMERA_DISABLE_COPY(Request) > > > > > > > > > > @@ -73,6 +76,8 @@ private: > > > > > > > > > > const uint64_t cookie_; > > > > > Status status_; > > > > > + > > > > > + std::unordered_set<unsigned int> completedMetadata_; > > > > > }; > > > > > > > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > > index a86f552a..5ffae23e 100644 > > > > > --- a/src/libcamera/camera.cpp > > > > > +++ b/src/libcamera/camera.cpp > > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > > > > > * completed > > > > > */ > > > > > > > > > > +/** > > > > > + * \var Camera::metadataCompleted > > > > > + * \brief Signal emitted when some metadata for a request is completed as a > > > > > + * partial result > > > > > > > > Currently, we return metadata as part of a Request signalled with > > > > Camera::requestCompleted. Existing applications do not handle the new > > > > signal and I don't think we should make it mandatory to do so. > > > > According to the implementation of this patch, the same application > > > > running on a pipeline handler that calls > > > > PipelineHandler::completeMetadata and on a pipeline handler that > > > > doesn't will receive different sets of metadata. > > > > > > > > - If a pipeline handler calls PipelineHandler::completeMetadata() it > > > > will send metadata in chunks and only the ones not previously > > > > signalled with be part of Request::metadata(). > > > > > > Actually, it's not true. > > > In this patch, `PipelineHandler::completeMetadata` will merge the > > > partially available metadata into Request: > > > `request->metadata().merge(validMetadata);` > > > > > > > Ups, sorry I missed it! > > > > > Therefore, whether an application handles the new signal, it can > > > still get access to all the metadata in a request via > > > `Request.metadata()`. This should ease your concern of the old > > > and new implementations of applications. > > > > > > > Yes, if this time I got this right, the full metadata pack will be > > available in Request::metadata() at requestCompleted time, but it will > > be completed in chunks if the pipeline handler calls > > completeMetadata() earlier. > > > > There's only one thing left that I'm not sure about: in > > requestCompleted() > > > > const ControlList &validMetadata = request->addCompletedMetadata( > > request->metadata()); > > if (!validMetadata.empty()) > > camera->metadataCompleted.emit(request, validMetadata); > > > > If the pipeline handler never calls completeMetadata() then > > validMetadata == request->metadata(). So that applications will > > receive the same metadata buffer in metadataCompleted and > > requestCompleted. I wonder if we should emit the metadataCompleted > > signal at all if the pipeline handler never calls completeMetadata() > > (it would be easy to check, just make sure that Request:completedMetadata_ > > is empty with an helper function). > > I think this is how we expect a pipeline handler to behave and how an > application to handle signals. Do you expect all pipelines to use the new signal ? I quickly checked rkisp1 where most metadata are computed by the IPA in one go after parsing statistics. Only the frame timestamp and the scaler crop are available at different times (the buffer completion, so possibily later than the IPA computed ones). > > In the current implementation, it's safer for applications that whether > a pipeline handler calls completeMetadata, applications still get full > metadata set with the new signal, and they don't need to rely on > signal requestCompleted. In other words, I want to guarantee that > applications can choose to use either metadataAvailable or > requestCompleted. > Triggering the new signal if there's any metadata left is just a safer > option to support all pipeline handlers. My concern is about sending over a signal a possibly long list of controls two times in the case all metadata are sent in a single chunk at requestComplete() time. > > Of course we can choose to check if > `Request::Private::completedMetadata_` is empty, and only handle > this case, and expect the proper implementations of pipeline > handlers call completeMetadata properly without missing any > metadata. I just feel that it's unnecessary. > Also, I'm not sure if there's a case that pipeline handler doesn't > want to report some metadata earlier, and would rather send > them altogether when a request is completed. In this case, > your approach would require a pipeline handler to record > those unsent metadata on its own. > > WDYT? > > > > > > > > > > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata() > > > > the full metadata buffer will be in Request::metadata(). > > > > > > > > How does an application know what to expect ? > > > > > > > > Imo we should advertise if the camera supports partial metadata or not > > > > (maybe through a property). If it does applications should opt-in to > > > > receive partial metadata. This could even be done per-configuration by > > > > adding a field to CameraConfiguration ? > > > > > > > > If applications enable early metdata completion they agree to handle > > > > the new signal, and the behaviour will be the one implemented in this > > > > patch. If they do not enable early metadata completion then > > > > PipelineHandler::completeMetadata() should simply merge the partial > > > > metadata in Request::metadata() and not emit the new signal but return > > > > the full metadata buffer as part of the Request in requestCompleted, > > > > as they currently do so that pipelines that use early completion and > > > > pipelines that do not will behave identically from an application > > > > perspective. > > > > > > > > What do you think ? > > > > > > > > > + */ > > > > > + > > > > > /** > > > > > * \var Camera::requestCompleted > > > > > * \brief Signal emitted when a request queued to the camera has completed > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > > index e5940469..5d2999cb 100644 > > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > > > > > return request->_d()->completeBuffer(buffer); > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Complete part of metadata for a request > > > > > + * \param[in] request The request the buffer belongs to > > > > > > > > * \param[in] request The Request the metadata buffer belongs to > > > > > > Perhaps: > > > `* \param[in] request The Request the metadata belongs to` > > > ? > > > > > > > ack > > > > > > > > > > > > > > > + * \param[in] metadata The partial metadata that has completed > > > > > + * > > > > > + * This function could be called by pipeline handlers to signal completion of > > > > > + * the \a metadata part of the \a request. It notifies applications of metadata > > > > > + * completion. > > > > > > > > I think a little more details are needed. > > > > > > > > * This function could be called by pipeline handlers to signal > > > > * availability of \a metadata before \a request completes. Early > > > > * metadata completion allows to notify applications about the > > > > * availability of a partial metadata buffer before the associated > > > > * Request has completed. > > > > > > Thanks! Updated. > > > > > > > > > > > 1) According to this implementation metadata can be completed > > > > out-of-order (partial metadata for Request X+1 can be signalled before > > > > Request X completes). Is this desirable ? > > > > > > Yes, this is what we expect, and Android also allows that. > > > WDYT? > > > > > > > I'll ask others what to they think > > > > > > > > > > > + * > > > > > + * \context This function shall be called from the CameraManager thread. > > > > > + */ > > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > > > > > +{ > > > > > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); > > > > > > > > Request::addCompleteMetadata constructs and returns a ControlList, > > > > I'm surprised the compiler doesn't complain you're taking a reference > > > > to a temporary object (it's apparently legal for const references only). > > > > > > > > However, as a temporary will be constructed anyway this doesn't > > > > buy you any gain, possibily the contrary, as this will disallow C++ > > > > copy elision from happening (afaiu). I think you should drop & > > > > > > Right, it's definitely a mistake. Removed. > > > > > > > > > > > > > > > > + if (!validMetadata.empty()) { > > > > > + request->metadata().merge(validMetadata); > > > > > + > > > > > + Camera *camera = request->_d()->camera(); > > > > > + camera->metadataCompleted.emit(request, validMetadata); > > > > > + } > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Signal request completion > > > > > * \param[in] request The request that has completed > > > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > > > > > > > > > Camera::Private *data = camera->_d(); > > > > > > > > > > + /* > > > > > + * Collect metadata which is not yet completed by the Camera, and > > > > > + * create one partial result to cover the missing metadata before > > > > > + * completing the whole request. This guarantees the aggregation of > > > > > + * metadata in completed partial results equals to the global metadata > > > > > + * in the request. > > > > > + * > > > > > + * \todo: Forbid merging metadata into request.metadata() directly and > > > > > + * force calling completeMetadata() to report metadata. > > > > > + */ > > > > Could you please clarify me what this \todo means ? > > Similar to the above reasons: > Currently all pipeline handlers merge metadata into request.metadata() > directly, except for the upcoming mtkisp7. We were expecting every > pipeline handler to migrate to `PipelineHandler::completeMetadata` > eventually. > > Let me know if you think this todo doesn't make sense, as we should > still allow the previous way to merge into request.metadata() directly, > or there's a case that a pipeline handler doesn't want to report some > metadata earlier. > I'm just not 100% sure all pipelines will be capable of actually send metadata earlier. True, in the RkISP1 case I used as an example as soon as the IPA has processed stats and computed metadata the pipeline can call metadataComplete() but we have no guarantee the application handles the signal, so we end up sending the same metadata list twice (one in chunks, the other as Request::metadata() at requestComplete() time. I'll check with others what are the implications in terms of performances of this. Thanks j > > > > > > > + const ControlList &validMetadata = request->addCompletedMetadata( > > > > > + request->metadata()); > > > > > + if (!validMetadata.empty()) > > > > > + camera->metadataCompleted.emit(request, validMetadata); > > > > > + > > > > > while (!data->queuedRequests_.empty()) { > > > > > Request *req = data->queuedRequests_.front(); > > > > > if (req->status() == Request::RequestPending) > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > > index 8c56ed30..3ce23a8e 100644 > > > > > --- a/src/libcamera/request.cpp > > > > > +++ b/src/libcamera/request.cpp > > > > > @@ -598,6 +598,26 @@ std::string Request::toString() const > > > > > return ss.str(); > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Add completed metadata, as a partial result > > > > > + * \param[in] metadata The metadata completed > > > > > + * > > > > > + * Request will record the entries that has been sent to the application, to > > > > > + * prevent duplicated controls. > > > > > + * > > > > > + * \return ControlList that hasn't been completed before > > > > > + */ > > > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > > > > > +{ > > > > > + ControlList resultMetadata; > > > > > + for (auto &[id, value] : metadata) { > > > > > + if (!completedMetadata_.count(id)) > > > > > > > > completedMetadata_ should be cleared in Request::reset(). > > > > > > Nice catch! Then how about we move the member variable and the member > > > function to `Request::Private` instead? > > > > > > > Yes, they both do not need to be exposed to applications indeed. > > > > > > > > > > I wonder if we should actually prevent the same metadata to be > > > > returned multiple times by the pipeline handler or not. > > > > > > This is actually prohibited by Android camera hal [1]: > > > `Partial metadata submitted should not include any metadata key > > > returned in a previous partial result for a given frame.` > > > > > > We can certainly prevent that in Android Adapter only, while I think > > > having this restriction in libcamera core library also makes sense. > > > WDYT? I can also update the description of the new signal. > > > > > > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL. > > > > I can't find in the text where this is forbidden, however I'm not > > pushing for the other option, I think it's actually saner to only > > signal a metadata once per Request. I wonder however if this shouldn't > > be a WARN as it means the pipeline handler is doing something > > unexpected. > > True that a WARN is an option. > I would prefer the current approach though, otherwise I need to > implement the logic in Android Adapter instead. > > Please let me know if anyone insists on the other approach. > > > Also, the expectations that a metadata is returned once > > per Request has to be documented in the documentation of > > PipelineHandler::completeMetadata() > > I'll update the doc in the next version :) > > BR, > Harvey > > > > > Thanks > > j > > > > > > > > BR, > > > Harvey > > > > > > > > > + resultMetadata.set(id, value); > > > > > + } > > > > > + > > > > > + return resultMetadata; > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Insert a text representation of a Request into an output stream > > > > > * \param[in] out The output stream > > > > > -- > > > > > 2.46.0.598.g6f2099f65c-goog > > > > >
Hi Jacopo, On Thu, Nov 14, 2024 at 6:17 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Thu, Nov 14, 2024 at 05:11:42PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > On Thu, Nov 14, 2024 at 4:36 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Harvey > > > > > > On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote: > > > > Hi Jacopo, > > > > > > > > I'll upload a new version when all discussion is done. > > > > > > > > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > Hi Harvey, > > > > > sorry for the late feedback > > > > > > > > > > On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote: > > > > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > > > > > Allows pipeline handler to signal metadata completion by adding the > > > > > > following signals to Camera: > > > > > > > > > > > > Signal<Request *, const ControlList &> metadataCompleted; > > > > > > > > > > Bikeshedding on the name a bit, as this is allowed to be called > > > > > multiple times, with partial metadata, I would call it > > > > > 'metadataAvailable'. > > > > > > > > Sure, 'metadataAvailable' does make more sense. > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to > > > > > > return buffers and partial metadata at any stage of processing. > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > > --- > > > > > > include/libcamera/camera.h | 1 + > > > > > > include/libcamera/internal/pipeline_handler.h | 1 + > > > > > > include/libcamera/request.h | 5 +++ > > > > > > src/libcamera/camera.cpp | 6 +++ > > > > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++ > > > > > > src/libcamera/request.cpp | 20 ++++++++++ > > > > > > 6 files changed, 70 insertions(+) > > > > > > > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > > > > index 94cee7bd..08c5c58c 100644 > > > > > > --- a/include/libcamera/camera.h > > > > > > +++ b/include/libcamera/camera.h > > > > > > @@ -122,6 +122,7 @@ public: > > > > > > > > > > > > const std::string &id() const; > > > > > > > > > > > > + Signal<Request *, const ControlList &> metadataCompleted; > > > > > > Signal<Request *, FrameBuffer *> bufferCompleted; > > > > > > Signal<Request *> requestCompleted; > > > > > > Signal<> disconnected; > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > > > > index 0d380803..1af63a23 100644 > > > > > > --- a/include/libcamera/internal/pipeline_handler.h > > > > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > > > > @@ -58,6 +58,7 @@ public: > > > > > > void registerRequest(Request *request); > > > > > > void queueRequest(Request *request); > > > > > > > > > > > > + void completeMetadata(Request *request, const ControlList &metadata); > > > > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > > > > > void completeRequest(Request *request); > > > > > > > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > > > > index e214a9d1..0200f4a1 100644 > > > > > > --- a/include/libcamera/request.h > > > > > > +++ b/include/libcamera/request.h > > > > > > @@ -12,6 +12,7 @@ > > > > > > #include <ostream> > > > > > > #include <stdint.h> > > > > > > #include <string> > > > > > > +#include <unordered_set> > > > > > > > > > > > > #include <libcamera/base/class.h> > > > > > > #include <libcamera/base/signal.h> > > > > > > @@ -64,6 +65,8 @@ public: > > > > > > > > > > > > std::string toString() const; > > > > > > > > > > > > + ControlList addCompletedMetadata(const ControlList &metadata); > > > > > > + > > > > > > private: > > > > > > LIBCAMERA_DISABLE_COPY(Request) > > > > > > > > > > > > @@ -73,6 +76,8 @@ private: > > > > > > > > > > > > const uint64_t cookie_; > > > > > > Status status_; > > > > > > + > > > > > > + std::unordered_set<unsigned int> completedMetadata_; > > > > > > }; > > > > > > > > > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > > > index a86f552a..5ffae23e 100644 > > > > > > --- a/src/libcamera/camera.cpp > > > > > > +++ b/src/libcamera/camera.cpp > > > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const > > > > > > * completed > > > > > > */ > > > > > > > > > > > > +/** > > > > > > + * \var Camera::metadataCompleted > > > > > > + * \brief Signal emitted when some metadata for a request is completed as a > > > > > > + * partial result > > > > > > > > > > Currently, we return metadata as part of a Request signalled with > > > > > Camera::requestCompleted. Existing applications do not handle the new > > > > > signal and I don't think we should make it mandatory to do so. > > > > > According to the implementation of this patch, the same application > > > > > running on a pipeline handler that calls > > > > > PipelineHandler::completeMetadata and on a pipeline handler that > > > > > doesn't will receive different sets of metadata. > > > > > > > > > > - If a pipeline handler calls PipelineHandler::completeMetadata() it > > > > > will send metadata in chunks and only the ones not previously > > > > > signalled with be part of Request::metadata(). > > > > > > > > Actually, it's not true. > > > > In this patch, `PipelineHandler::completeMetadata` will merge the > > > > partially available metadata into Request: > > > > `request->metadata().merge(validMetadata);` > > > > > > > > > > Ups, sorry I missed it! > > > > > > > Therefore, whether an application handles the new signal, it can > > > > still get access to all the metadata in a request via > > > > `Request.metadata()`. This should ease your concern of the old > > > > and new implementations of applications. > > > > > > > > > > Yes, if this time I got this right, the full metadata pack will be > > > available in Request::metadata() at requestCompleted time, but it will > > > be completed in chunks if the pipeline handler calls > > > completeMetadata() earlier. > > > > > > There's only one thing left that I'm not sure about: in > > > requestCompleted() > > > > > > const ControlList &validMetadata = request->addCompletedMetadata( > > > request->metadata()); > > > if (!validMetadata.empty()) > > > camera->metadataCompleted.emit(request, validMetadata); > > > > > > If the pipeline handler never calls completeMetadata() then > > > validMetadata == request->metadata(). So that applications will > > > receive the same metadata buffer in metadataCompleted and > > > requestCompleted. I wonder if we should emit the metadataCompleted > > > signal at all if the pipeline handler never calls completeMetadata() > > > (it would be easy to check, just make sure that Request:completedMetadata_ > > > is empty with an helper function). > > > > I think this is how we expect a pipeline handler to behave and how an > > application to handle signals. > > Do you expect all pipelines to use the new signal ? Not really, while I still think that we need to ensure the new signal sends a complete set of metadata keys. > > I quickly checked rkisp1 where most metadata are computed by the IPA > in one go after parsing statistics. Only the frame timestamp and the > scaler crop are available at different times (the buffer completion, > so possibily later than the IPA computed ones). > > > > > In the current implementation, it's safer for applications that whether > > a pipeline handler calls completeMetadata, applications still get full > > metadata set with the new signal, and they don't need to rely on > > signal requestCompleted. In other words, I want to guarantee that > > applications can choose to use either metadataAvailable or > > requestCompleted. > > Triggering the new signal if there's any metadata left is just a safer > > option to support all pipeline handlers. > > My concern is about sending over a signal a possibly long list of > controls two times in the case all metadata are sent in a single chunk > at requestComplete() time. > > > > > Of course we can choose to check if > > `Request::Private::completedMetadata_` is empty, and only handle > > this case, and expect the proper implementations of pipeline > > handlers call completeMetadata properly without missing any > > metadata. I just feel that it's unnecessary. > > Also, I'm not sure if there's a case that pipeline handler doesn't > > want to report some metadata earlier, and would rather send > > them altogether when a request is completed. In this case, > > your approach would require a pipeline handler to record > > those unsent metadata on its own. > > > > WDYT? > > > > > > > > > > > > > > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata() > > > > > the full metadata buffer will be in Request::metadata(). > > > > > > > > > > How does an application know what to expect ? > > > > > > > > > > Imo we should advertise if the camera supports partial metadata or not > > > > > (maybe through a property). If it does applications should opt-in to > > > > > receive partial metadata. This could even be done per-configuration by > > > > > adding a field to CameraConfiguration ? > > > > > > > > > > If applications enable early metdata completion they agree to handle > > > > > the new signal, and the behaviour will be the one implemented in this > > > > > patch. If they do not enable early metadata completion then > > > > > PipelineHandler::completeMetadata() should simply merge the partial > > > > > metadata in Request::metadata() and not emit the new signal but return > > > > > the full metadata buffer as part of the Request in requestCompleted, > > > > > as they currently do so that pipelines that use early completion and > > > > > pipelines that do not will behave identically from an application > > > > > perspective. > > > > > > > > > > What do you think ? > > > > > > > > > > > + */ > > > > > > + > > > > > > /** > > > > > > * \var Camera::requestCompleted > > > > > > * \brief Signal emitted when a request queued to the camera has completed > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > > > index e5940469..5d2999cb 100644 > > > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > > > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) > > > > > > return request->_d()->completeBuffer(buffer); > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * \brief Complete part of metadata for a request > > > > > > + * \param[in] request The request the buffer belongs to > > > > > > > > > > * \param[in] request The Request the metadata buffer belongs to > > > > > > > > Perhaps: > > > > `* \param[in] request The Request the metadata belongs to` > > > > ? > > > > > > > > > > ack > > > > > > > > > > > > > > > > > > > + * \param[in] metadata The partial metadata that has completed > > > > > > + * > > > > > > + * This function could be called by pipeline handlers to signal completion of > > > > > > + * the \a metadata part of the \a request. It notifies applications of metadata > > > > > > + * completion. > > > > > > > > > > I think a little more details are needed. > > > > > > > > > > * This function could be called by pipeline handlers to signal > > > > > * availability of \a metadata before \a request completes. Early > > > > > * metadata completion allows to notify applications about the > > > > > * availability of a partial metadata buffer before the associated > > > > > * Request has completed. > > > > > > > > Thanks! Updated. > > > > > > > > > > > > > > 1) According to this implementation metadata can be completed > > > > > out-of-order (partial metadata for Request X+1 can be signalled before > > > > > Request X completes). Is this desirable ? > > > > > > > > Yes, this is what we expect, and Android also allows that. > > > > WDYT? > > > > > > > > > > I'll ask others what to they think > > > > > > > > > > > > > > + * > > > > > > + * \context This function shall be called from the CameraManager thread. > > > > > > + */ > > > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) > > > > > > +{ > > > > > > + const ControlList &validMetadata = request->addCompletedMetadata(metadata); > > > > > > > > > > Request::addCompleteMetadata constructs and returns a ControlList, > > > > > I'm surprised the compiler doesn't complain you're taking a reference > > > > > to a temporary object (it's apparently legal for const references only). > > > > > > > > > > However, as a temporary will be constructed anyway this doesn't > > > > > buy you any gain, possibily the contrary, as this will disallow C++ > > > > > copy elision from happening (afaiu). I think you should drop & > > > > > > > > Right, it's definitely a mistake. Removed. > > > > > > > > > > > > > > > > > > > > + if (!validMetadata.empty()) { > > > > > > + request->metadata().merge(validMetadata); > > > > > > + > > > > > > + Camera *camera = request->_d()->camera(); > > > > > > + camera->metadataCompleted.emit(request, validMetadata); > > > > > > + } > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * \brief Signal request completion > > > > > > * \param[in] request The request that has completed > > > > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) > > > > > > > > > > > > Camera::Private *data = camera->_d(); > > > > > > > > > > > > + /* > > > > > > + * Collect metadata which is not yet completed by the Camera, and > > > > > > + * create one partial result to cover the missing metadata before > > > > > > + * completing the whole request. This guarantees the aggregation of > > > > > > + * metadata in completed partial results equals to the global metadata > > > > > > + * in the request. > > > > > > + * > > > > > > + * \todo: Forbid merging metadata into request.metadata() directly and > > > > > > + * force calling completeMetadata() to report metadata. > > > > > > + */ > > > > > > Could you please clarify me what this \todo means ? > > > > Similar to the above reasons: > > Currently all pipeline handlers merge metadata into request.metadata() > > directly, except for the upcoming mtkisp7. We were expecting every > > pipeline handler to migrate to `PipelineHandler::completeMetadata` > > eventually. > > > > Let me know if you think this todo doesn't make sense, as we should > > still allow the previous way to merge into request.metadata() directly, > > or there's a case that a pipeline handler doesn't want to report some > > metadata earlier. > > > > I'm just not 100% sure all pipelines will be capable of actually send > metadata earlier. True, in the RkISP1 case I used as an example as > soon as the IPA has processed stats and computed metadata the pipeline > can call metadataComplete() but we have no guarantee the application > handles the signal, so we end up sending the same metadata list twice > (one in chunks, the other as Request::metadata() at requestComplete() > time. I understand your concern, while `libcamera::Signal` doesn't do anything if there's no slot connected. We still prepare the data chunks though. We can also check if `libcamera::Signal::slots()` is empty before preparing the data to be sent. WDYT? BR, Harvey > > I'll check with others what are the implications in terms of > performances of this. > > Thanks > j > > > > > > > > > > + const ControlList &validMetadata = request->addCompletedMetadata( > > > > > > + request->metadata()); > > > > > > + if (!validMetadata.empty()) > > > > > > + camera->metadataCompleted.emit(request, validMetadata); > > > > > > + > > > > > > while (!data->queuedRequests_.empty()) { > > > > > > Request *req = data->queuedRequests_.front(); > > > > > > if (req->status() == Request::RequestPending) > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > > > index 8c56ed30..3ce23a8e 100644 > > > > > > --- a/src/libcamera/request.cpp > > > > > > +++ b/src/libcamera/request.cpp > > > > > > @@ -598,6 +598,26 @@ std::string Request::toString() const > > > > > > return ss.str(); > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * \brief Add completed metadata, as a partial result > > > > > > + * \param[in] metadata The metadata completed > > > > > > + * > > > > > > + * Request will record the entries that has been sent to the application, to > > > > > > + * prevent duplicated controls. > > > > > > + * > > > > > > + * \return ControlList that hasn't been completed before > > > > > > + */ > > > > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata) > > > > > > +{ > > > > > > + ControlList resultMetadata; > > > > > > + for (auto &[id, value] : metadata) { > > > > > > + if (!completedMetadata_.count(id)) > > > > > > > > > > completedMetadata_ should be cleared in Request::reset(). > > > > > > > > Nice catch! Then how about we move the member variable and the member > > > > function to `Request::Private` instead? > > > > > > > > > > Yes, they both do not need to be exposed to applications indeed. > > > > > > > > > > > > > I wonder if we should actually prevent the same metadata to be > > > > > returned multiple times by the pipeline handler or not. > > > > > > > > This is actually prohibited by Android camera hal [1]: > > > > `Partial metadata submitted should not include any metadata key > > > > returned in a previous partial result for a given frame.` > > > > > > > > We can certainly prevent that in Android Adapter only, while I think > > > > having this restriction in libcamera core library also makes sense. > > > > WDYT? I can also update the description of the new signal. > > > > > > > > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL. > > > > > > I can't find in the text where this is forbidden, however I'm not > > > pushing for the other option, I think it's actually saner to only > > > signal a metadata once per Request. I wonder however if this shouldn't > > > be a WARN as it means the pipeline handler is doing something > > > unexpected. > > > > True that a WARN is an option. > > I would prefer the current approach though, otherwise I need to > > implement the logic in Android Adapter instead. > > > > Please let me know if anyone insists on the other approach. > > > > > Also, the expectations that a metadata is returned once > > > per Request has to be documented in the documentation of > > > PipelineHandler::completeMetadata() > > > > I'll update the doc in the next version :) > > > > BR, > > Harvey > > > > > > > > Thanks > > > j > > > > > > > > > > > BR, > > > > Harvey > > > > > > > > > > > + resultMetadata.set(id, value); > > > > > > + } > > > > > > + > > > > > > + return resultMetadata; > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * \brief Insert a text representation of a Request into an output stream > > > > > > * \param[in] out The output stream > > > > > > -- > > > > > > 2.46.0.598.g6f2099f65c-goog > > > > > >
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 94cee7bd..08c5c58c 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -122,6 +122,7 @@ public: const std::string &id() const; + Signal<Request *, const ControlList &> metadataCompleted; Signal<Request *, FrameBuffer *> bufferCompleted; Signal<Request *> requestCompleted; Signal<> disconnected; diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 0d380803..1af63a23 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -58,6 +58,7 @@ public: void registerRequest(Request *request); void queueRequest(Request *request); + void completeMetadata(Request *request, const ControlList &metadata); bool completeBuffer(Request *request, FrameBuffer *buffer); void completeRequest(Request *request); diff --git a/include/libcamera/request.h b/include/libcamera/request.h index e214a9d1..0200f4a1 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -12,6 +12,7 @@ #include <ostream> #include <stdint.h> #include <string> +#include <unordered_set> #include <libcamera/base/class.h> #include <libcamera/base/signal.h> @@ -64,6 +65,8 @@ public: std::string toString() const; + ControlList addCompletedMetadata(const ControlList &metadata); + private: LIBCAMERA_DISABLE_COPY(Request) @@ -73,6 +76,8 @@ private: const uint64_t cookie_; Status status_; + + std::unordered_set<unsigned int> completedMetadata_; }; std::ostream &operator<<(std::ostream &out, const Request &r); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index a86f552a..5ffae23e 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -892,6 +892,12 @@ const std::string &Camera::id() const * completed */ +/** + * \var Camera::metadataCompleted + * \brief Signal emitted when some metadata for a request is completed as a + * partial result + */ + /** * \var Camera::requestCompleted * \brief Signal emitted when a request queued to the camera has completed diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index e5940469..5d2999cb 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer) return request->_d()->completeBuffer(buffer); } +/** + * \brief Complete part of metadata for a request + * \param[in] request The request the buffer belongs to + * \param[in] metadata The partial metadata that has completed + * + * This function could be called by pipeline handlers to signal completion of + * the \a metadata part of the \a request. It notifies applications of metadata + * completion. + * + * \context This function shall be called from the CameraManager thread. + */ +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata) +{ + const ControlList &validMetadata = request->addCompletedMetadata(metadata); + if (!validMetadata.empty()) { + request->metadata().merge(validMetadata); + + Camera *camera = request->_d()->camera(); + camera->metadataCompleted.emit(request, validMetadata); + } +} + /** * \brief Signal request completion * \param[in] request The request that has completed @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request) Camera::Private *data = camera->_d(); + /* + * Collect metadata which is not yet completed by the Camera, and + * create one partial result to cover the missing metadata before + * completing the whole request. This guarantees the aggregation of + * metadata in completed partial results equals to the global metadata + * in the request. + * + * \todo: Forbid merging metadata into request.metadata() directly and + * force calling completeMetadata() to report metadata. + */ + const ControlList &validMetadata = request->addCompletedMetadata( + request->metadata()); + if (!validMetadata.empty()) + camera->metadataCompleted.emit(request, validMetadata); + while (!data->queuedRequests_.empty()) { Request *req = data->queuedRequests_.front(); if (req->status() == Request::RequestPending) diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 8c56ed30..3ce23a8e 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -598,6 +598,26 @@ std::string Request::toString() const return ss.str(); } +/** + * \brief Add completed metadata, as a partial result + * \param[in] metadata The metadata completed + * + * Request will record the entries that has been sent to the application, to + * prevent duplicated controls. + * + * \return ControlList that hasn't been completed before + */ +ControlList Request::addCompletedMetadata(const ControlList &metadata) +{ + ControlList resultMetadata; + for (auto &[id, value] : metadata) { + if (!completedMetadata_.count(id)) + resultMetadata.set(id, value); + } + + return resultMetadata; +} + /** * \brief Insert a text representation of a Request into an output stream * \param[in] out The output stream