| Message ID | 20250303151634.734176-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Mon, Mar 03, 2025 at 04:16:34PM +0100, Barnabás Pőcze wrote: > Take the unique pointer to the `Fence` object by rvalue reference > so that it is not destroyed if the function returns an error code > and does not take ownership of the unique pointer. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/request.h | 2 +- > src/libcamera/request.cpp | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index e214a9d13..0c5939f7b 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -53,7 +53,7 @@ public: > ControlList &metadata() { return *metadata_; } > const BufferMap &buffers() const { return bufferMap_; } > int addBuffer(const Stream *stream, FrameBuffer *buffer, > - std::unique_ptr<Fence> fence = nullptr); > + std::unique_ptr<Fence> &&fence = {}); There's one caller in src/android/camera_device.cpp that passes nullptr as the third argument. Could you fix that ? The other caller in the same file creates the unique_ptr right before calling the function, so the fence will be destroyed anyway. I suppose that's fine for now. Should the unit test also be extended to validate the new behaviour ? The function documentation should also be updated. > FrameBuffer *findBuffer(const Stream *stream) const; > > uint32_t sequence() const; > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index b206ac132..7d02f09fd 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags) > * \retval -EINVAL The buffer does not reference a valid Stream > */ > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > - std::unique_ptr<Fence> fence) > + std::unique_ptr<Fence> &&fence) > { > if (!stream) { > LOG(Request, Error) << "Invalid stream reference";
Hi 2025. 03. 03. 23:26 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Mon, Mar 03, 2025 at 04:16:34PM +0100, Barnabás Pőcze wrote: >> Take the unique pointer to the `Fence` object by rvalue reference >> so that it is not destroyed if the function returns an error code >> and does not take ownership of the unique pointer. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/request.h | 2 +- >> src/libcamera/request.cpp | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >> index e214a9d13..0c5939f7b 100644 >> --- a/include/libcamera/request.h >> +++ b/include/libcamera/request.h >> @@ -53,7 +53,7 @@ public: >> ControlList &metadata() { return *metadata_; } >> const BufferMap &buffers() const { return bufferMap_; } >> int addBuffer(const Stream *stream, FrameBuffer *buffer, >> - std::unique_ptr<Fence> fence = nullptr); >> + std::unique_ptr<Fence> &&fence = {}); > > There's one caller in src/android/camera_device.cpp that passes nullptr > as the third argument. Could you fix that ? It does not cause any issues, but I'll add a separate commit to change it. > > The other caller in the same file creates the unique_ptr right before > calling the function, so the fence will be destroyed anyway. I suppose > that's fine for now. There is no change in behaviour in case of success, and in case of failure there is minimal difference in most cases. No in-tree users are affected as far as I could tell. > > Should the unit test also be extended to validate the new behaviour ? I can add some extra checks to `test/fence.cpp` if that's fine? > The function documentation should also be updated. ACK Regards, Barnabás Pőcze > >> FrameBuffer *findBuffer(const Stream *stream) const; >> >> uint32_t sequence() const; >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >> index b206ac132..7d02f09fd 100644 >> --- a/src/libcamera/request.cpp >> +++ b/src/libcamera/request.cpp >> @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags) >> * \retval -EINVAL The buffer does not reference a valid Stream >> */ >> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, >> - std::unique_ptr<Fence> fence) >> + std::unique_ptr<Fence> &&fence) >> { >> if (!stream) { >> LOG(Request, Error) << "Invalid stream reference"; >
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index e214a9d13..0c5939f7b 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -53,7 +53,7 @@ public: ControlList &metadata() { return *metadata_; } const BufferMap &buffers() const { return bufferMap_; } int addBuffer(const Stream *stream, FrameBuffer *buffer, - std::unique_ptr<Fence> fence = nullptr); + std::unique_ptr<Fence> &&fence = {}); FrameBuffer *findBuffer(const Stream *stream) const; uint32_t sequence() const; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index b206ac132..7d02f09fd 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags) * \retval -EINVAL The buffer does not reference a valid Stream */ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, - std::unique_ptr<Fence> fence) + std::unique_ptr<Fence> &&fence) { if (!stream) { LOG(Request, Error) << "Invalid stream reference";
Take the unique pointer to the `Fence` object by rvalue reference so that it is not destroyed if the function returns an error code and does not take ownership of the unique pointer. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/request.h | 2 +- src/libcamera/request.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)