[libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel private
Dorota Czaplejewicz
dorota.czaplejewicz at puri.sm
Wed Apr 13 14:23:57 CEST 2022
Hi,
Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz at puri.sm>
Did I do that right?
--Dorota
On Wed, 13 Apr 2022 10:17:31 +0100
Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
> Hi Dorota,
>
> Quoting Dorota Czaplejewicz (2021-12-22 12:16:29)
> > On Wed, 22 Dec 2021 12:05:20 +0000
> > Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
> >
> > > Hi Dorota,
> > >
> > > Quoting Kieran Bingham (2021-11-22 11:14:27)
> > > > Hi Dorota,
> > > >
> > > > Quoting Laurent Pinchart (2021-11-19 22:37:30)
> > > > > Hi Dorota,
> > > > >
> > > > > On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote:
> > > > > > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:
> > > > > >
> > > > > > > Hi Dorota,
> > > > > > >
> > > > > > > Thank you for the patch.
> > > > > > >
> > > > > > > The subject line should be
> > > > > > >
> > > > > > > libcamera: framebuffer: Make FrameBuffer::cancel() private
> > > > > > >
> > > > > > > to match the usual style (running `git log` on the file(s) you change is
> > > > > > > a good way to see what the style is).
> > > > > > >
> > > > > > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote:
> > > > > > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel .
> > > > > > >
> > > > > > > Commit messages should be wrapped to 72 columns (git + vim does that by
> > > > > > > default, I'm not sure about other editors).
> > > > > > >
> > > > > > > I'd write
> > > > > > >
> > > > > > > FrameBuffer::cancel() is not meant to be used by applications. Move it
> > > > > > > to the FrameBuffer::Private class.
> > > > > > >
> > > > > > > as "API consumer" could be understood as either the application, or the
> > > > > > > internal API users.
> > > > > > >
> > > > > > > Your Signed-off-by line is missing (see
> > > > > > > https://libcamera.org/contributing.html#submitting-patches).
>
> I was hoping to clean this up and merge it, but to retain your
> authorship I need to add your Signed-off by tag. Could you reply with it
> to this email to signify adding it to the patch please?
>
> --
> Kieran
>
>
> > > > > > >
> > > > > > > > ---
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I'm sending this as discussed on the mailing list.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Dorota
> > > > > > > >
> > > > > > > > include/libcamera/framebuffer.h | 2 --
> > > > > > > > include/libcamera/internal/framebuffer.h | 2 ++
> > > > > > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------
> > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--
> > > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-
> > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-
> > > > > > > > src/libcamera/request.cpp | 2 +-
> > > > > > > > 7 files changed, 17 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > > > > > > index 7f2f176a..367a8a71 100644
> > > > > > > > --- a/include/libcamera/framebuffer.h
> > > > > > > > +++ b/include/libcamera/framebuffer.h
> > > > > > > > @@ -66,8 +66,6 @@ public:
> > > > > > > > unsigned int cookie() const { return cookie_; }
> > > > > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }
> > > > > > > >
> > > > > > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }
> > > > > > > > -
> > > > > > > > private:
> > > > > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)
> > > > > > > >
> > > > > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > > > > > > > index cd33c295..27676212 100644
> > > > > > > > --- a/include/libcamera/internal/framebuffer.h
> > > > > > > > +++ b/include/libcamera/internal/framebuffer.h
> > > > > > > > @@ -23,6 +23,8 @@ public:
> > > > > > > > void setRequest(Request *request) { request_ = request; }
> > > > > > > > bool isContiguous() const { return isContiguous_; }
> > > > > > > >
> > > > > > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; }
> > > > > > >
> > > > > > > We try to keep line within a 80 columns soft limit when it doesn't
> > > > > > > otherwise hinder readability, so this could be
> > > > > > >
> > > > > > > void cancel()
> > > > > > > {
> > > > > > > FrameBuffer *const o = LIBCAMERA_O_PTR();
> > > > > > > o->metadata_.status = FrameMetadata::FrameCancelled;
> > > > > > > }
> > > > > > >
> > > > > > > > +
> > > > > > > > private:
> > > > > > > > Request *request_;
> > > > > > > > bool isContiguous_;
> > > > > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > > > > > > index 337ea115..dd344ed8 100644
> > > > > > > > --- a/src/libcamera/framebuffer.cpp
> > > > > > > > +++ b/src/libcamera/framebuffer.cpp
> > > > > > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()
> > > > > > > > * \return True if the planes are stored contiguously in memory, false otherwise
> > > > > > > > */
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * \fn FrameBuffer::Private::cancel()
> > > > > > > > + * \brief Marks the buffer as cancelled
> > > > > > > > + *
> > > > > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to
> > > > > > > > + * indicate that the metadata is invalid.
> > > > > > >
> > > > > > > We've discussed the fact that the documentation isn't very explicit, but
> > > > > > > that should be addressed in a separate patch, not here.
> > > > > >
> > > > > > Why should it go in a separate patch? I only moved the doc because I
> > > > > > got a warning from doxygen.
> > > > >
> > > > > I meant that we should improve the documentation on top. This patch only
> > > > > moves it, which is right. There's nothing to change.
> > > > >
> > > > > > > The patch looks good overall. With these small issues fixed,
> > > > > > >
> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > >
> > > > > > > You can include that line in your v2.
> > > >
> > > > As previously discussed moving cancel() certainly makes it
> > > > clear that it can not be used by external public interfaces.
> > > >
> > > > With the small changes highlighted by Laurent:
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > Do you plan to send a revised version of this patch, or would you like
> > > me to handle it for you?
> > >
> > Oh, sorry, this slipped my attention.
> >
> > I don't mind if you handle it before I get to it.
> >
> > Cheers,
> > Dorota
> >
> > > --
> > > Regards
> > >
> > > Kieran
> > >
> > > >
> > > >
> > > > > > >
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > /**
> > > > > > > > * \class FrameBuffer
> > > > > > > > * \brief Frame buffer data and its associated dynamic metadata
> > > > > > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const
> > > > > > > > * libcamera core never modifies the buffer cookie.
> > > > > > > > */
> > > > > > > >
> > > > > > > > -/**
> > > > > > > > - * \fn FrameBuffer::cancel()
> > > > > > > > - * \brief Marks the buffer as cancelled
> > > > > > > > - *
> > > > > > > > - * If a buffer is not used by a request, it shall be marked as cancelled to
> > > > > > > > - * indicate that the metadata is invalid.
> > > > > > > > - */
> > > > > > > > -
> > > > > > > > } /* namespace libcamera */
> > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > > index c65afdb2..f8516cba 100644
> > > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > > @@ -27,6 +27,7 @@
> > > > > > > > #include "libcamera/internal/camera_sensor.h"
> > > > > > > > #include "libcamera/internal/delayed_controls.h"
> > > > > > > > #include "libcamera/internal/device_enumerator.h"
> > > > > > > > +#include "libcamera/internal/framebuffer.h"
> > > > > > > > #include "libcamera/internal/ipa_manager.h"
> > > > > > > > #include "libcamera/internal/media_device.h"
> > > > > > > > #include "libcamera/internal/pipeline_handler.h"
> > > > > > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()
> > > > > > > >
> > > > > > > > for (auto it : request->buffers()) {
> > > > > > > > FrameBuffer *buffer = it.second;
> > > > > > > > - buffer->cancel();
> > > > > > > > + buffer->_d()->cancel();
> > > > > > > > pipe()->completeBuffer(request, buffer);
> > > > > > > > }
> > > > > > > >
> > > > > > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> > > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > > > > > > > for (auto it : request->buffers()) {
> > > > > > > > FrameBuffer *b = it.second;
> > > > > > > > - b->cancel();
> > > > > > > > + b->_d()->cancel();
> > > > > > > > pipe()->completeBuffer(request, b);
> > > > > > > > }
> > > > > > > >
> > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > > index 5e1f2273..b7cabf5e 100644
> > > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()
> > > > > > > > * request? If not, do so now.
> > > > > > > > */
> > > > > > > > if (buffer->request()) {
> > > > > > > > - buffer->cancel();
> > > > > > > > + buffer->_d()->cancel();
> > > > > > > > pipe()->completeBuffer(request, buffer);
> > > > > > > > }
> > > > > > > > }
> > > > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > > > index e453091d..1ec814d1 100644
> > > > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > > #include "libcamera/internal/camera.h"
> > > > > > > > #include "libcamera/internal/camera_sensor.h"
> > > > > > > > #include "libcamera/internal/device_enumerator.h"
> > > > > > > > +#include "libcamera/internal/framebuffer.h"
> > > > > > > > #include "libcamera/internal/ipa_manager.h"
> > > > > > > > #include "libcamera/internal/media_device.h"
> > > > > > > > #include "libcamera/internal/pipeline_handler.h"
> > > > > > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
> > > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > > > > > > > for (auto it : request->buffers()) {
> > > > > > > > FrameBuffer *b = it.second;
> > > > > > > > - b->cancel();
> > > > > > > > + b->_d()->cancel();
> > > > > > > > pipe->completeBuffer(request, b);
> > > > > > > > }
> > > > > > > >
> > > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > > > > > > index 17fefab7..a4dbf750 100644
> > > > > > > > --- a/src/libcamera/request.cpp
> > > > > > > > +++ b/src/libcamera/request.cpp
> > > > > > > > @@ -304,7 +304,7 @@ void Request::cancel()
> > > > > > > > ASSERT(status_ == RequestPending);
> > > > > > > >
> > > > > > > > for (FrameBuffer *buffer : pending_) {
> > > > > > > > - buffer->cancel();
> > > > > > > > + buffer->_d()->cancel();
> > > > > > > > camera_->bufferCompleted.emit(this, buffer);
> > > > > > > > }
> > > > > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > >
> > > > > Laurent Pinchart
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220413/6c0aa004/attachment-0001.sig>
More information about the libcamera-devel
mailing list