[libcamera-devel,10/15] android: camera_stream: Allocate FrameBuffers
diff mbox series

Message ID 20201005104649.10812-11-laurent.pinchart@ideasonboard.com
State Rejected
Delegated to: Laurent Pinchart
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>

Allocate FrameBuffers using the allocator_ class member in the
CameraStream class at CameraStream::configure() time.

As FrameBuffer allocation can only happen after the Camera has been
correctly configured, move the CameraStream configuration loop
after the Camera::configure() call in CameraDevice.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 22 +++++++++++-----------
 src/android/camera_stream.cpp | 17 +++++++++++++++--
 src/android/camera_stream.h   |  2 ++
 3 files changed, 28 insertions(+), 13 deletions(-)

Comments

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

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Allocate FrameBuffers using the allocator_ class member in the
> CameraStream class at CameraStream::configure() time.
> 
> As FrameBuffer allocation can only happen after the Camera has been
> correctly configured, move the CameraStream configuration loop
> after the Camera::configure() call in CameraDevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 22 +++++++++++-----------
>  src/android/camera_stream.cpp | 17 +++++++++++++++--
>  src/android/camera_stream.h   |  2 ++
>  3 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index adaec54dbfdb..537883b63f15 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Once the CameraConfiguration has been adjusted/validated
> +	 * it can be applied to the camera.
> +	 */
> +	int ret = camera_->configure(config_.get());
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to configure camera '"
> +				<< camera_->id() << "'";
> +		return ret;
> +	}
> +
>  	/*
>  	 * Configure the HAL CameraStream instances using the associated
>  	 * StreamConfiguration and set the number of required buffers in
> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		stream->max_buffers = cfg.bufferCount;
>  	}
>  
> -	/*
> -	 * Once the CameraConfiguration has been adjusted/validated
> -	 * it can be applied to the camera.
> -	 */
> -	int ret = camera_->configure(config_.get());
> -	if (ret) {
> -		LOG(HAL, Error) << "Failed to configure camera '"
> -				<< camera_->id() << "'";
> -		return ret;
> -	}
> -
>  	return 0;
>  }
>  
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 9f3e7026b1a4..76e7d240f4e7 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const
>  
>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>  {
> -	if (encoder_)
> -		return encoder_->configure(cfg);
> +	if (encoder_) {
> +		int ret = encoder_->configure(cfg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (allocator_) {
> +		int ret = allocator_->allocate(stream());
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Save a pointer to the reserved frame buffers */
> +		for (const auto &frameBuffer : allocator_->buffers(stream()))
> +			buffers_.push_back(frameBuffer.get());

I'd move this to patch 12/15, it's not clear in this patch why you need
it.

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

> +	}
>  
>  	return 0;
>  }
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index eaf4357ed096..e399e17b2a2f 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -8,6 +8,7 @@
>  #define __ANDROID_CAMERA_STREAM_H__
>  
>  #include <memory>
> +#include <vector>
>  
>  #include <hardware/camera3.h>
>  
> @@ -140,6 +141,7 @@ private:
>  	libcamera::CameraConfiguration *config_;
>  	Encoder *encoder_;
>  	libcamera::FrameBufferAllocator *allocator_;
> +	std::vector<libcamera::FrameBuffer *> buffers_;
>  };
>  
>  #endif /* __ANDROID_CAMERA_STREAM__ */
Kieran Bingham Oct. 6, 2020, 12:29 p.m. UTC | #2 
Hi Jacopo

On 05/10/2020 13:28, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Allocate FrameBuffers using the allocator_ class member in the
>> CameraStream class at CameraStream::configure() time.
>>
>> As FrameBuffer allocation can only happen after the Camera has been
>> correctly configured, move the CameraStream configuration loop
>> after the Camera::configure() call in CameraDevice.

If we know that now, should we update the patch earlier to put it there
in the first place?

(Or is that more effort than it's worth, or possible not correct at that
point in the series).


>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 22 +++++++++++-----------
>>  src/android/camera_stream.cpp | 17 +++++++++++++++--
>>  src/android/camera_stream.h   |  2 ++
>>  3 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index adaec54dbfdb..537883b63f15 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * Once the CameraConfiguration has been adjusted/validated
>> +	 * it can be applied to the camera.
>> +	 */
>> +	int ret = camera_->configure(config_.get());
>> +	if (ret) {
>> +		LOG(HAL, Error) << "Failed to configure camera '"
>> +				<< camera_->id() << "'";
>> +		return ret;
>> +	}
>> +
>>  	/*
>>  	 * Configure the HAL CameraStream instances using the associated
>>  	 * StreamConfiguration and set the number of required buffers in
>> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		stream->max_buffers = cfg.bufferCount;
>>  	}
>>  
>> -	/*
>> -	 * Once the CameraConfiguration has been adjusted/validated
>> -	 * it can be applied to the camera.
>> -	 */
>> -	int ret = camera_->configure(config_.get());
>> -	if (ret) {
>> -		LOG(HAL, Error) << "Failed to configure camera '"
>> -				<< camera_->id() << "'";
>> -		return ret;
>> -	}
>> -
>>  	return 0;
>>  }
>>  
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 9f3e7026b1a4..76e7d240f4e7 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const
>>  
>>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>>  {
>> -	if (encoder_)
>> -		return encoder_->configure(cfg);
>> +	if (encoder_) {
>> +		int ret = encoder_->configure(cfg);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (allocator_) {
>> +		int ret = allocator_->allocate(stream());
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* Save a pointer to the reserved frame buffers */
>> +		for (const auto &frameBuffer : allocator_->buffers(stream()))
>> +			buffers_.push_back(frameBuffer.get());
> 
> I'd move this to patch 12/15, it's not clear in this patch why you need
> it.


If the configure gets moved in the patch when it is originally added,
and this buffers_.push_back gets moved to patch 12/15 - the only thing
left here might be the allocate - which at that piont might also be
easily squashed into 12/15... but it's not a requirement - just what
might happen if the other blocks reduce this patch to very little ;-)

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

If it stays otherwise:

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


> 
>> +	}
>>  
>>  	return 0;
>>  }
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index eaf4357ed096..e399e17b2a2f 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -8,6 +8,7 @@
>>  #define __ANDROID_CAMERA_STREAM_H__
>>  
>>  #include <memory>
>> +#include <vector>
>>  
>>  #include <hardware/camera3.h>
>>  
>> @@ -140,6 +141,7 @@ private:
>>  	libcamera::CameraConfiguration *config_;
>>  	Encoder *encoder_;
>>  	libcamera::FrameBufferAllocator *allocator_;
>> +	std::vector<libcamera::FrameBuffer *> buffers_;
>>  };
>>  
>>  #endif /* __ANDROID_CAMERA_STREAM__ */
>
Jacopo Mondi Oct. 6, 2020, 12:40 p.m. UTC | #3 
Hi Kieran

On Tue, Oct 06, 2020 at 01:29:30PM +0100, Kieran Bingham wrote:
> Hi Jacopo
>
> On 05/10/2020 13:28, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Allocate FrameBuffers using the allocator_ class member in the
> >> CameraStream class at CameraStream::configure() time.
> >>
> >> As FrameBuffer allocation can only happen after the Camera has been
> >> correctly configured, move the CameraStream configuration loop
> >> after the Camera::configure() call in CameraDevice.
>
> If we know that now, should we update the patch earlier to put it there
> in the first place?
>
> (Or is that more effort than it's worth, or possible not correct at that
> point in the series).

It is not required before CameraStream::configure() has to allocate
buffers, so I think the change should happen at the same time.

>
>
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/android/camera_device.cpp | 22 +++++++++++-----------
> >>  src/android/camera_stream.cpp | 17 +++++++++++++++--
> >>  src/android/camera_stream.h   |  2 ++
> >>  3 files changed, 28 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index adaec54dbfdb..537883b63f15 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	/*
> >> +	 * Once the CameraConfiguration has been adjusted/validated
> >> +	 * it can be applied to the camera.
> >> +	 */
> >> +	int ret = camera_->configure(config_.get());
> >> +	if (ret) {
> >> +		LOG(HAL, Error) << "Failed to configure camera '"
> >> +				<< camera_->id() << "'";
> >> +		return ret;
> >> +	}
> >> +
> >>  	/*
> >>  	 * Configure the HAL CameraStream instances using the associated
> >>  	 * StreamConfiguration and set the number of required buffers in
> >> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		stream->max_buffers = cfg.bufferCount;
> >>  	}
> >>
> >> -	/*
> >> -	 * Once the CameraConfiguration has been adjusted/validated
> >> -	 * it can be applied to the camera.
> >> -	 */
> >> -	int ret = camera_->configure(config_.get());
> >> -	if (ret) {
> >> -		LOG(HAL, Error) << "Failed to configure camera '"
> >> -				<< camera_->id() << "'";
> >> -		return ret;
> >> -	}
> >> -
> >>  	return 0;
> >>  }
> >>
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index 9f3e7026b1a4..76e7d240f4e7 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const
> >>
> >>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
> >>  {
> >> -	if (encoder_)
> >> -		return encoder_->configure(cfg);
> >> +	if (encoder_) {
> >> +		int ret = encoder_->configure(cfg);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	if (allocator_) {
> >> +		int ret = allocator_->allocate(stream());
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		/* Save a pointer to the reserved frame buffers */
> >> +		for (const auto &frameBuffer : allocator_->buffers(stream()))
> >> +			buffers_.push_back(frameBuffer.get());
> >
> > I'd move this to patch 12/15, it's not clear in this patch why you need
> > it.
>
>
> If the configure gets moved in the patch when it is originally added,
> and this buffers_.push_back gets moved to patch 12/15 - the only thing
> left here might be the allocate - which at that piont might also be
> easily squashed into 12/15... but it's not a requirement - just what
> might happen if the other blocks reduce this patch to very little ;-)
>

I've moved everything to patch 12/15

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> If it stays otherwise:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Thanks
  j

>
> >
> >> +	}
> >>
> >>  	return 0;
> >>  }
> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >> index eaf4357ed096..e399e17b2a2f 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -8,6 +8,7 @@
> >>  #define __ANDROID_CAMERA_STREAM_H__
> >>
> >>  #include <memory>
> >> +#include <vector>
> >>
> >>  #include <hardware/camera3.h>
> >>
> >> @@ -140,6 +141,7 @@ private:
> >>  	libcamera::CameraConfiguration *config_;
> >>  	Encoder *encoder_;
> >>  	libcamera::FrameBufferAllocator *allocator_;
> >> +	std::vector<libcamera::FrameBuffer *> buffers_;
> >>  };
> >>
> >>  #endif /* __ANDROID_CAMERA_STREAM__ */
> >
>
> --
> Regards
> --
> Kieran

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index adaec54dbfdb..537883b63f15 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1282,6 +1282,17 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		return -EINVAL;
 	}
 
+	/*
+	 * Once the CameraConfiguration has been adjusted/validated
+	 * it can be applied to the camera.
+	 */
+	int ret = camera_->configure(config_.get());
+	if (ret) {
+		LOG(HAL, Error) << "Failed to configure camera '"
+				<< camera_->id() << "'";
+		return ret;
+	}
+
 	/*
 	 * Configure the HAL CameraStream instances using the associated
 	 * StreamConfiguration and set the number of required buffers in
@@ -1300,17 +1311,6 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		stream->max_buffers = cfg.bufferCount;
 	}
 
-	/*
-	 * Once the CameraConfiguration has been adjusted/validated
-	 * it can be applied to the camera.
-	 */
-	int ret = camera_->configure(config_.get());
-	if (ret) {
-		LOG(HAL, Error) << "Failed to configure camera '"
-				<< camera_->id() << "'";
-		return ret;
-	}
-
 	return 0;
 }
 
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 9f3e7026b1a4..76e7d240f4e7 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -58,8 +58,21 @@  const Size &CameraStream::size() const
 
 int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
 {
-	if (encoder_)
-		return encoder_->configure(cfg);
+	if (encoder_) {
+		int ret = encoder_->configure(cfg);
+		if (ret)
+			return ret;
+	}
+
+	if (allocator_) {
+		int ret = allocator_->allocate(stream());
+		if (ret < 0)
+			return ret;
+
+		/* Save a pointer to the reserved frame buffers */
+		for (const auto &frameBuffer : allocator_->buffers(stream()))
+			buffers_.push_back(frameBuffer.get());
+	}
 
 	return 0;
 }
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index eaf4357ed096..e399e17b2a2f 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -8,6 +8,7 @@ 
 #define __ANDROID_CAMERA_STREAM_H__
 
 #include <memory>
+#include <vector>
 
 #include <hardware/camera3.h>
 
@@ -140,6 +141,7 @@  private:
 	libcamera::CameraConfiguration *config_;
 	Encoder *encoder_;
 	libcamera::FrameBufferAllocator *allocator_;
+	std::vector<libcamera::FrameBuffer *> buffers_;
 };
 
 #endif /* __ANDROID_CAMERA_STREAM__ */