[libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel private
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Nov 22 12:14:27 CET 2021
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).
> > >
> > > > ---
> > > > 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>
> > >
> > > > + */
> > > > +
> > > > /**
> > > > * \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
More information about the libcamera-devel
mailing list