| Message ID | 20210420130741.236848-7-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for the patch ! On 20/04/2021 15:07, Kieran Bingham wrote: > Provide an extensible private object for the Request class. > This allows us to make modifications to the private API and storage of > requests without affecting the public API or ABI. > > The D pointer is obtained in all Request functions implemented in the > request.cpp file along with an assertion that the D pointer was valid to > provide extra validation checking that the Request has not been > deleted, while in use as it is 'owned' by the application. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > The assertions added in findBuffer, complete() and completeBuffer() > allow us to ensure that the Request is still valid while asynchronous > actions occur on the Request internally in libcamera, and provide > (almost) equivalent functionality as the Request Canary previously > proposed. > > The additions in reuse() and addBuffer() are called from applications, > so the assertions may be less helpful there, but I've added them for > consistency. > > include/libcamera/request.h | 4 +++- > src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 4cf5ff3f7d3b..909a1aebc2d2 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -24,8 +24,10 @@ class CameraControlValidator; > class FrameBuffer; > class Stream; > > -class Request > +class Request : public Extensible > { > + LIBCAMERA_DECLARE_PRIVATE(Request) > + > public: > enum Status { > RequestPending, > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index ce2dd7b17f10..977bfac4fce6 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -28,6 +28,19 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(Request) > > +class Request::Private : public Extensible::Private > +{ > + LIBCAMERA_DECLARE_PUBLIC(Request) > + > +public: > + Private(Request *request); > +}; > + > +Request::Private::Private(Request *request) > + : Extensible::Private(request) > +{ > +} > + > /** > * \enum Request::Status > * Request completion status > @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) > * > */ > Request::Request(Camera *camera, uint64_t cookie) > - : camera_(camera), sequence_(0), cookie_(cookie), > - status_(RequestPending), cancelled_(false) > + : Extensible(new Private(this)), camera_(camera), sequence_(0), > + cookie_(cookie), status_(RequestPending), cancelled_(false) > { > /** > * \todo Should the Camera expose a validator instance, to avoid > @@ -114,6 +127,9 @@ Request::~Request() > */ > void Request::reuse(ReuseFlag flags) > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > LIBCAMERA_TRACEPOINT(request_reuse, this); > > pending_.clear(); > @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) > */ > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > if (!stream) { > LOG(Request, Error) << "Invalid stream reference"; > return -EINVAL; > @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > */ > FrameBuffer *Request::findBuffer(const Stream *stream) const > { > + const Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > const auto it = bufferMap_.find(stream); > if (it == bufferMap_.end()) > return nullptr; > @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > */ > void Request::complete() > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > ASSERT(status_ == RequestPending); > ASSERT(!hasPendingBuffers()); > > @@ -307,6 +331,9 @@ void Request::complete() > */ > bool Request::completeBuffer(FrameBuffer *buffer) > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); > > int ret = pending_.erase(buffer); > @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) > */ > std::string Request::toString() const > { > + const Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > std::stringstream ss; > > /* Pending, Completed, Cancelled(X). */ >
Hi Kieran, Thank you for the patch. On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote: > Provide an extensible private object for the Request class. > This allows us to make modifications to the private API and storage of > requests without affecting the public API or ABI. > > The D pointer is obtained in all Request functions implemented in the > request.cpp file along with an assertion that the D pointer was valid to > provide extra validation checking that the Request has not been > deleted, while in use as it is 'owned' by the application. s/, while in use/ while in use,/ > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > The assertions added in findBuffer, complete() and completeBuffer() > allow us to ensure that the Request is still valid while asynchronous > actions occur on the Request internally in libcamera, and provide > (almost) equivalent functionality as the Request Canary previously > proposed. Does std::unique_ptr<> guarantees that it will reset its internal pointer when deleted ? libc++ calls reset() in the destructor, and stdlibc++ seems to set the pointer to nullptr manually, but that doesn't seem to be guaranteed by the C++ standard. > The additions in reuse() and addBuffer() are called from applications, > so the assertions may be less helpful there, but I've added them for > consistency. > > include/libcamera/request.h | 4 +++- > src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 4cf5ff3f7d3b..909a1aebc2d2 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -24,8 +24,10 @@ class CameraControlValidator; > class FrameBuffer; > class Stream; > > -class Request > +class Request : public Extensible > { > + LIBCAMERA_DECLARE_PRIVATE(Request) > + > public: > enum Status { > RequestPending, > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index ce2dd7b17f10..977bfac4fce6 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -28,6 +28,19 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(Request) > > +class Request::Private : public Extensible::Private > +{ > + LIBCAMERA_DECLARE_PUBLIC(Request) > + > +public: > + Private(Request *request); > +}; > + > +Request::Private::Private(Request *request) > + : Extensible::Private(request) > +{ > +} > + > /** > * \enum Request::Status > * Request completion status > @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) > * > */ > Request::Request(Camera *camera, uint64_t cookie) > - : camera_(camera), sequence_(0), cookie_(cookie), > - status_(RequestPending), cancelled_(false) > + : Extensible(new Private(this)), camera_(camera), sequence_(0), > + cookie_(cookie), status_(RequestPending), cancelled_(false) Should we move some of the data to Private (in a subsequent patch) ? As an exercise, how about moving the member data that we think will be subject to change when we'll rework the request completion API, and see if we manage to complete that rework without breaking the API of the request class ? A subsequent patch should also move the public functions that are not called by applications to the Private class. > { > /** > * \todo Should the Camera expose a validator instance, to avoid > @@ -114,6 +127,9 @@ Request::~Request() > */ > void Request::reuse(ReuseFlag flags) > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > LIBCAMERA_TRACEPOINT(request_reuse, this); > > pending_.clear(); > @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) > */ > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > if (!stream) { > LOG(Request, Error) << "Invalid stream reference"; > return -EINVAL; > @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > */ > FrameBuffer *Request::findBuffer(const Stream *stream) const > { > + const Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > const auto it = bufferMap_.find(stream); > if (it == bufferMap_.end()) > return nullptr; > @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > */ > void Request::complete() > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > ASSERT(status_ == RequestPending); > ASSERT(!hasPendingBuffers()); > > @@ -307,6 +331,9 @@ void Request::complete() > */ > bool Request::completeBuffer(FrameBuffer *buffer) > { > + Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); > > int ret = pending_.erase(buffer); > @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) > */ > std::string Request::toString() const > { > + const Private *const d = LIBCAMERA_D_PTR(); > + ASSERT(d); > + > std::stringstream ss; > > /* Pending, Completed, Cancelled(X). */
On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > Thank you for the patch. > > On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote: > > Provide an extensible private object for the Request class. > > This allows us to make modifications to the private API and storage of > > requests without affecting the public API or ABI. > > > > The D pointer is obtained in all Request functions implemented in the > > request.cpp file along with an assertion that the D pointer was valid to > > provide extra validation checking that the Request has not been > > deleted, while in use as it is 'owned' by the application. > > s/, while in use/ while in use,/ > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > The assertions added in findBuffer, complete() and completeBuffer() > > allow us to ensure that the Request is still valid while asynchronous > > actions occur on the Request internally in libcamera, and provide > > (almost) equivalent functionality as the Request Canary previously > > proposed. > > Does std::unique_ptr<> guarantees that it will reset its internal > pointer when deleted ? libc++ calls reset() in the destructor, and > stdlibc++ seems to set the pointer to nullptr manually, but that doesn't > seem to be guaranteed by the C++ standard. > I wonder if this is worth doing. When d_ is invalid, Request itself is also invalid. Regardless of unique_ptr implementation, calling a function of a deleted class (Request here) causes an intended behavior. I would not add these ASSERTs. -Hiro > > The additions in reuse() and addBuffer() are called from applications, > > so the assertions may be less helpful there, but I've added them for > > consistency. > > > > include/libcamera/request.h | 4 +++- > > src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- > > 2 files changed, 35 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index 4cf5ff3f7d3b..909a1aebc2d2 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -24,8 +24,10 @@ class CameraControlValidator; > > class FrameBuffer; > > class Stream; > > > > -class Request > > +class Request : public Extensible > > { > > + LIBCAMERA_DECLARE_PRIVATE(Request) > > + > > public: > > enum Status { > > RequestPending, > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index ce2dd7b17f10..977bfac4fce6 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -28,6 +28,19 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(Request) > > > > +class Request::Private : public Extensible::Private > > +{ > > + LIBCAMERA_DECLARE_PUBLIC(Request) > > + > > +public: > > + Private(Request *request); > > +}; > > + > > +Request::Private::Private(Request *request) > > + : Extensible::Private(request) > > +{ > > +} > > + > > /** > > * \enum Request::Status > > * Request completion status > > @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) > > * > > */ > > Request::Request(Camera *camera, uint64_t cookie) > > - : camera_(camera), sequence_(0), cookie_(cookie), > > - status_(RequestPending), cancelled_(false) > > + : Extensible(new Private(this)), camera_(camera), sequence_(0), > > + cookie_(cookie), status_(RequestPending), cancelled_(false) > > Should we move some of the data to Private (in a subsequent patch) ? As > an exercise, how about moving the member data that we think will be > subject to change when we'll rework the request completion API, and see > if we manage to complete that rework without breaking the API of the > request class ? > > A subsequent patch should also move the public functions that are not > called by applications to the Private class. > > > { > > /** > > * \todo Should the Camera expose a validator instance, to avoid > > @@ -114,6 +127,9 @@ Request::~Request() > > */ > > void Request::reuse(ReuseFlag flags) > > { > > + Private *const d = LIBCAMERA_D_PTR(); > > + ASSERT(d); > > + > > LIBCAMERA_TRACEPOINT(request_reuse, this); > > > > pending_.clear(); > > @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) > > */ > > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > > { > > + Private *const d = LIBCAMERA_D_PTR(); > > + ASSERT(d); > > + > > if (!stream) { > > LOG(Request, Error) << "Invalid stream reference"; > > return -EINVAL; > > @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > > */ > > FrameBuffer *Request::findBuffer(const Stream *stream) const > > { > > + const Private *const d = LIBCAMERA_D_PTR(); > > + ASSERT(d); > > + > > const auto it = bufferMap_.find(stream); > > if (it == bufferMap_.end()) > > return nullptr; > > @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > > */ > > void Request::complete() > > { > > + Private *const d = LIBCAMERA_D_PTR(); > > + ASSERT(d); > > ASSERT(status_ == RequestPending); > > ASSERT(!hasPendingBuffers()); > > > > @@ -307,6 +331,9 @@ void Request::complete() > > */ > > bool Request::completeBuffer(FrameBuffer *buffer) > > { > > + Private *const d = LIBCAMERA_D_PTR(); > > + ASSERT(d); > > + > > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); > > > > int ret = pending_.erase(buffer); > > @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) > > */ > > std::string Request::toString() const > > { > > + const Private *const d = LIBCAMERA_D_PTR(); > > + ASSERT(d); > > + > > std::stringstream ss; > > > > /* Pending, Completed, Cancelled(X). */ > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On Wed, Apr 21, 2021 at 03:40:36PM +0900, Hirokazu Honda wrote: > On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart wrote: > > On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote: > > > Provide an extensible private object for the Request class. > > > This allows us to make modifications to the private API and storage of > > > requests without affecting the public API or ABI. > > > > > > The D pointer is obtained in all Request functions implemented in the > > > request.cpp file along with an assertion that the D pointer was valid to > > > provide extra validation checking that the Request has not been > > > deleted, while in use as it is 'owned' by the application. > > > > s/, while in use/ while in use,/ > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > --- > > > The assertions added in findBuffer, complete() and completeBuffer() > > > allow us to ensure that the Request is still valid while asynchronous > > > actions occur on the Request internally in libcamera, and provide > > > (almost) equivalent functionality as the Request Canary previously > > > proposed. > > > > Does std::unique_ptr<> guarantees that it will reset its internal > > pointer when deleted ? libc++ calls reset() in the destructor, and > > stdlibc++ seems to set the pointer to nullptr manually, but that doesn't > > seem to be guaranteed by the C++ standard. > > I wonder if this is worth doing. When d_ is invalid, Request itself is > also invalid. > Regardless of unique_ptr implementation, calling a function of a > deleted class (Request here) causes an intended behavior. > I would not add these ASSERTs. It's definitely an invalid behaviour, and shouldn't happen. The purpose of the ASSERT() is to ensure it will be caught right away, instead of running incorrectly and possibly corrupting memory that will lead to a crash later. Delayed crashes are very annoying to debug. > > > The additions in reuse() and addBuffer() are called from applications, > > > so the assertions may be less helpful there, but I've added them for > > > consistency. > > > > > > include/libcamera/request.h | 4 +++- > > > src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- > > > 2 files changed, 35 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > index 4cf5ff3f7d3b..909a1aebc2d2 100644 > > > --- a/include/libcamera/request.h > > > +++ b/include/libcamera/request.h > > > @@ -24,8 +24,10 @@ class CameraControlValidator; > > > class FrameBuffer; > > > class Stream; > > > > > > -class Request > > > +class Request : public Extensible > > > { > > > + LIBCAMERA_DECLARE_PRIVATE(Request) > > > + > > > public: > > > enum Status { > > > RequestPending, > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index ce2dd7b17f10..977bfac4fce6 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -28,6 +28,19 @@ namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(Request) > > > > > > +class Request::Private : public Extensible::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(Request) > > > + > > > +public: > > > + Private(Request *request); > > > +}; > > > + > > > +Request::Private::Private(Request *request) > > > + : Extensible::Private(request) > > > +{ > > > +} > > > + > > > /** > > > * \enum Request::Status > > > * Request completion status > > > @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) > > > * > > > */ > > > Request::Request(Camera *camera, uint64_t cookie) > > > - : camera_(camera), sequence_(0), cookie_(cookie), > > > - status_(RequestPending), cancelled_(false) > > > + : Extensible(new Private(this)), camera_(camera), sequence_(0), > > > + cookie_(cookie), status_(RequestPending), cancelled_(false) > > > > Should we move some of the data to Private (in a subsequent patch) ? As > > an exercise, how about moving the member data that we think will be > > subject to change when we'll rework the request completion API, and see > > if we manage to complete that rework without breaking the API of the > > request class ? > > > > A subsequent patch should also move the public functions that are not > > called by applications to the Private class. > > > > > { > > > /** > > > * \todo Should the Camera expose a validator instance, to avoid > > > @@ -114,6 +127,9 @@ Request::~Request() > > > */ > > > void Request::reuse(ReuseFlag flags) > > > { > > > + Private *const d = LIBCAMERA_D_PTR(); > > > + ASSERT(d); > > > + > > > LIBCAMERA_TRACEPOINT(request_reuse, this); > > > > > > pending_.clear(); > > > @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) > > > */ > > > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > > > { > > > + Private *const d = LIBCAMERA_D_PTR(); > > > + ASSERT(d); > > > + > > > if (!stream) { > > > LOG(Request, Error) << "Invalid stream reference"; > > > return -EINVAL; > > > @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) > > > */ > > > FrameBuffer *Request::findBuffer(const Stream *stream) const > > > { > > > + const Private *const d = LIBCAMERA_D_PTR(); > > > + ASSERT(d); > > > + > > > const auto it = bufferMap_.find(stream); > > > if (it == bufferMap_.end()) > > > return nullptr; > > > @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > > > */ > > > void Request::complete() > > > { > > > + Private *const d = LIBCAMERA_D_PTR(); > > > + ASSERT(d); > > > ASSERT(status_ == RequestPending); > > > ASSERT(!hasPendingBuffers()); > > > > > > @@ -307,6 +331,9 @@ void Request::complete() > > > */ > > > bool Request::completeBuffer(FrameBuffer *buffer) > > > { > > > + Private *const d = LIBCAMERA_D_PTR(); > > > + ASSERT(d); > > > + > > > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); > > > > > > int ret = pending_.erase(buffer); > > > @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) > > > */ > > > std::string Request::toString() const > > > { > > > + const Private *const d = LIBCAMERA_D_PTR(); > > > + ASSERT(d); > > > + > > > std::stringstream ss; > > > > > > /* Pending, Completed, Cancelled(X). */
Hi Laurent, On 20/04/2021 23:18, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote: >> Provide an extensible private object for the Request class. >> This allows us to make modifications to the private API and storage of >> requests without affecting the public API or ABI. >> >> The D pointer is obtained in all Request functions implemented in the >> request.cpp file along with an assertion that the D pointer was valid to >> provide extra validation checking that the Request has not been >> deleted, while in use as it is 'owned' by the application. > > s/, while in use/ while in use,/ > >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> The assertions added in findBuffer, complete() and completeBuffer() >> allow us to ensure that the Request is still valid while asynchronous >> actions occur on the Request internally in libcamera, and provide >> (almost) equivalent functionality as the Request Canary previously >> proposed. > > Does std::unique_ptr<> guarantees that it will reset its internal > pointer when deleted ? libc++ calls reset() in the destructor, and > stdlibc++ seems to set the pointer to nullptr manually, but that doesn't > seem to be guaranteed by the C++ standard. I've manually tested that it gets reset, but that doesn't offer any guarantee beyond that. >> The additions in reuse() and addBuffer() are called from applications, >> so the assertions may be less helpful there, but I've added them for >> consistency. >> >> include/libcamera/request.h | 4 +++- >> src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- >> 2 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >> index 4cf5ff3f7d3b..909a1aebc2d2 100644 >> --- a/include/libcamera/request.h >> +++ b/include/libcamera/request.h >> @@ -24,8 +24,10 @@ class CameraControlValidator; >> class FrameBuffer; >> class Stream; >> >> -class Request >> +class Request : public Extensible >> { >> + LIBCAMERA_DECLARE_PRIVATE(Request) >> + >> public: >> enum Status { >> RequestPending, >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >> index ce2dd7b17f10..977bfac4fce6 100644 >> --- a/src/libcamera/request.cpp >> +++ b/src/libcamera/request.cpp >> @@ -28,6 +28,19 @@ namespace libcamera { >> >> LOG_DEFINE_CATEGORY(Request) >> >> +class Request::Private : public Extensible::Private >> +{ >> + LIBCAMERA_DECLARE_PUBLIC(Request) >> + >> +public: >> + Private(Request *request); >> +}; >> + >> +Request::Private::Private(Request *request) >> + : Extensible::Private(request) >> +{ >> +} >> + >> /** >> * \enum Request::Status >> * Request completion status >> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) >> * >> */ >> Request::Request(Camera *camera, uint64_t cookie) >> - : camera_(camera), sequence_(0), cookie_(cookie), >> - status_(RequestPending), cancelled_(false) >> + : Extensible(new Private(this)), camera_(camera), sequence_(0), >> + cookie_(cookie), status_(RequestPending), cancelled_(false) > > Should we move some of the data to Private (in a subsequent patch) ? As > an exercise, how about moving the member data that we think will be > subject to change when we'll rework the request completion API, and see > if we manage to complete that rework without breaking the API of the > request class ? Of course, the goal of this patch isn't just to add assertions, it's to make it extensible so we can extend it in subsequent patches ;-) > A subsequent patch should also move the public functions that are not > called by applications to the Private class. Agreed, I wanted to keep this patch as simple as possible to start with. > >> { >> /** >> * \todo Should the Camera expose a validator instance, to avoid >> @@ -114,6 +127,9 @@ Request::~Request() >> */ >> void Request::reuse(ReuseFlag flags) >> { >> + Private *const d = LIBCAMERA_D_PTR(); >> + ASSERT(d); >> + >> LIBCAMERA_TRACEPOINT(request_reuse, this); >> >> pending_.clear(); >> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) >> */ >> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) >> { >> + Private *const d = LIBCAMERA_D_PTR(); >> + ASSERT(d); >> + >> if (!stream) { >> LOG(Request, Error) << "Invalid stream reference"; >> return -EINVAL; >> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) >> */ >> FrameBuffer *Request::findBuffer(const Stream *stream) const >> { >> + const Private *const d = LIBCAMERA_D_PTR(); >> + ASSERT(d); >> + >> const auto it = bufferMap_.find(stream); >> if (it == bufferMap_.end()) >> return nullptr; >> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const >> */ >> void Request::complete() >> { >> + Private *const d = LIBCAMERA_D_PTR(); >> + ASSERT(d); >> ASSERT(status_ == RequestPending); >> ASSERT(!hasPendingBuffers()); >> >> @@ -307,6 +331,9 @@ void Request::complete() >> */ >> bool Request::completeBuffer(FrameBuffer *buffer) >> { >> + Private *const d = LIBCAMERA_D_PTR(); >> + ASSERT(d); >> + >> LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); >> >> int ret = pending_.erase(buffer); >> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) >> */ >> std::string Request::toString() const >> { >> + const Private *const d = LIBCAMERA_D_PTR(); >> + ASSERT(d); >> + >> std::stringstream ss; >> >> /* Pending, Completed, Cancelled(X). */ >
On 21/04/2021 10:01, Laurent Pinchart wrote: > Hi Hiro, > > On Wed, Apr 21, 2021 at 03:40:36PM +0900, Hirokazu Honda wrote: >> On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart wrote: >>> On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote: >>>> Provide an extensible private object for the Request class. >>>> This allows us to make modifications to the private API and storage of >>>> requests without affecting the public API or ABI. >>>> >>>> The D pointer is obtained in all Request functions implemented in the >>>> request.cpp file along with an assertion that the D pointer was valid to >>>> provide extra validation checking that the Request has not been >>>> deleted, while in use as it is 'owned' by the application. >>> >>> s/, while in use/ while in use,/ >>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> >>>> --- >>>> The assertions added in findBuffer, complete() and completeBuffer() >>>> allow us to ensure that the Request is still valid while asynchronous >>>> actions occur on the Request internally in libcamera, and provide >>>> (almost) equivalent functionality as the Request Canary previously >>>> proposed. >>> >>> Does std::unique_ptr<> guarantees that it will reset its internal >>> pointer when deleted ? libc++ calls reset() in the destructor, and >>> stdlibc++ seems to set the pointer to nullptr manually, but that doesn't >>> seem to be guaranteed by the C++ standard. >> >> I wonder if this is worth doing. When d_ is invalid, Request itself is >> also invalid. >> Regardless of unique_ptr implementation, calling a function of a >> deleted class (Request here) causes an intended behavior. >> I would not add these ASSERTs. > > It's definitely an invalid behaviour, and shouldn't happen. The purpose > of the ASSERT() is to ensure it will be caught right away, instead of > running incorrectly and possibly corrupting memory that will lead to a > crash later. Delayed crashes are very annoying to debug. Exactly that. I spent quite some time debugging IPU3 issues and one of the rabbit holes was that we were sending 'completed' Requests back to the application before they were really completed. This meant that the application could free the request while it was still in use by libcamera (and thus crashed in arbitrary distant places). Now granted, I've (in this series) added some extra protection to try to catch that early to make sure when we stop all requests are guaranteed to be handed back, but there's still the scope that the application is in control of and expected to delete requests. If anything happens on a request after it has been deleted - I want to make sure we fire an assertion as quickly as possible, declaring that. That's what led to the 'REQUEST_CANARY', which is now (almost equivalently, but not exactly) handled by this. >>>> The additions in reuse() and addBuffer() are called from applications, >>>> so the assertions may be less helpful there, but I've added them for >>>> consistency. >>>> >>>> include/libcamera/request.h | 4 +++- >>>> src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- >>>> 2 files changed, 35 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>>> index 4cf5ff3f7d3b..909a1aebc2d2 100644 >>>> --- a/include/libcamera/request.h >>>> +++ b/include/libcamera/request.h >>>> @@ -24,8 +24,10 @@ class CameraControlValidator; >>>> class FrameBuffer; >>>> class Stream; >>>> >>>> -class Request >>>> +class Request : public Extensible >>>> { >>>> + LIBCAMERA_DECLARE_PRIVATE(Request) >>>> + >>>> public: >>>> enum Status { >>>> RequestPending, >>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >>>> index ce2dd7b17f10..977bfac4fce6 100644 >>>> --- a/src/libcamera/request.cpp >>>> +++ b/src/libcamera/request.cpp >>>> @@ -28,6 +28,19 @@ namespace libcamera { >>>> >>>> LOG_DEFINE_CATEGORY(Request) >>>> >>>> +class Request::Private : public Extensible::Private >>>> +{ >>>> + LIBCAMERA_DECLARE_PUBLIC(Request) >>>> + >>>> +public: >>>> + Private(Request *request); >>>> +}; >>>> + >>>> +Request::Private::Private(Request *request) >>>> + : Extensible::Private(request) >>>> +{ >>>> +} >>>> + >>>> /** >>>> * \enum Request::Status >>>> * Request completion status >>>> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) >>>> * >>>> */ >>>> Request::Request(Camera *camera, uint64_t cookie) >>>> - : camera_(camera), sequence_(0), cookie_(cookie), >>>> - status_(RequestPending), cancelled_(false) >>>> + : Extensible(new Private(this)), camera_(camera), sequence_(0), >>>> + cookie_(cookie), status_(RequestPending), cancelled_(false) >>> >>> Should we move some of the data to Private (in a subsequent patch) ? As >>> an exercise, how about moving the member data that we think will be >>> subject to change when we'll rework the request completion API, and see >>> if we manage to complete that rework without breaking the API of the >>> request class ? >>> >>> A subsequent patch should also move the public functions that are not >>> called by applications to the Private class. >>> >>>> { >>>> /** >>>> * \todo Should the Camera expose a validator instance, to avoid >>>> @@ -114,6 +127,9 @@ Request::~Request() >>>> */ >>>> void Request::reuse(ReuseFlag flags) >>>> { >>>> + Private *const d = LIBCAMERA_D_PTR(); >>>> + ASSERT(d); >>>> + >>>> LIBCAMERA_TRACEPOINT(request_reuse, this); >>>> >>>> pending_.clear(); >>>> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) >>>> */ >>>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) >>>> { >>>> + Private *const d = LIBCAMERA_D_PTR(); >>>> + ASSERT(d); >>>> + >>>> if (!stream) { >>>> LOG(Request, Error) << "Invalid stream reference"; >>>> return -EINVAL; >>>> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) >>>> */ >>>> FrameBuffer *Request::findBuffer(const Stream *stream) const >>>> { >>>> + const Private *const d = LIBCAMERA_D_PTR(); >>>> + ASSERT(d); >>>> + >>>> const auto it = bufferMap_.find(stream); >>>> if (it == bufferMap_.end()) >>>> return nullptr; >>>> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const >>>> */ >>>> void Request::complete() >>>> { >>>> + Private *const d = LIBCAMERA_D_PTR(); >>>> + ASSERT(d); >>>> ASSERT(status_ == RequestPending); >>>> ASSERT(!hasPendingBuffers()); >>>> >>>> @@ -307,6 +331,9 @@ void Request::complete() >>>> */ >>>> bool Request::completeBuffer(FrameBuffer *buffer) >>>> { >>>> + Private *const d = LIBCAMERA_D_PTR(); >>>> + ASSERT(d); >>>> + >>>> LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); >>>> >>>> int ret = pending_.erase(buffer); >>>> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) >>>> */ >>>> std::string Request::toString() const >>>> { >>>> + const Private *const d = LIBCAMERA_D_PTR(); >>>> + ASSERT(d); >>>> + >>>> std::stringstream ss; >>>> >>>> /* Pending, Completed, Cancelled(X). */ >
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 4cf5ff3f7d3b..909a1aebc2d2 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -24,8 +24,10 @@ class CameraControlValidator; class FrameBuffer; class Stream; -class Request +class Request : public Extensible { + LIBCAMERA_DECLARE_PRIVATE(Request) + public: enum Status { RequestPending, diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index ce2dd7b17f10..977bfac4fce6 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -28,6 +28,19 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Request) +class Request::Private : public Extensible::Private +{ + LIBCAMERA_DECLARE_PUBLIC(Request) + +public: + Private(Request *request); +}; + +Request::Private::Private(Request *request) + : Extensible::Private(request) +{ +} + /** * \enum Request::Status * Request completion status @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request) * */ Request::Request(Camera *camera, uint64_t cookie) - : camera_(camera), sequence_(0), cookie_(cookie), - status_(RequestPending), cancelled_(false) + : Extensible(new Private(this)), camera_(camera), sequence_(0), + cookie_(cookie), status_(RequestPending), cancelled_(false) { /** * \todo Should the Camera expose a validator instance, to avoid @@ -114,6 +127,9 @@ Request::~Request() */ void Request::reuse(ReuseFlag flags) { + Private *const d = LIBCAMERA_D_PTR(); + ASSERT(d); + LIBCAMERA_TRACEPOINT(request_reuse, this); pending_.clear(); @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags) */ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) { + Private *const d = LIBCAMERA_D_PTR(); + ASSERT(d); + if (!stream) { LOG(Request, Error) << "Invalid stream reference"; return -EINVAL; @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer) */ FrameBuffer *Request::findBuffer(const Stream *stream) const { + const Private *const d = LIBCAMERA_D_PTR(); + ASSERT(d); + const auto it = bufferMap_.find(stream); if (it == bufferMap_.end()) return nullptr; @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const */ void Request::complete() { + Private *const d = LIBCAMERA_D_PTR(); + ASSERT(d); ASSERT(status_ == RequestPending); ASSERT(!hasPendingBuffers()); @@ -307,6 +331,9 @@ void Request::complete() */ bool Request::completeBuffer(FrameBuffer *buffer) { + Private *const d = LIBCAMERA_D_PTR(); + ASSERT(d); + LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer); int ret = pending_.erase(buffer); @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer) */ std::string Request::toString() const { + const Private *const d = LIBCAMERA_D_PTR(); + ASSERT(d); + std::stringstream ss; /* Pending, Completed, Cancelled(X). */
Provide an extensible private object for the Request class. This allows us to make modifications to the private API and storage of requests without affecting the public API or ABI. The D pointer is obtained in all Request functions implemented in the request.cpp file along with an assertion that the D pointer was valid to provide extra validation checking that the Request has not been deleted, while in use as it is 'owned' by the application. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- The assertions added in findBuffer, complete() and completeBuffer() allow us to ensure that the Request is still valid while asynchronous actions occur on the Request internally in libcamera, and provide (almost) equivalent functionality as the Request Canary previously proposed. The additions in reuse() and addBuffer() are called from applications, so the assertions may be less helpful there, but I've added them for consistency. include/libcamera/request.h | 4 +++- src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-)