[libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel private

[Laurent Pinchart]

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 at 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);
>  	}
>  

-- 
Regards,

Laurent Pinchart