[libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it extensible

[Hirokazu Honda]

On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart
<laurent.pinchart at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel