[libcamera-devel] FrameBuffer: Make FrameBuffer::cancel private
diff mbox series

Message ID 20211119150421.974848-1-dorota.czaplejewicz@puri.sm
State Accepted
Headers show
Series
  • [libcamera-devel] FrameBuffer: Make FrameBuffer::cancel private
Related show

Commit Message

Dorota Czaplejewicz Nov. 19, 2021, 3:05 p.m. UTC 
FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel .
---
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(-)

Comments

Laurent Pinchart Nov. 19, 2021, 9:13 p.m. UTC | #1 
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.

The patch looks good overall. With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

You can include that line in your v2.

> + */
> +
>  /**
>   * \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);
>  	}
>
Dorota Czaplejewicz Nov. 19, 2021, 10:33 p.m. UTC | #2 
Hi,

On Fri, 19 Nov 2021 23:13:41 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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.

--Dorota

> The patch looks good overall. With these small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> You can include that line in your v2.
> 
> > + */
> > +
> >  /**
> >   * \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);
> >  	}
> >    
>
Laurent Pinchart Nov. 19, 2021, 10:37 p.m. UTC | #3 
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@ideasonboard.com>
> > 
> > You can include that line in your v2.
> > 
> > > + */
> > > +
> > >  /**
> > >   * \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);
> > >  	}
> > >
Kieran Bingham Nov. 22, 2021, 11:14 a.m. UTC | #4 
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@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@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
Kieran Bingham Dec. 22, 2021, 12:05 p.m. UTC | #5 
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).
> > > > 
> > > > > ---
> > > > > 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@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@ideasonboard.com>

Do you plan to send a revised version of this patch, or would you like
me to handle it for you?

--
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
Dorota Czaplejewicz Dec. 22, 2021, 12:16 p.m. UTC | #6 
On Wed, 22 Dec 2021 12:05:20 +0000
Kieran Bingham <kieran.bingham@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).
> > > > >   
> > > > > > ---
> > > > > > 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@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@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
Kieran Bingham April 13, 2022, 9:17 a.m. UTC | #7 
Hi Dorota,

Quoting Dorota Czaplejewicz (2021-12-22 12:16:29)
> On Wed, 22 Dec 2021 12:05:20 +0000
> Kieran Bingham <kieran.bingham@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@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@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  
>
Dorota Czaplejewicz April 13, 2022, 12:23 p.m. UTC | #8 
Hi,

Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>

Did I do that right?
--Dorota

On Wed, 13 Apr 2022 10:17:31 +0100
Kieran Bingham <kieran.bingham@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@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@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@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    
> >
Kieran Bingham April 13, 2022, 12:56 p.m. UTC | #9 
Quoting Dorota Czaplejewicz (2022-04-13 13:23:57)
> Hi,
> 
> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> 
> Did I do that right?

Perfectly, thanks.

> --Dorota
> 
> On Wed, 13 Apr 2022 10:17:31 +0100
> Kieran Bingham <kieran.bingham@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@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@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@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    
> > >  
>

Patch
diff mbox series 

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; }
+
 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.
+ */
+
 /**
  * \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);
 	}