[libcamera-devel,v3] android: Camera3RequestDescriptor: Provide a constructor for StreamBuffer
diff mbox series

Message ID 20211028170254.502151-1-umang.jain@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v3] android: Camera3RequestDescriptor: Provide a constructor for StreamBuffer
Related show

Commit Message

Umang Jain Oct. 28, 2021, 5:02 p.m. UTC 
Provide a constructor for StreamBuffer and use that while populating
Camera3RequestDescriptor::buffers_ vector. Also provide the default
move-constructor (required as StreamBuffer is stored in a vector in
Camera3RequestDescriptor) and destructor for the StreamBuffer struct.

Also declare a default move assignment operator and disable the
copy constructor and move operator explicitly with
LIBCAMERA_DISABLE_COPY().

While at it, initialize pointers members in the StreamBuffer struct
to nullptr, with StreamBuffer::status set to Status::Success by default.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_request.cpp | 19 ++++++++++++++++---
 src/android/camera_request.h   | 17 ++++++++++++++---
 2 files changed, 30 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart Oct. 28, 2021, 5:38 p.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Thu, Oct 28, 2021 at 10:32:54PM +0530, Umang Jain wrote:
> Provide a constructor for StreamBuffer and use that while populating
> Camera3RequestDescriptor::buffers_ vector. Also provide the default
> move-constructor (required as StreamBuffer is stored in a vector in
> Camera3RequestDescriptor) and destructor for the StreamBuffer struct.
> 
> Also declare a default move assignment operator and disable the
> copy constructor and move operator explicitly with
> LIBCAMERA_DISABLE_COPY().
> 
> While at it, initialize pointers members in the StreamBuffer struct
> to nullptr, with StreamBuffer::status set to Status::Success by default.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_request.cpp | 19 ++++++++++++++++---
>  src/android/camera_request.h   | 17 ++++++++++++++---
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 5bac1b8f..8eced51d 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -37,9 +37,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  		CameraStream *stream =
>  			static_cast<CameraStream *>(buffer.stream->priv);
>  
> -		buffers_.push_back({ stream, buffer.buffer, nullptr,
> -				     buffer.acquire_fence, Status::Success,
> -				     nullptr, nullptr, nullptr, this });
> +		buffers_.emplace_back(stream, buffer, this);
>  	}
>  
>  	/* Clone the controls associated with the camera3 request. */
> @@ -54,3 +52,18 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  }
>  
>  Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> +
> +Camera3RequestDescriptor::StreamBuffer::StreamBuffer(
> +	CameraStream *stream, const camera3_stream_buffer_t &buffer,
> +	Camera3RequestDescriptor *request)
> +	: camera3Buffer(buffer.buffer), fence(buffer.acquire_fence),
> +	  stream(stream), request(request)
> +{
> +}
> +
> +Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;
> +
> +Camera3RequestDescriptor::StreamBuffer::StreamBuffer(StreamBuffer &&other) = default;
> +
> +Camera3RequestDescriptor::StreamBuffer &
> +Camera3RequestDescriptor::StreamBuffer::operator=(Camera3RequestDescriptor::StreamBuffer &&) = default;
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index cfafa445..d2935c99 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -34,15 +34,26 @@ public:
>  	};
>  
>  	struct StreamBuffer {
> +		StreamBuffer(CameraStream *stream,
> +			     const camera3_stream_buffer_t &buffer,
> +			     Camera3RequestDescriptor *request);
> +		~StreamBuffer();
> +
> +		StreamBuffer(StreamBuffer &&other);
> +		StreamBuffer &operator=(StreamBuffer &&);

		StreamBuffer &operator=(StreamBuffer &&other);

and possibly the same in the .cpp file (although I won't object if you
instead remove the variable name from the move constructor there).

> +
>  		CameraStream *stream;
>  		buffer_handle_t *camera3Buffer;
>  		std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
>  		int fence;
> -		Status status;
> -		libcamera::FrameBuffer *internalBuffer;
> -		const libcamera::FrameBuffer *srcBuffer;
> +		Status status = Status::Success;
> +		libcamera::FrameBuffer *internalBuffer = nullptr;
> +		const libcamera::FrameBuffer *srcBuffer = nullptr;
>  		std::unique_ptr<CameraBuffer> dstBuffer;
>  		Camera3RequestDescriptor *request;
> +
> +	private:
> +		LIBCAMERA_DISABLE_COPY(StreamBuffer)
>  	};
>  
>  	/* Keeps track of streams requiring post-processing. */
Hirokazu Honda Oct. 29, 2021, 2:54 a.m. UTC | #2 
HI Umang,

On Fri, Oct 29, 2021 at 2:38 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Umang,
>
> Thank you for the patch.
>
> On Thu, Oct 28, 2021 at 10:32:54PM +0530, Umang Jain wrote:
> > Provide a constructor for StreamBuffer and use that while populating
> > Camera3RequestDescriptor::buffers_ vector. Also provide the default
> > move-constructor (required as StreamBuffer is stored in a vector in
> > Camera3RequestDescriptor) and destructor for the StreamBuffer struct.
> >
> > Also declare a default move assignment operator and disable the
> > copy constructor and move operator explicitly with
> > LIBCAMERA_DISABLE_COPY().
> >
> > While at it, initialize pointers members in the StreamBuffer struct
> > to nullptr, with StreamBuffer::status set to Status::Success by default.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/android/camera_request.cpp | 19 ++++++++++++++++---
> >  src/android/camera_request.h   | 17 ++++++++++++++---
> >  2 files changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> > index 5bac1b8f..8eced51d 100644
> > --- a/src/android/camera_request.cpp
> > +++ b/src/android/camera_request.cpp
> > @@ -37,9 +37,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> >               CameraStream *stream =
> >                       static_cast<CameraStream *>(buffer.stream->priv);
> >
> > -             buffers_.push_back({ stream, buffer.buffer, nullptr,
> > -                                  buffer.acquire_fence, Status::Success,
> > -                                  nullptr, nullptr, nullptr, this });
> > +             buffers_.emplace_back(stream, buffer, this);
> >       }
> >
> >       /* Clone the controls associated with the camera3 request. */
> > @@ -54,3 +52,18 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> >  }
> >
> >  Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> > +
> > +Camera3RequestDescriptor::StreamBuffer::StreamBuffer(
> > +     CameraStream *stream, const camera3_stream_buffer_t &buffer,
> > +     Camera3RequestDescriptor *request)
> > +     : camera3Buffer(buffer.buffer), fence(buffer.acquire_fence),
> > +       stream(stream), request(request)
> > +{
> > +}
> > +
> > +Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;
> > +
> > +Camera3RequestDescriptor::StreamBuffer::StreamBuffer(StreamBuffer &&other) = default;
> > +
> > +Camera3RequestDescriptor::StreamBuffer &
> > +Camera3RequestDescriptor::StreamBuffer::operator=(Camera3RequestDescriptor::StreamBuffer &&) = default;
> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > index cfafa445..d2935c99 100644
> > --- a/src/android/camera_request.h
> > +++ b/src/android/camera_request.h
> > @@ -34,15 +34,26 @@ public:
> >       };
> >
> >       struct StreamBuffer {
> > +             StreamBuffer(CameraStream *stream,
> > +                          const camera3_stream_buffer_t &buffer,
> > +                          Camera3RequestDescriptor *request);
> > +             ~StreamBuffer();
> > +
> > +             StreamBuffer(StreamBuffer &&other);
> > +             StreamBuffer &operator=(StreamBuffer &&);
>
>                 StreamBuffer &operator=(StreamBuffer &&other);
>
> and possibly the same in the .cpp file (although I won't object if you
> instead remove the variable name from the move constructor there).
>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > +
> >               CameraStream *stream;
> >               buffer_handle_t *camera3Buffer;
> >               std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
> >               int fence;
> > -             Status status;
> > -             libcamera::FrameBuffer *internalBuffer;
> > -             const libcamera::FrameBuffer *srcBuffer;
> > +             Status status = Status::Success;
> > +             libcamera::FrameBuffer *internalBuffer = nullptr;
> > +             const libcamera::FrameBuffer *srcBuffer = nullptr;
> >               std::unique_ptr<CameraBuffer> dstBuffer;
> >               Camera3RequestDescriptor *request;
> > +
> > +     private:
> > +             LIBCAMERA_DISABLE_COPY(StreamBuffer)
> >       };
> >
> >       /* Keeps track of streams requiring post-processing. */
>
> --
> Regards,
>
> Laurent Pinchart
Jacopo Mondi Oct. 29, 2021, 7:42 a.m. UTC | #3 
Hi Uman,

On Thu, Oct 28, 2021 at 10:32:54PM +0530, Umang Jain wrote:
> Provide a constructor for StreamBuffer and use that while populating
> Camera3RequestDescriptor::buffers_ vector. Also provide the default
> move-constructor (required as StreamBuffer is stored in a vector in
> Camera3RequestDescriptor) and destructor for the StreamBuffer struct.
>
> Also declare a default move assignment operator and disable the
> copy constructor and move operator explicitly with
> LIBCAMERA_DISABLE_COPY().
>
> While at it, initialize pointers members in the StreamBuffer struct
> to nullptr, with StreamBuffer::status set to Status::Success by default.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  src/android/camera_request.cpp | 19 ++++++++++++++++---
>  src/android/camera_request.h   | 17 ++++++++++++++---
>  2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 5bac1b8f..8eced51d 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -37,9 +37,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  		CameraStream *stream =
>  			static_cast<CameraStream *>(buffer.stream->priv);
>
> -		buffers_.push_back({ stream, buffer.buffer, nullptr,
> -				     buffer.acquire_fence, Status::Success,
> -				     nullptr, nullptr, nullptr, this });
> +		buffers_.emplace_back(stream, buffer, this);
>  	}
>
>  	/* Clone the controls associated with the camera3 request. */
> @@ -54,3 +52,18 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  }
>
>  Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> +
> +Camera3RequestDescriptor::StreamBuffer::StreamBuffer(
> +	CameraStream *stream, const camera3_stream_buffer_t &buffer,
> +	Camera3RequestDescriptor *request)
> +	: camera3Buffer(buffer.buffer), fence(buffer.acquire_fence),
> +	  stream(stream), request(request)
> +{
> +}
> +
> +Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;
> +
> +Camera3RequestDescriptor::StreamBuffer::StreamBuffer(StreamBuffer &&other) = default;
> +
> +Camera3RequestDescriptor::StreamBuffer &
> +Camera3RequestDescriptor::StreamBuffer::operator=(Camera3RequestDescriptor::StreamBuffer &&) = default;
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index cfafa445..d2935c99 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -34,15 +34,26 @@ public:
>  	};
>
>  	struct StreamBuffer {
> +		StreamBuffer(CameraStream *stream,
> +			     const camera3_stream_buffer_t &buffer,
> +			     Camera3RequestDescriptor *request);
> +		~StreamBuffer();
> +
> +		StreamBuffer(StreamBuffer &&other);
> +		StreamBuffer &operator=(StreamBuffer &&);
> +
>  		CameraStream *stream;
>  		buffer_handle_t *camera3Buffer;
>  		std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
>  		int fence;
> -		Status status;
> -		libcamera::FrameBuffer *internalBuffer;
> -		const libcamera::FrameBuffer *srcBuffer;
> +		Status status = Status::Success;
> +		libcamera::FrameBuffer *internalBuffer = nullptr;
> +		const libcamera::FrameBuffer *srcBuffer = nullptr;
>  		std::unique_ptr<CameraBuffer> dstBuffer;
>  		Camera3RequestDescriptor *request;
> +
> +	private:
> +		LIBCAMERA_DISABLE_COPY(StreamBuffer)
>  	};
>
>  	/* Keeps track of streams requiring post-processing. */
> --
> 2.31.0
>

Patch
diff mbox series 

diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
index 5bac1b8f..8eced51d 100644
--- a/src/android/camera_request.cpp
+++ b/src/android/camera_request.cpp
@@ -37,9 +37,7 @@  Camera3RequestDescriptor::Camera3RequestDescriptor(
 		CameraStream *stream =
 			static_cast<CameraStream *>(buffer.stream->priv);
 
-		buffers_.push_back({ stream, buffer.buffer, nullptr,
-				     buffer.acquire_fence, Status::Success,
-				     nullptr, nullptr, nullptr, this });
+		buffers_.emplace_back(stream, buffer, this);
 	}
 
 	/* Clone the controls associated with the camera3 request. */
@@ -54,3 +52,18 @@  Camera3RequestDescriptor::Camera3RequestDescriptor(
 }
 
 Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
+
+Camera3RequestDescriptor::StreamBuffer::StreamBuffer(
+	CameraStream *stream, const camera3_stream_buffer_t &buffer,
+	Camera3RequestDescriptor *request)
+	: camera3Buffer(buffer.buffer), fence(buffer.acquire_fence),
+	  stream(stream), request(request)
+{
+}
+
+Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;
+
+Camera3RequestDescriptor::StreamBuffer::StreamBuffer(StreamBuffer &&other) = default;
+
+Camera3RequestDescriptor::StreamBuffer &
+Camera3RequestDescriptor::StreamBuffer::operator=(Camera3RequestDescriptor::StreamBuffer &&) = default;
diff --git a/src/android/camera_request.h b/src/android/camera_request.h
index cfafa445..d2935c99 100644
--- a/src/android/camera_request.h
+++ b/src/android/camera_request.h
@@ -34,15 +34,26 @@  public:
 	};
 
 	struct StreamBuffer {
+		StreamBuffer(CameraStream *stream,
+			     const camera3_stream_buffer_t &buffer,
+			     Camera3RequestDescriptor *request);
+		~StreamBuffer();
+
+		StreamBuffer(StreamBuffer &&other);
+		StreamBuffer &operator=(StreamBuffer &&);
+
 		CameraStream *stream;
 		buffer_handle_t *camera3Buffer;
 		std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
 		int fence;
-		Status status;
-		libcamera::FrameBuffer *internalBuffer;
-		const libcamera::FrameBuffer *srcBuffer;
+		Status status = Status::Success;
+		libcamera::FrameBuffer *internalBuffer = nullptr;
+		const libcamera::FrameBuffer *srcBuffer = nullptr;
 		std::unique_ptr<CameraBuffer> dstBuffer;
 		Camera3RequestDescriptor *request;
+
+	private:
+		LIBCAMERA_DISABLE_COPY(StreamBuffer)
 	};
 
 	/* Keeps track of streams requiring post-processing. */