[libcamera-devel] [PATCH 03/15] android: camera_stream: Delegate Encoder construction

[Jacopo Mondi]

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 at 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 at 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 at 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 at 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