[libcamera-devel] [PATCH v4 08/10] android: camera_device: Rework CameraStream handling

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sat, Sep 12, 2020 at 12:11:27PM +0200, Jacopo Mondi wrote:
> The CameraDevice::streams_ vector of CameraStream instances is
> currently mostly accessed by index. The current implementation
> creates all the CameraStream during the first loop that inspects the
> camera3_stream instances, and the update the index of the

s/the/then/

> StreamConfiguration associated with the CameraStream during a second
> loop that inspects MJPEG streams. A third loop creates the JPEG encoder
> associated with CameraStreams that produce MJPEG format.

s/CameraStreams/camera streams/ (or CameraStream instances)

> As the index-based association is hard to follow and rather fragile,
> rework the creation and handling of CameraStream:
> 
> 1) Make the StreamConfiguration index a constructor parameter and a
>    private struct member.  This disallow the creation of CameraStream

s/ This disallow/This disallows/

>    without a StreamConfiguration index assigned.
> 
> 2) Create CameraStream only after the associated StreamConfiguration
>    has been identified. The first loop creates CameraStream for non-JPEG
>    streams, the second for the JPEG ones after having identified the
>    associated StreamConfiguration. Since we have just created the
>    CameraStream, create the JPEG encoder at the same time instead of
>    deferring it.
> 
> This change removes all accesses by index to the CameraDevice::streams_
> vector.
> 
> No functional changes intended, but this change aims to make the code
> easier to follow and more robust.
> 
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
>  src/android/camera_device.h   | 18 ++++---
>  2 files changed, 57 insertions(+), 52 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index af2905007b28..59acfd762a89 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -169,8 +169,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  	}
>  }
>  
> -CameraStream::CameraStream(PixelFormat f, Size s)
> -	: index(-1), format(f), size(s), jpeg(nullptr)
> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> +	: format(f), size(s), jpeg(nullptr), index_(i)
>  {
>  }
>  
> @@ -1191,6 +1191,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	streams_.reserve(stream_list->num_streams);
>  
>  	/* First handle all non-MJPEG streams. */
> +	camera3_stream_t *jpegStream = nullptr;
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>  		Size size(stream->width, stream->height);
> @@ -1207,52 +1208,50 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		if (!format.isValid())
>  			return -EINVAL;
>  
> -		streams_.emplace_back(format, size);
> -		stream->priv = static_cast<void *>(&streams_[i]);
> -
>  		/* Defer handling of MJPEG streams until all others are known. */
> -		if (stream->format == HAL_PIXEL_FORMAT_BLOB)
> +		if (stream->format == HAL_PIXEL_FORMAT_BLOB) {

I would add

			if (jpegStream) {
				LOG(HAL, Error)
					<< "Multiple JPEG streams are not supported";
				return -EINVAL;
			}

or something along those lines.

> +			jpegStream = stream;
>  			continue;
> +		}
>  
>  		StreamConfiguration streamConfiguration;
> -
>  		streamConfiguration.size = size;
>  		streamConfiguration.pixelFormat = format;
>  
>  		config_->addConfiguration(streamConfiguration);
> -		streams_[i].index = config_->size() - 1;
> +		unsigned int index = config_->size() - 1;
> +		streams_.emplace_back(format, size, index);
> +		stream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
>  	/* Now handle MJPEG streams, adding a new stream if required. */

s/MJPEG streams/the MPJEG stream/

> -	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> -		camera3_stream_t *stream = stream_list->streams[i];
> -		bool match = false;
> -
> -		if (stream->format != HAL_PIXEL_FORMAT_BLOB)
> -			continue;
> +	if (jpegStream) {
> +		int index = -1;
>  
> -		/* Search for a compatible stream */
> -		for (unsigned int j = 0; j < config_->size(); j++) {
> -			StreamConfiguration &cfg = config_->at(j);
> +		/* Search for a compatible stream in the non-JPEG ones. */
> +		for (unsigned int i = 0; i < config_->size(); i++) {

I would have searched in streams_ instead of config_, but the result
should be equivalent.

> +			StreamConfiguration &cfg = config_->at(i);
>  
>  			/*
>  			 * \todo The PixelFormat must also be compatible with
>  			 * the encoder.
>  			 */
> -			if (cfg.size == streams_[i].size) {
> -				LOG(HAL, Info) << "Stream " << i
> -					       << " using libcamera stream " << j;
> +			if (cfg.size.width != jpegStream->width ||
> +			    cfg.size.height != jpegStream->height)
> +				continue;
>  
> -				match = true;
> -				streams_[i].index = j;
> -			}
> +			LOG(HAL, Info)
> +				<< "Android JPEG stream mapped on stream " << i;

s/on stream/to libcamera stream/

> +
> +			index = i;
> +			break;
>  		}
>  
>  		/*
>  		 * Without a compatible match for JPEG encoding we must
>  		 * introduce a new stream to satisfy the request requirements.
>  		 */
> -		if (!match) {
> +		if (index < 0) {
>  			StreamConfiguration streamConfiguration;
>  
>  			/*
> @@ -1261,15 +1260,31 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			 * handled, and should be considered as part of any
>  			 * stream configuration reworks.
>  			 */
> -			streamConfiguration.size.width = stream->width;
> -			streamConfiguration.size.height = stream->height;
> +			streamConfiguration.size.width = jpegStream->width;
> +			streamConfiguration.size.height = jpegStream->height;
>  			streamConfiguration.pixelFormat = formats::NV12;
>  
>  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>  				       << " for MJPEG support";
>  
>  			config_->addConfiguration(streamConfiguration);
> -			streams_[i].index = config_->size() - 1;
> +			index = config_->size() - 1;
> +		}
> +
> +		StreamConfiguration &cfg = config_->at(index);
> +		CameraStream &cameraStream =
> +			streams_.emplace_back(formats::MJPEG, cfg.size, index);
> +		jpegStream->priv = static_cast<void *>(&cameraStream);
> +
> +		/*
> +		 * Construct a software encoder for MJPEG streams from the

s/MJPEG streams/the MJPEG stream/

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

> +		 * chosen libcamera source stream.
> +		 */
> +		cameraStream.jpeg = new EncoderLibJpeg();
> +		int ret = cameraStream.jpeg->configure(cfg);
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to configure encoder";
> +			return ret;
>  		}
>  	}
>  
> @@ -1292,25 +1307,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
> -		CameraStream *cameraStream = &streams_[i];
> -		StreamConfiguration &cfg = config_->at(cameraStream->index);
> +		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
> +		StreamConfiguration &cfg = config_->at(cameraStream->index());
>  
>  		/* Use the bufferCount confirmed by the validation process. */
>  		stream->max_buffers = cfg.bufferCount;
> -
> -		/*
> -		 * Construct a software encoder for MJPEG streams from the
> -		 * chosen libcamera source stream.
> -		 */
> -		if (cameraStream->format == formats::MJPEG) {
> -			cameraStream->jpeg = new EncoderLibJpeg();
> -			int ret = cameraStream->jpeg->configure(cfg);
> -			if (ret) {
> -				LOG(HAL, Error)
> -					<< "Failed to configure encoder";
> -				return ret;
> -			}
> -		}
>  	}
>  
>  	/*
> @@ -1424,7 +1425,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		}
>  		descriptor->frameBuffers.emplace_back(buffer);
>  
> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>  		Stream *stream = streamConfiguration->stream();
>  
>  		request->addBuffer(stream, buffer);
> @@ -1479,7 +1480,7 @@ void CameraDevice::requestComplete(Request *request)
>  			continue;
>  		}
>  
> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>  		Stream *stream = streamConfiguration->stream();
>  		FrameBuffer *buffer = request->findBuffer(stream);
>  		if (!buffer) {
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index c0732fb8ed7f..376d001ea7d7 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -28,20 +28,24 @@
>  class CameraMetadata;
>  
>  struct CameraStream {
> -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> +public:
> +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
>  	~CameraStream();
>  
> -	/*
> -	 * The index of the libcamera StreamConfiguration as added during
> -	 * configureStreams(). A single libcamera Stream may be used to deliver
> -	 * one or more streams to the Android framework.
> -	 */
> -	unsigned int index;
> +	unsigned int index() const { return index_; }
>  
>  	libcamera::PixelFormat format;
>  	libcamera::Size size;
>  
>  	Encoder *jpeg;
> +
> +private:
> +	/*
> +	 * The index of the libcamera StreamConfiguration as added during
> +	 * configureStreams(). A single libcamera Stream may be used to deliver
> +	 * one or more streams to the Android framework.
> +	 */
> +	unsigned int index_;
>  };
>  
>  class CameraDevice : protected libcamera::Loggable

-- 
Regards,

Laurent Pinchart