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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran