[libcamera-devel,06/15] android: camera_stream: Retrieve Stream and Configuration
diff mbox series

Message ID 20201005104649.10812-7-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • CameraStream refactoring
Related show

Commit Message

Laurent Pinchart Oct. 5, 2020, 10:46 a.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

It's a common pattern to access the libcamera::Stream and
libcamera::StreamConfiguration using the CameraStream instance's
index.

Add two methods to the CameraStream to shorten access to the
two fields. This allows removing the index() method from the class
interface.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 11 +++--------
 src/android/camera_stream.cpp | 10 ++++++++++
 src/android/camera_stream.h   |  4 +++-
 3 files changed, 16 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart Oct. 5, 2020, 12:10 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:40PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> It's a common pattern to access the libcamera::Stream and
> libcamera::StreamConfiguration using the CameraStream instance's
> index.
> 
> Add two methods to the CameraStream to shorten access to the
> two fields. This allows removing the index() method from the class
> interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 11 +++--------
>  src/android/camera_stream.cpp | 10 ++++++++++
>  src/android/camera_stream.h   |  4 +++-
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index c44904300e0a..4f8f3e5790ca 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1292,7 +1292,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>  		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
> -		StreamConfiguration &cfg = config_->at(cameraStream->index());
> +		const StreamConfiguration &cfg = cameraStream->streamConfiguration();
>  
>  		int ret = cameraStream->configure(cfg);
>  		if (ret)
> @@ -1413,10 +1413,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		}
>  		descriptor->frameBuffers.emplace_back(buffer);
>  
> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> -		Stream *stream = streamConfiguration->stream();
> -
> -		request->addBuffer(stream, buffer);
> +		request->addBuffer(cameraStream->stream(), buffer);
>  	}
>  
>  	int ret = camera_->queueRequest(request);
> @@ -1462,9 +1459,7 @@ void CameraDevice::requestComplete(Request *request)
>  		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
>  			continue;
>  
> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> -		Stream *stream = streamConfiguration->stream();
> -		FrameBuffer *buffer = request->findBuffer(stream);
> +		FrameBuffer *buffer = request->findBuffer(cameraStream->stream());
>  		if (!buffer) {
>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
>  			continue;
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 072edc9457bb..250f0ab0a3b4 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -32,6 +32,16 @@ CameraStream::CameraStream(CameraDevice *cameraDev,
>  		encoder_.reset(new EncoderLibJpeg);
>  }
>  
> +const StreamConfiguration &CameraStream::streamConfiguration() const
> +{
> +	return config_->at(index_);
> +}
> +
> +Stream *CameraStream::stream() const
> +{
> +	return streamConfiguration().stream();
> +}
> +
>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>  {
>  	if (encoder_)
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index dbcd1e53219f..fa295a69404f 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -116,7 +116,9 @@ public:
>  	const libcamera::PixelFormat &format() const { return format_; }
>  	const libcamera::Size &size() const { return size_; }
>  	Type type() const { return type_; }
> -	unsigned int index() const { return index_; }
> +
> +	const libcamera::StreamConfiguration &streamConfiguration() const;

Maybe s/streamConfiguration/configuration/ ?

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

> +	libcamera::Stream *stream() const;
>  
>  	int configure(const libcamera::StreamConfiguration &cfg);
>  	int process(libcamera::FrameBuffer *source,
Kieran Bingham Oct. 5, 2020, 9:46 p.m. UTC | #2 
Hi Jacopo

On 05/10/2020 13:10, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:40PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo@jmondi.org>
>>
>> It's a common pattern to access the libcamera::Stream and
>> libcamera::StreamConfiguration using the CameraStream instance's
>> index.
>>
>> Add two methods to the CameraStream to shorten access to the
>> two fields. This allows removing the index() method from the class
>> interface.

Sounds like a win here too ;-)

>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 11 +++--------
>>  src/android/camera_stream.cpp | 10 ++++++++++
>>  src/android/camera_stream.h   |  4 +++-
>>  3 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index c44904300e0a..4f8f3e5790ca 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1292,7 +1292,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>>  		camera3_stream_t *stream = stream_list->streams[i];
>>  		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
>> -		StreamConfiguration &cfg = config_->at(cameraStream->index());
>> +		const StreamConfiguration &cfg = cameraStream->streamConfiguration();
>>  
>>  		int ret = cameraStream->configure(cfg);
>>  		if (ret)
>> @@ -1413,10 +1413,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>  		}
>>  		descriptor->frameBuffers.emplace_back(buffer);
>>  
>> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>> -		Stream *stream = streamConfiguration->stream();
>> -
>> -		request->addBuffer(stream, buffer);
>> +		request->addBuffer(cameraStream->stream(), buffer);
>>  	}
>>  
>>  	int ret = camera_->queueRequest(request);
>> @@ -1462,9 +1459,7 @@ void CameraDevice::requestComplete(Request *request)
>>  		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
>>  			continue;
>>  
>> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>> -		Stream *stream = streamConfiguration->stream();
>> -		FrameBuffer *buffer = request->findBuffer(stream);
>> +		FrameBuffer *buffer = request->findBuffer(cameraStream->stream());
>>  		if (!buffer) {
>>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
>>  			continue;
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 072edc9457bb..250f0ab0a3b4 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -32,6 +32,16 @@ CameraStream::CameraStream(CameraDevice *cameraDev,
>>  		encoder_.reset(new EncoderLibJpeg);
>>  }
>>  
>> +const StreamConfiguration &CameraStream::streamConfiguration() const
>> +{
>> +	return config_->at(index_);
>> +}
>> +
>> +Stream *CameraStream::stream() const
>> +{
>> +	return streamConfiguration().stream();
>> +}
>> +
>>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>>  {
>>  	if (encoder_)
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index dbcd1e53219f..fa295a69404f 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -116,7 +116,9 @@ public:
>>  	const libcamera::PixelFormat &format() const { return format_; }
>>  	const libcamera::Size &size() const { return size_; }
>>  	Type type() const { return type_; }
>> -	unsigned int index() const { return index_; }
>> +
>> +	const libcamera::StreamConfiguration &streamConfiguration() const;
> 
> Maybe s/streamConfiguration/configuration/ ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Likewise.

Reviewed-by: Kieran  Bingham <kieran.bingham@ideasonboard.com>

> 
>> +	libcamera::Stream *stream() const;
>>  
>>  	int configure(const libcamera::StreamConfiguration &cfg);
>>  	int process(libcamera::FrameBuffer *source,
>

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index c44904300e0a..4f8f3e5790ca 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1292,7 +1292,7 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
 		camera3_stream_t *stream = stream_list->streams[i];
 		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
-		StreamConfiguration &cfg = config_->at(cameraStream->index());
+		const StreamConfiguration &cfg = cameraStream->streamConfiguration();
 
 		int ret = cameraStream->configure(cfg);
 		if (ret)
@@ -1413,10 +1413,7 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		}
 		descriptor->frameBuffers.emplace_back(buffer);
 
-		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
-		Stream *stream = streamConfiguration->stream();
-
-		request->addBuffer(stream, buffer);
+		request->addBuffer(cameraStream->stream(), buffer);
 	}
 
 	int ret = camera_->queueRequest(request);
@@ -1462,9 +1459,7 @@  void CameraDevice::requestComplete(Request *request)
 		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
 			continue;
 
-		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
-		Stream *stream = streamConfiguration->stream();
-		FrameBuffer *buffer = request->findBuffer(stream);
+		FrameBuffer *buffer = request->findBuffer(cameraStream->stream());
 		if (!buffer) {
 			LOG(HAL, Error) << "Failed to find a source stream buffer";
 			continue;
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 072edc9457bb..250f0ab0a3b4 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -32,6 +32,16 @@  CameraStream::CameraStream(CameraDevice *cameraDev,
 		encoder_.reset(new EncoderLibJpeg);
 }
 
+const StreamConfiguration &CameraStream::streamConfiguration() const
+{
+	return config_->at(index_);
+}
+
+Stream *CameraStream::stream() const
+{
+	return streamConfiguration().stream();
+}
+
 int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
 {
 	if (encoder_)
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index dbcd1e53219f..fa295a69404f 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -116,7 +116,9 @@  public:
 	const libcamera::PixelFormat &format() const { return format_; }
 	const libcamera::Size &size() const { return size_; }
 	Type type() const { return type_; }
-	unsigned int index() const { return index_; }
+
+	const libcamera::StreamConfiguration &streamConfiguration() const;
+	libcamera::Stream *stream() const;
 
 	int configure(const libcamera::StreamConfiguration &cfg);
 	int process(libcamera::FrameBuffer *source,