[libcamera-devel,03/15] android: camera_stream: Delegate Encoder construction
diff mbox series

Message ID 20201005104649.10812-4-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>

Delegate the construction of the encoder to the CameraStream class
for streams that need post-processing.

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

Comments

Laurent Pinchart Oct. 5, 2020, 11:26 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:37PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Delegate the construction of the encoder to the CameraStream class
> for streams that need post-processing.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 23 ++++++++++-------------
>  src/android/camera_stream.cpp | 17 ++++++++++++++---
>  src/android/camera_stream.h   |  4 +++-
>  3 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 0600ebc81c64..9c9a5cfa3c2f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  		StreamConfiguration &cfg = config_->at(index);
>  
> -		/*
> -		 * Construct a software encoder for the MJPEG streams from the
> -		 * chosen libcamera source stream.
> -		 */
> -		Encoder *encoder = new EncoderLibJpeg();
> -		int ret = encoder->configure(cfg);
> -		if (ret) {
> -			LOG(HAL, Error) << "Failed to configure encoder";
> -			delete encoder;
> -			return ret;
> -		}
> -
> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Configure the HAL CameraStream instances using the associated
> +	 * StreamConfiguration and set the number of required buffers in
> +	 * the Android camera3_stream_t.
> +	 */
>  	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());
>  
> +		int ret = cameraStream->configure(cfg);
> +		if (ret)
> +			return ret;
> +
>  		/* Use the bufferCount confirmed by the validation process. */
>  		stream->max_buffers = cfg.bufferCount;
>  	}
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 585bf2b68f4f..5b2b625c563b 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -7,10 +7,21 @@
>  
>  #include "camera_stream.h"
>  
> +#include "jpeg/encoder_libjpeg.h"
> +
>  using namespace libcamera;
>  
> -CameraStream::CameraStream(PixelFormat format, Size size,
> -			   Type type, unsigned int index, Encoder *encoder)
> -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
> +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
>  {
> +	if (type_ == Type::Internal || type_ == Type::Mapped)
> +		encoder_.reset(new EncoderLibJpeg);

		encoder_ = std::make_unique<EncoderLibJpeg>();

This is functionally equivalent but more "C++".

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

> +}
> +
> +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
> +{
> +	if (encoder_)
> +		return encoder_->configure(cfg);
> +
> +	return 0;
>  }
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 07c714e3a365..d0dc40d81151 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -100,7 +100,7 @@ public:
>  		Mapped,
>  	};
>  	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> -		     Type type, unsigned int index, Encoder *encoder = nullptr);
> +		     Type type, unsigned int index);
>  
>  	const libcamera::PixelFormat &format() const { return format_; }
>  	const libcamera::Size &size() const { return size_; }
> @@ -108,6 +108,8 @@ public:
>  	unsigned int index() const { return index_; }
>  	Encoder *encoder() const { return encoder_.get(); }
>  
> +	int configure(const libcamera::StreamConfiguration &cfg);
> +
>  private:
>  	libcamera::PixelFormat format_;
>  	libcamera::Size size_;
Kieran Bingham Oct. 5, 2020, 4:18 p.m. UTC | #2 
Hi Jacopo

On 05/10/2020 12:26, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:37PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Delegate the construction of the encoder to the CameraStream class
>> for streams that need post-processing.
>>

Very happy to see this get pushed down to CameraStream ...
This series is getting me far too excited - and I'm only on patch 3!

:-)


>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 23 ++++++++++-------------
>>  src/android/camera_stream.cpp | 17 ++++++++++++++---
>>  src/android/camera_stream.h   |  4 +++-
>>  3 files changed, 27 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 0600ebc81c64..9c9a5cfa3c2f 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  
>>  		StreamConfiguration &cfg = config_->at(index);
>>  
>> -		/*
>> -		 * Construct a software encoder for the MJPEG streams from the
>> -		 * chosen libcamera source stream.
>> -		 */
>> -		Encoder *encoder = new EncoderLibJpeg();
>> -		int ret = encoder->configure(cfg);
>> -		if (ret) {
>> -			LOG(HAL, Error) << "Failed to configure encoder";
>> -			delete encoder;
>> -			return ret;
>> -		}
>> -
>> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
>> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
>>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>>  	}
>>  
>> @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * Configure the HAL CameraStream instances using the associated
>> +	 * StreamConfiguration and set the number of required buffers in
>> +	 * the Android camera3_stream_t.
>> +	 */
>>  	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());
>>  
>> +		int ret = cameraStream->configure(cfg);
>> +		if (ret)
>> +			return ret;
>> +
>>  		/* Use the bufferCount confirmed by the validation process. */
>>  		stream->max_buffers = cfg.bufferCount;
>>  	}
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 585bf2b68f4f..5b2b625c563b 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -7,10 +7,21 @@
>>  
>>  #include "camera_stream.h"
>>  
>> +#include "jpeg/encoder_libjpeg.h"
>> +
>>  using namespace libcamera;
>>  
>> -CameraStream::CameraStream(PixelFormat format, Size size,
>> -			   Type type, unsigned int index, Encoder *encoder)
>> -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
>> +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
>> +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
>>  {
>> +	if (type_ == Type::Internal || type_ == Type::Mapped)
>> +		encoder_.reset(new EncoderLibJpeg);
> 
> 		encoder_ = std::make_unique<EncoderLibJpeg>();
> 
> This is functionally equivalent but more "C++".
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +}
>> +
>> +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>> +{
>> +	if (encoder_)
>> +		return encoder_->configure(cfg);

I wonder if in the near future, the creation of the Encoder might happen
on demand here depending on what is needed to fulfil the
streamConfiguration, rather than being in the constructor.

But I'm not worried about that now, and I'm very pleased to see the
CameraStream specific code moving into here, so lets proceed!

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 07c714e3a365..d0dc40d81151 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -100,7 +100,7 @@ public:
>>  		Mapped,
>>  	};
>>  	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
>> -		     Type type, unsigned int index, Encoder *encoder = nullptr);
>> +		     Type type, unsigned int index);
>>  
>>  	const libcamera::PixelFormat &format() const { return format_; }
>>  	const libcamera::Size &size() const { return size_; }
>> @@ -108,6 +108,8 @@ public:
>>  	unsigned int index() const { return index_; }
>>  	Encoder *encoder() const { return encoder_.get(); }
>>  
>> +	int configure(const libcamera::StreamConfiguration &cfg);
>> +
>>  private:
>>  	libcamera::PixelFormat format_;
>>  	libcamera::Size size_;
>
Umang Jain Oct. 6, 2020, 11:07 a.m. UTC | #3 
Hi Jacopo,

On 10/5/20 4:16 PM, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> Delegate the construction of the encoder to the CameraStream class
> for streams that need post-processing.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   src/android/camera_device.cpp | 23 ++++++++++-------------
>   src/android/camera_stream.cpp | 17 ++++++++++++++---
>   src/android/camera_stream.h   |  4 +++-
>   3 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 0600ebc81c64..9c9a5cfa3c2f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   
>   		StreamConfiguration &cfg = config_->at(index);
>   
> -		/*
> -		 * Construct a software encoder for the MJPEG streams from the
> -		 * chosen libcamera source stream.
> -		 */
> -		Encoder *encoder = new EncoderLibJpeg();
> -		int ret = encoder->configure(cfg);
> -		if (ret) {
> -			LOG(HAL, Error) << "Failed to configure encoder";
> -			delete encoder;
> -			return ret;
> -		}
> -
> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
>   		jpegStream->priv = static_cast<void *>(&streams_.back());
>   	}
>   
> @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   		return -EINVAL;
>   	}
>   
> +	/*
> +	 * Configure the HAL CameraStream instances using the associated
> +	 * StreamConfiguration and set the number of required buffers in
> +	 * the Android camera3_stream_t.
> +	 */
>   	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());
>   
> +		int ret = cameraStream->configure(cfg);
> +		if (ret)
> +			return ret;
> +
>   		/* Use the bufferCount confirmed by the validation process. */
>   		stream->max_buffers = cfg.bufferCount;
>   	}
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 585bf2b68f4f..5b2b625c563b 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -7,10 +7,21 @@
>   
>   #include "camera_stream.h"
>   
> +#include "jpeg/encoder_libjpeg.h"
> +
>   using namespace libcamera;
>   
> -CameraStream::CameraStream(PixelFormat format, Size size,
> -			   Type type, unsigned int index, Encoder *encoder)
> -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
> +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
I think this should be s/encoder(nullptr)/encoder_(nullptr)/

Reviewed-by: Umang Jain <email@uajain.com>
>   {
> +	if (type_ == Type::Internal || type_ == Type::Mapped)
> +		encoder_.reset(new EncoderLibJpeg);
> +}
> +
> +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
> +{
> +	if (encoder_)
> +		return encoder_->configure(cfg);
> +
> +	return 0;
>   }
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 07c714e3a365..d0dc40d81151 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -100,7 +100,7 @@ public:
>   		Mapped,
>   	};
>   	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> -		     Type type, unsigned int index, Encoder *encoder = nullptr);
> +		     Type type, unsigned int index);
>   
>   	const libcamera::PixelFormat &format() const { return format_; }
>   	const libcamera::Size &size() const { return size_; }
> @@ -108,6 +108,8 @@ public:
>   	unsigned int index() const { return index_; }
>   	Encoder *encoder() const { return encoder_.get(); }
>   
> +	int configure(const libcamera::StreamConfiguration &cfg);
> +
>   private:
>   	libcamera::PixelFormat format_;
>   	libcamera::Size size_;
Jacopo Mondi Oct. 6, 2020, 11:21 a.m. UTC | #4 
Hi Umang,

On Tue, Oct 06, 2020 at 04:37:45PM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> On 10/5/20 4:16 PM, Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Delegate the construction of the encoder to the CameraStream class
> > for streams that need post-processing.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >   src/android/camera_device.cpp | 23 ++++++++++-------------
> >   src/android/camera_stream.cpp | 17 ++++++++++++++---
> >   src/android/camera_stream.h   |  4 +++-
> >   3 files changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 0600ebc81c64..9c9a5cfa3c2f 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >   		StreamConfiguration &cfg = config_->at(index);
> > -		/*
> > -		 * Construct a software encoder for the MJPEG streams from the
> > -		 * chosen libcamera source stream.
> > -		 */
> > -		Encoder *encoder = new EncoderLibJpeg();
> > -		int ret = encoder->configure(cfg);
> > -		if (ret) {
> > -			LOG(HAL, Error) << "Failed to configure encoder";
> > -			delete encoder;
> > -			return ret;
> > -		}
> > -
> > -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
> > +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> >   		jpegStream->priv = static_cast<void *>(&streams_.back());
> >   	}
> > @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >   		return -EINVAL;
> >   	}
> > +	/*
> > +	 * Configure the HAL CameraStream instances using the associated
> > +	 * StreamConfiguration and set the number of required buffers in
> > +	 * the Android camera3_stream_t.
> > +	 */
> >   	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());
> > +		int ret = cameraStream->configure(cfg);
> > +		if (ret)
> > +			return ret;
> > +
> >   		/* Use the bufferCount confirmed by the validation process. */
> >   		stream->max_buffers = cfg.bufferCount;
> >   	}
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 585bf2b68f4f..5b2b625c563b 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -7,10 +7,21 @@
> >   #include "camera_stream.h"
> > +#include "jpeg/encoder_libjpeg.h"
> > +
> >   using namespace libcamera;
> > -CameraStream::CameraStream(PixelFormat format, Size size,
> > -			   Type type, unsigned int index, Encoder *encoder)
> > -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
> > +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> > +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> I think this should be s/encoder(nullptr)/encoder_(nullptr)/
>

Very very correct indeed!
in V2 encoder_ stays wrapped in a unique_ptr<> so this will be gone,
but thanks for spotting this!

> Reviewed-by: Umang Jain <email@uajain.com>

Thanks
  j

> >   {
> > +	if (type_ == Type::Internal || type_ == Type::Mapped)
> > +		encoder_.reset(new EncoderLibJpeg);
> > +}
> > +
> > +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
> > +{
> > +	if (encoder_)
> > +		return encoder_->configure(cfg);
> > +
> > +	return 0;
> >   }
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 07c714e3a365..d0dc40d81151 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -100,7 +100,7 @@ public:
> >   		Mapped,
> >   	};
> >   	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > -		     Type type, unsigned int index, Encoder *encoder = nullptr);
> > +		     Type type, unsigned int index);
> >   	const libcamera::PixelFormat &format() const { return format_; }
> >   	const libcamera::Size &size() const { return size_; }
> > @@ -108,6 +108,8 @@ public:
> >   	unsigned int index() const { return index_; }
> >   	Encoder *encoder() const { return encoder_.get(); }
> > +	int configure(const libcamera::StreamConfiguration &cfg);
> > +
> >   private:
> >   	libcamera::PixelFormat format_;
> >   	libcamera::Size size_;
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Oct. 6, 2020, 12:44 p.m. UTC | #5 
Hi Kieran,

On Mon, Oct 05, 2020 at 05:18:31PM +0100, Kieran Bingham wrote:
> Hi Jacopo
>
> On 05/10/2020 12:26, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Mon, Oct 05, 2020 at 01:46:37PM +0300, Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Delegate the construction of the encoder to the CameraStream class
> >> for streams that need post-processing.
> >>
>
> Very happy to see this get pushed down to CameraStream ...
> This series is getting me far too excited - and I'm only on patch 3!
>
> :-)
>
>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/android/camera_device.cpp | 23 ++++++++++-------------
> >>  src/android/camera_stream.cpp | 17 ++++++++++++++---
> >>  src/android/camera_stream.h   |  4 +++-
> >>  3 files changed, 27 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index 0600ebc81c64..9c9a5cfa3c2f 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>
> >>  		StreamConfiguration &cfg = config_->at(index);
> >>
> >> -		/*
> >> -		 * Construct a software encoder for the MJPEG streams from the
> >> -		 * chosen libcamera source stream.
> >> -		 */
> >> -		Encoder *encoder = new EncoderLibJpeg();
> >> -		int ret = encoder->configure(cfg);
> >> -		if (ret) {
> >> -			LOG(HAL, Error) << "Failed to configure encoder";
> >> -			delete encoder;
> >> -			return ret;
> >> -		}
> >> -
> >> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
> >> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> >>  		jpegStream->priv = static_cast<void *>(&streams_.back());
> >>  	}
> >>
> >> @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	/*
> >> +	 * Configure the HAL CameraStream instances using the associated
> >> +	 * StreamConfiguration and set the number of required buffers in
> >> +	 * the Android camera3_stream_t.
> >> +	 */
> >>  	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());
> >>
> >> +		int ret = cameraStream->configure(cfg);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >>  		/* Use the bufferCount confirmed by the validation process. */
> >>  		stream->max_buffers = cfg.bufferCount;
> >>  	}
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index 585bf2b68f4f..5b2b625c563b 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -7,10 +7,21 @@
> >>
> >>  #include "camera_stream.h"
> >>
> >> +#include "jpeg/encoder_libjpeg.h"
> >> +
> >>  using namespace libcamera;
> >>
> >> -CameraStream::CameraStream(PixelFormat format, Size size,
> >> -			   Type type, unsigned int index, Encoder *encoder)
> >> -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
> >> +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> >> +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> >>  {
> >> +	if (type_ == Type::Internal || type_ == Type::Mapped)
> >> +		encoder_.reset(new EncoderLibJpeg);
> >
> > 		encoder_ = std::make_unique<EncoderLibJpeg>();
> >
> > This is functionally equivalent but more "C++".
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> >> +}
> >> +
> >> +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
> >> +{
> >> +	if (encoder_)
> >> +		return encoder_->configure(cfg);
>
> I wonder if in the near future, the creation of the Encoder might happen
> on demand here depending on what is needed to fulfil the
> streamConfiguration, rather than being in the constructor.
>
> But I'm not worried about that now, and I'm very pleased to see the
> CameraStream specific code moving into here, so lets proceed!

Right now the Encoder is constructed only for Type::Internal which is
assigned by the CameraDevice. It's a bit clunky, and my preference
would be a CameraStream subclass (maybe two, one that does encoding
and one that does allocation ?) Anyway, it's a bit over-engineerd for
the current status I think.

Thanks
  j

>
> 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 07c714e3a365..d0dc40d81151 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -100,7 +100,7 @@ public:
> >>  		Mapped,
> >>  	};
> >>  	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> >> -		     Type type, unsigned int index, Encoder *encoder = nullptr);
> >> +		     Type type, unsigned int index);
> >>
> >>  	const libcamera::PixelFormat &format() const { return format_; }
> >>  	const libcamera::Size &size() const { return size_; }
> >> @@ -108,6 +108,8 @@ public:
> >>  	unsigned int index() const { return index_; }
> >>  	Encoder *encoder() const { return encoder_.get(); }
> >>
> >> +	int configure(const libcamera::StreamConfiguration &cfg);
> >> +
> >>  private:
> >>  	libcamera::PixelFormat format_;
> >>  	libcamera::Size size_;
> >
>
> --
> Regards
> --
> Kieran
Kieran Bingham Oct. 6, 2020, 12:53 p.m. UTC | #6 
Hi Jacopo,

On 06/10/2020 13:44, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Oct 05, 2020 at 05:18:31PM +0100, Kieran Bingham wrote:
>> Hi Jacopo
>>
>> On 05/10/2020 12:26, Laurent Pinchart wrote:
>>> Hi Jacopo,
>>>
>>> Thank you for the patch.
>>>
>>> On Mon, Oct 05, 2020 at 01:46:37PM +0300, Laurent Pinchart wrote:
>>>> From: Jacopo Mondi <jacopo@jmondi.org>
>>>>
>>>> Delegate the construction of the encoder to the CameraStream class
>>>> for streams that need post-processing.
>>>>
>>
>> Very happy to see this get pushed down to CameraStream ...
>> This series is getting me far too excited - and I'm only on patch 3!
>>
>> :-)
>>
>>
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> ---
>>>>  src/android/camera_device.cpp | 23 ++++++++++-------------
>>>>  src/android/camera_stream.cpp | 17 ++++++++++++++---
>>>>  src/android/camera_stream.h   |  4 +++-
>>>>  3 files changed, 27 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>>> index 0600ebc81c64..9c9a5cfa3c2f 100644
>>>> --- a/src/android/camera_device.cpp
>>>> +++ b/src/android/camera_device.cpp
>>>> @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>>>
>>>>  		StreamConfiguration &cfg = config_->at(index);
>>>>
>>>> -		/*
>>>> -		 * Construct a software encoder for the MJPEG streams from the
>>>> -		 * chosen libcamera source stream.
>>>> -		 */
>>>> -		Encoder *encoder = new EncoderLibJpeg();
>>>> -		int ret = encoder->configure(cfg);
>>>> -		if (ret) {
>>>> -			LOG(HAL, Error) << "Failed to configure encoder";
>>>> -			delete encoder;
>>>> -			return ret;
>>>> -		}
>>>> -
>>>> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
>>>> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
>>>>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>>>>  	}
>>>>
>>>> @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>>>  		return -EINVAL;
>>>>  	}
>>>>
>>>> +	/*
>>>> +	 * Configure the HAL CameraStream instances using the associated
>>>> +	 * StreamConfiguration and set the number of required buffers in
>>>> +	 * the Android camera3_stream_t.
>>>> +	 */
>>>>  	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());
>>>>
>>>> +		int ret = cameraStream->configure(cfg);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>>  		/* Use the bufferCount confirmed by the validation process. */
>>>>  		stream->max_buffers = cfg.bufferCount;
>>>>  	}
>>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>>>> index 585bf2b68f4f..5b2b625c563b 100644
>>>> --- a/src/android/camera_stream.cpp
>>>> +++ b/src/android/camera_stream.cpp
>>>> @@ -7,10 +7,21 @@
>>>>
>>>>  #include "camera_stream.h"
>>>>
>>>> +#include "jpeg/encoder_libjpeg.h"
>>>> +
>>>>  using namespace libcamera;
>>>>
>>>> -CameraStream::CameraStream(PixelFormat format, Size size,
>>>> -			   Type type, unsigned int index, Encoder *encoder)
>>>> -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
>>>> +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
>>>> +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
>>>>  {
>>>> +	if (type_ == Type::Internal || type_ == Type::Mapped)
>>>> +		encoder_.reset(new EncoderLibJpeg);
>>>
>>> 		encoder_ = std::make_unique<EncoderLibJpeg>();
>>>
>>> This is functionally equivalent but more "C++".
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>>> +}
>>>> +
>>>> +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>>>> +{
>>>> +	if (encoder_)
>>>> +		return encoder_->configure(cfg);
>>
>> I wonder if in the near future, the creation of the Encoder might happen
>> on demand here depending on what is needed to fulfil the
>> streamConfiguration, rather than being in the constructor.
>>
>> But I'm not worried about that now, and I'm very pleased to see the
>> CameraStream specific code moving into here, so lets proceed!
> 
> Right now the Encoder is constructed only for Type::Internal which is
> assigned by the CameraDevice. It's a bit clunky, and my preference
> would be a CameraStream subclass (maybe two, one that does encoding
> and one that does allocation ?) Anyway, it's a bit over-engineerd for
> the current status I think.

I disagree to 'over-engineered' ... it's laying foundations for what we
need to do next (supporting YUVY->NV12, Rescaling, Encoding, Decoding...)

But no blockers here ;-) Moving on ...

> 
> Thanks
>   j
> 
>>
>> 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 07c714e3a365..d0dc40d81151 100644
>>>> --- a/src/android/camera_stream.h
>>>> +++ b/src/android/camera_stream.h
>>>> @@ -100,7 +100,7 @@ public:
>>>>  		Mapped,
>>>>  	};
>>>>  	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
>>>> -		     Type type, unsigned int index, Encoder *encoder = nullptr);
>>>> +		     Type type, unsigned int index);
>>>>
>>>>  	const libcamera::PixelFormat &format() const { return format_; }
>>>>  	const libcamera::Size &size() const { return size_; }
>>>> @@ -108,6 +108,8 @@ public:
>>>>  	unsigned int index() const { return index_; }
>>>>  	Encoder *encoder() const { return encoder_.get(); }
>>>>
>>>> +	int configure(const libcamera::StreamConfiguration &cfg);
>>>> +
>>>>  private:
>>>>  	libcamera::PixelFormat format_;
>>>>  	libcamera::Size size_;
>>>
>>
>> --
>> Regards
>> --
>> Kieran

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 0600ebc81c64..9c9a5cfa3c2f 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1273,19 +1273,7 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 
 		StreamConfiguration &cfg = config_->at(index);
 
-		/*
-		 * Construct a software encoder for the MJPEG streams from the
-		 * chosen libcamera source stream.
-		 */
-		Encoder *encoder = new EncoderLibJpeg();
-		int ret = encoder->configure(cfg);
-		if (ret) {
-			LOG(HAL, Error) << "Failed to configure encoder";
-			delete encoder;
-			return ret;
-		}
-
-		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
+		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
 		jpegStream->priv = static_cast<void *>(&streams_.back());
 	}
 
@@ -1306,11 +1294,20 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		return -EINVAL;
 	}
 
+	/*
+	 * Configure the HAL CameraStream instances using the associated
+	 * StreamConfiguration and set the number of required buffers in
+	 * the Android camera3_stream_t.
+	 */
 	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());
 
+		int ret = cameraStream->configure(cfg);
+		if (ret)
+			return ret;
+
 		/* Use the bufferCount confirmed by the validation process. */
 		stream->max_buffers = cfg.bufferCount;
 	}
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 585bf2b68f4f..5b2b625c563b 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -7,10 +7,21 @@ 
 
 #include "camera_stream.h"
 
+#include "jpeg/encoder_libjpeg.h"
+
 using namespace libcamera;
 
-CameraStream::CameraStream(PixelFormat format, Size size,
-			   Type type, unsigned int index, Encoder *encoder)
-	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
+CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
+	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
 {
+	if (type_ == Type::Internal || type_ == Type::Mapped)
+		encoder_.reset(new EncoderLibJpeg);
+}
+
+int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
+{
+	if (encoder_)
+		return encoder_->configure(cfg);
+
+	return 0;
 }
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index 07c714e3a365..d0dc40d81151 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -100,7 +100,7 @@  public:
 		Mapped,
 	};
 	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
-		     Type type, unsigned int index, Encoder *encoder = nullptr);
+		     Type type, unsigned int index);
 
 	const libcamera::PixelFormat &format() const { return format_; }
 	const libcamera::Size &size() const { return size_; }
@@ -108,6 +108,8 @@  public:
 	unsigned int index() const { return index_; }
 	Encoder *encoder() const { return encoder_.get(); }
 
+	int configure(const libcamera::StreamConfiguration &cfg);
+
 private:
 	libcamera::PixelFormat format_;
 	libcamera::Size size_;