[libcamera-devel,14/30] libcamera: request: In addBuffer() do not fetch stream from Buffer
diff mbox series

Message ID 20191126233620.1695316-15-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Rework buffer API
Related show

Commit Message

Niklas Söderlund Nov. 26, 2019, 11:36 p.m. UTC 
In the FrameBuffer interface the stream will not be available from the
buffer object as the buffer might be allocated externally. The
application needs to explicitly state which stream the buffer is being
added for to the request.

Extend the addBuffer() function to get this information explicitly from
the caller.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/request.h   | 2 +-
 src/android/camera_device.cpp | 2 +-
 src/cam/capture.cpp           | 4 ++--
 src/libcamera/request.cpp     | 4 ++--
 src/qcam/main_window.cpp      | 4 ++--
 test/camera/buffer_import.cpp | 2 +-
 test/camera/capture.cpp       | 4 ++--
 test/camera/statemachine.cpp  | 2 +-
 8 files changed, 12 insertions(+), 12 deletions(-)

Comments

Jacopo Mondi Nov. 27, 2019, 2:59 p.m. UTC | #1 
Hi Niklas,

On Wed, Nov 27, 2019 at 12:36:04AM +0100, Niklas Söderlund wrote:
> In the FrameBuffer interface the stream will not be available from the
> buffer object as the buffer might be allocated externally. The
> application needs to explicitly state which stream the buffer is being
> added for to the request.
>
> Extend the addBuffer() function to get this information explicitly from
> the caller.

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

Thanks
  j

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/request.h   | 2 +-
>  src/android/camera_device.cpp | 2 +-
>  src/cam/capture.cpp           | 4 ++--
>  src/libcamera/request.cpp     | 4 ++--
>  src/qcam/main_window.cpp      | 4 ++--
>  test/camera/buffer_import.cpp | 2 +-
>  test/camera/capture.cpp       | 4 ++--
>  test/camera/statemachine.cpp  | 2 +-
>  8 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 2d5a5964e99eb75f..a8708010ec1247a2 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -39,7 +39,7 @@ public:
>  	ControlList &controls() { return *controls_; }
>  	ControlList &metadata() { return *metadata_; }
>  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> -	int addBuffer(std::unique_ptr<Buffer> buffer);
> +	int addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer);
>  	Buffer *findBuffer(Stream *stream) const;
>
>  	uint64_t cookie() const { return cookie_; }
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 065e0292e186c2ad..09588c16a6301649 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -754,7 +754,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
>
>  	Request *request =
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> -	request->addBuffer(std::move(buffer));
> +	request->addBuffer(stream, std::move(buffer));
>
>  	int ret = camera_->queueRequest(request);
>  	if (ret) {
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 4b65b1d0a2dbed35..1a4dbe7ce4a15a2d 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -95,7 +95,7 @@ int Capture::capture(EventLoop *loop)
>  			Stream *stream = cfg.stream();
>  			std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
>
> -			ret = request->addBuffer(std::move(buffer));
> +			ret = request->addBuffer(stream, std::move(buffer));
>  			if (ret < 0) {
>  				std::cerr << "Can't set buffer for request"
>  					  << std::endl;
> @@ -185,7 +185,7 @@ void Capture::requestComplete(Request *request)
>  			return;
>  		}
>
> -		request->addBuffer(std::move(newBuffer));
> +		request->addBuffer(stream, std::move(newBuffer));
>  	}
>
>  	camera_->queueRequest(request);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index c2854dc2e8caab2e..3b49e52510918eee 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -113,6 +113,7 @@ Request::~Request()
>
>  /**
>   * \brief Store a Buffer with its associated Stream in the Request
> + * \param[in] stream The stream the buffer belongs to
>   * \param[in] buffer The Buffer to store in the request
>   *
>   * Ownership of the buffer is passed to the request. It will be deleted when
> @@ -125,9 +126,8 @@ Request::~Request()
>   * \retval -EEXIST The request already contains a buffer for the stream
>   * \retval -EINVAL The buffer does not reference a valid Stream
>   */
> -int Request::addBuffer(std::unique_ptr<Buffer> buffer)
> +int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)
>  {
> -	Stream *stream = buffer->stream();
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
>  		return -EINVAL;
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index a828a176cbbf4854..4cecf7e351214f3d 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -190,7 +190,7 @@ int MainWindow::startCapture()
>  			goto error;
>  		}
>
> -		ret = request->addBuffer(std::move(buffer));
> +		ret = request->addBuffer(stream, std::move(buffer));
>  		if (ret < 0) {
>  			std::cerr << "Can't set buffer for request" << std::endl;
>  			goto error;
> @@ -288,7 +288,7 @@ void MainWindow::requestComplete(Request *request)
>  			return;
>  		}
>
> -		request->addBuffer(std::move(newBuffer));
> +		request->addBuffer(stream, std::move(newBuffer));
>  	}
>
>  	camera_->queueRequest(request);
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 8b0d1c167bd2bbe8..dae907f9f41841c9 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -268,7 +268,7 @@ public:
>  		Request *request = camera_->createRequest(cookie);
>
>  		std::unique_ptr<Buffer> buffer = stream_->createBuffer({ dmabuf, -1, -1 });
> -		request->addBuffer(move(buffer));
> +		request->addBuffer(stream_, move(buffer));
>  		camera_->queueRequest(request);
>  	}
>
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 7cb76038f557d461..ca1ebe419946dd4d 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -49,7 +49,7 @@ protected:
>  		std::unique_ptr<Buffer> newBuffer = stream->createBuffer(buffer->index());
>
>  		request = camera_->createRequest();
> -		request->addBuffer(std::move(newBuffer));
> +		request->addBuffer(stream, std::move(newBuffer));
>  		camera_->queueRequest(request);
>  	}
>
> @@ -101,7 +101,7 @@ protected:
>  				return TestFail;
>  			}
>
> -			if (request->addBuffer(std::move(buffer))) {
> +			if (request->addBuffer(stream, std::move(buffer))) {
>  				cout << "Failed to associating buffer with request" << endl;
>  				return TestFail;
>  			}
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index afa13ba77b0b7fe0..f627b8f37422350e 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -219,7 +219,7 @@ protected:
>
>  		Stream *stream = *camera_->streams().begin();
>  		std::unique_ptr<Buffer> buffer = stream->createBuffer(0);
> -		if (request->addBuffer(std::move(buffer)))
> +		if (request->addBuffer(stream, std::move(buffer)))
>  			return TestFail;
>
>  		if (camera_->queueRequest(request))
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Dec. 9, 2019, 6:41 p.m. UTC | #2 
Hi Niklas,

Thank you for the patch.

On Wed, Nov 27, 2019 at 12:36:04AM +0100, Niklas Söderlund wrote:
> In the FrameBuffer interface the stream will not be available from the
> buffer object as the buffer might be allocated externally. The
> application needs to explicitly state which stream the buffer is being
> added for to the request.
> 
> Extend the addBuffer() function to get this information explicitly from
> the caller.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

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

> ---
>  include/libcamera/request.h   | 2 +-
>  src/android/camera_device.cpp | 2 +-
>  src/cam/capture.cpp           | 4 ++--
>  src/libcamera/request.cpp     | 4 ++--
>  src/qcam/main_window.cpp      | 4 ++--
>  test/camera/buffer_import.cpp | 2 +-
>  test/camera/capture.cpp       | 4 ++--
>  test/camera/statemachine.cpp  | 2 +-
>  8 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 2d5a5964e99eb75f..a8708010ec1247a2 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -39,7 +39,7 @@ public:
>  	ControlList &controls() { return *controls_; }
>  	ControlList &metadata() { return *metadata_; }
>  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> -	int addBuffer(std::unique_ptr<Buffer> buffer);
> +	int addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer);
>  	Buffer *findBuffer(Stream *stream) const;
>  
>  	uint64_t cookie() const { return cookie_; }
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 065e0292e186c2ad..09588c16a6301649 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -754,7 +754,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
>  
>  	Request *request =
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> -	request->addBuffer(std::move(buffer));
> +	request->addBuffer(stream, std::move(buffer));
>  
>  	int ret = camera_->queueRequest(request);
>  	if (ret) {
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 4b65b1d0a2dbed35..1a4dbe7ce4a15a2d 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -95,7 +95,7 @@ int Capture::capture(EventLoop *loop)
>  			Stream *stream = cfg.stream();
>  			std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
>  
> -			ret = request->addBuffer(std::move(buffer));
> +			ret = request->addBuffer(stream, std::move(buffer));
>  			if (ret < 0) {
>  				std::cerr << "Can't set buffer for request"
>  					  << std::endl;
> @@ -185,7 +185,7 @@ void Capture::requestComplete(Request *request)
>  			return;
>  		}
>  
> -		request->addBuffer(std::move(newBuffer));
> +		request->addBuffer(stream, std::move(newBuffer));
>  	}
>  
>  	camera_->queueRequest(request);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index c2854dc2e8caab2e..3b49e52510918eee 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -113,6 +113,7 @@ Request::~Request()
>  
>  /**
>   * \brief Store a Buffer with its associated Stream in the Request
> + * \param[in] stream The stream the buffer belongs to
>   * \param[in] buffer The Buffer to store in the request
>   *
>   * Ownership of the buffer is passed to the request. It will be deleted when
> @@ -125,9 +126,8 @@ Request::~Request()
>   * \retval -EEXIST The request already contains a buffer for the stream
>   * \retval -EINVAL The buffer does not reference a valid Stream
>   */
> -int Request::addBuffer(std::unique_ptr<Buffer> buffer)
> +int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)
>  {
> -	Stream *stream = buffer->stream();
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
>  		return -EINVAL;
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index a828a176cbbf4854..4cecf7e351214f3d 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -190,7 +190,7 @@ int MainWindow::startCapture()
>  			goto error;
>  		}
>  
> -		ret = request->addBuffer(std::move(buffer));
> +		ret = request->addBuffer(stream, std::move(buffer));
>  		if (ret < 0) {
>  			std::cerr << "Can't set buffer for request" << std::endl;
>  			goto error;
> @@ -288,7 +288,7 @@ void MainWindow::requestComplete(Request *request)
>  			return;
>  		}
>  
> -		request->addBuffer(std::move(newBuffer));
> +		request->addBuffer(stream, std::move(newBuffer));
>  	}
>  
>  	camera_->queueRequest(request);
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 8b0d1c167bd2bbe8..dae907f9f41841c9 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -268,7 +268,7 @@ public:
>  		Request *request = camera_->createRequest(cookie);
>  
>  		std::unique_ptr<Buffer> buffer = stream_->createBuffer({ dmabuf, -1, -1 });
> -		request->addBuffer(move(buffer));
> +		request->addBuffer(stream_, move(buffer));
>  		camera_->queueRequest(request);
>  	}
>  
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 7cb76038f557d461..ca1ebe419946dd4d 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -49,7 +49,7 @@ protected:
>  		std::unique_ptr<Buffer> newBuffer = stream->createBuffer(buffer->index());
>  
>  		request = camera_->createRequest();
> -		request->addBuffer(std::move(newBuffer));
> +		request->addBuffer(stream, std::move(newBuffer));
>  		camera_->queueRequest(request);
>  	}
>  
> @@ -101,7 +101,7 @@ protected:
>  				return TestFail;
>  			}
>  
> -			if (request->addBuffer(std::move(buffer))) {
> +			if (request->addBuffer(stream, std::move(buffer))) {
>  				cout << "Failed to associating buffer with request" << endl;
>  				return TestFail;
>  			}
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index afa13ba77b0b7fe0..f627b8f37422350e 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -219,7 +219,7 @@ protected:
>  
>  		Stream *stream = *camera_->streams().begin();
>  		std::unique_ptr<Buffer> buffer = stream->createBuffer(0);
> -		if (request->addBuffer(std::move(buffer)))
> +		if (request->addBuffer(stream, std::move(buffer)))
>  			return TestFail;
>  
>  		if (camera_->queueRequest(request))

Patch
diff mbox series 

diff --git a/include/libcamera/request.h b/include/libcamera/request.h
index 2d5a5964e99eb75f..a8708010ec1247a2 100644
--- a/include/libcamera/request.h
+++ b/include/libcamera/request.h
@@ -39,7 +39,7 @@  public:
 	ControlList &controls() { return *controls_; }
 	ControlList &metadata() { return *metadata_; }
 	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
-	int addBuffer(std::unique_ptr<Buffer> buffer);
+	int addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer);
 	Buffer *findBuffer(Stream *stream) const;
 
 	uint64_t cookie() const { return cookie_; }
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 065e0292e186c2ad..09588c16a6301649 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -754,7 +754,7 @@  void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
 
 	Request *request =
 		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
-	request->addBuffer(std::move(buffer));
+	request->addBuffer(stream, std::move(buffer));
 
 	int ret = camera_->queueRequest(request);
 	if (ret) {
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
index 4b65b1d0a2dbed35..1a4dbe7ce4a15a2d 100644
--- a/src/cam/capture.cpp
+++ b/src/cam/capture.cpp
@@ -95,7 +95,7 @@  int Capture::capture(EventLoop *loop)
 			Stream *stream = cfg.stream();
 			std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
 
-			ret = request->addBuffer(std::move(buffer));
+			ret = request->addBuffer(stream, std::move(buffer));
 			if (ret < 0) {
 				std::cerr << "Can't set buffer for request"
 					  << std::endl;
@@ -185,7 +185,7 @@  void Capture::requestComplete(Request *request)
 			return;
 		}
 
-		request->addBuffer(std::move(newBuffer));
+		request->addBuffer(stream, std::move(newBuffer));
 	}
 
 	camera_->queueRequest(request);
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index c2854dc2e8caab2e..3b49e52510918eee 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -113,6 +113,7 @@  Request::~Request()
 
 /**
  * \brief Store a Buffer with its associated Stream in the Request
+ * \param[in] stream The stream the buffer belongs to
  * \param[in] buffer The Buffer to store in the request
  *
  * Ownership of the buffer is passed to the request. It will be deleted when
@@ -125,9 +126,8 @@  Request::~Request()
  * \retval -EEXIST The request already contains a buffer for the stream
  * \retval -EINVAL The buffer does not reference a valid Stream
  */
-int Request::addBuffer(std::unique_ptr<Buffer> buffer)
+int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)
 {
-	Stream *stream = buffer->stream();
 	if (!stream) {
 		LOG(Request, Error) << "Invalid stream reference";
 		return -EINVAL;
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index a828a176cbbf4854..4cecf7e351214f3d 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -190,7 +190,7 @@  int MainWindow::startCapture()
 			goto error;
 		}
 
-		ret = request->addBuffer(std::move(buffer));
+		ret = request->addBuffer(stream, std::move(buffer));
 		if (ret < 0) {
 			std::cerr << "Can't set buffer for request" << std::endl;
 			goto error;
@@ -288,7 +288,7 @@  void MainWindow::requestComplete(Request *request)
 			return;
 		}
 
-		request->addBuffer(std::move(newBuffer));
+		request->addBuffer(stream, std::move(newBuffer));
 	}
 
 	camera_->queueRequest(request);
diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
index 8b0d1c167bd2bbe8..dae907f9f41841c9 100644
--- a/test/camera/buffer_import.cpp
+++ b/test/camera/buffer_import.cpp
@@ -268,7 +268,7 @@  public:
 		Request *request = camera_->createRequest(cookie);
 
 		std::unique_ptr<Buffer> buffer = stream_->createBuffer({ dmabuf, -1, -1 });
-		request->addBuffer(move(buffer));
+		request->addBuffer(stream_, move(buffer));
 		camera_->queueRequest(request);
 	}
 
diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
index 7cb76038f557d461..ca1ebe419946dd4d 100644
--- a/test/camera/capture.cpp
+++ b/test/camera/capture.cpp
@@ -49,7 +49,7 @@  protected:
 		std::unique_ptr<Buffer> newBuffer = stream->createBuffer(buffer->index());
 
 		request = camera_->createRequest();
-		request->addBuffer(std::move(newBuffer));
+		request->addBuffer(stream, std::move(newBuffer));
 		camera_->queueRequest(request);
 	}
 
@@ -101,7 +101,7 @@  protected:
 				return TestFail;
 			}
 
-			if (request->addBuffer(std::move(buffer))) {
+			if (request->addBuffer(stream, std::move(buffer))) {
 				cout << "Failed to associating buffer with request" << endl;
 				return TestFail;
 			}
diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
index afa13ba77b0b7fe0..f627b8f37422350e 100644
--- a/test/camera/statemachine.cpp
+++ b/test/camera/statemachine.cpp
@@ -219,7 +219,7 @@  protected:
 
 		Stream *stream = *camera_->streams().begin();
 		std::unique_ptr<Buffer> buffer = stream->createBuffer(0);
-		if (request->addBuffer(std::move(buffer)))
+		if (request->addBuffer(stream, std::move(buffer)))
 			return TestFail;
 
 		if (camera_->queueRequest(request))