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

[Jacopo Mondi]

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 at jmondi.org>
> >
> > Delegate the construction of the encoder to the CameraStream class
> > for streams that need post-processing.
> >
> > 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)
> 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel