[libcamera-devel] [PATCH v2 10/12] android: camera_device: Rework CameraStream handling

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 02, 2020 at 05:22:34PM +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
> 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.
> 
> 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
>    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: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 75 +++++++++++++++++++----------------
>  src/android/camera_device.h   | 18 +++++----
>  2 files changed, 51 insertions(+), 42 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3630e87e8814..9bcd1d993c17 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,8 +168,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)
>  {
>  }
>  
> @@ -1212,30 +1212,28 @@ 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)
>  			continue;
>  
>  		StreamConfiguration streamConfiguration;
> -
>  		streamConfiguration.size = size;
>  		streamConfiguration.pixelFormat = format;
>  
> -		streams_[i].index = config_->addConfiguration(streamConfiguration);
> +		unsigned int index = config_->addConfiguration(streamConfiguration);
> +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> +		stream->priv = static_cast<void *>(&cameraStream);

This is problematic. std::vector::emplace_back may cause a reallocation,
which invalidates all iterators, rendering the pointer invalid. One
option would be to turn streams_ into an std::list, another option to
fill the priv pointers in a separate loop after populating streams_.

>  	}
>  
>  	/* Now handle MJPEG streams, adding a new stream if required. */
>  	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;

Drive-by comment for a possible future simplification, can't we rely on
the fact that only a single JPEG stream will be requested ? If so, we
could store a pointer to the JPEG camera3_stream_configuration_t in the
previous loop, and have a

	if (jpegStream) {
		...
	}

here instead of a loop.

>  
> -		/* Search for a compatible stream */
> +		/* Search for a compatible stream in the non-JPEG ones. */
> +		bool match = false;
> +		unsigned int index;
>  		for (unsigned int j = 0; j < config_->size(); j++) {
>  			StreamConfiguration &cfg = config_->at(j);
>  
> @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			 * \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 != stream->width ||
> +			    cfg.size.height != stream->height)
> +				continue;
>  
> -				match = true;
> -				streams_[i].index = j;
> -			}
> +			LOG(HAL, Info) << "Stream " << i

s/Stream/Android stream/ ?

> +				       << " using libcamera stream " << j;
> +
> +			index = j;
> +			match = true;

Should you break ?

>  		}
>  
>  		/*
> @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>  				       << " for MJPEG support";
>  
> -			streams_[i].index = config_->addConfiguration(streamConfiguration);
> +			index = config_->addConfiguration(streamConfiguration);
> +		}
> +
> +		StreamConfiguration &cfg = config_->at(index);
> +		PixelFormat format = formats::MJPEG;
> +		Size size = cfg.size;
> +
> +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);

No need for the format and size local variables, you can write

		CameraStream &cameraStream =
			streams_.emplace_back(formats::MJPEG, cfg.size, index);

> +		stream->priv = static_cast<void *>(&cameraStream);
> +
> +		/*
> +		 * Construct a software encoder for MJPEG streams from the
> +		 * chosen libcamera source stream.
> +		 */
> +		cameraStream.jpeg = new EncoderLibJpeg();
> +		int ret = cameraStream.jpeg->configure(cfg);
> +		if (ret) {
> +			LOG(HAL, Error)
> +				<< "Failed to configure encoder";

This holds on a single line.

			LOG(HAL, Error) << "Failed to configure encoder";

> +			return ret;
>  		}
>  	}
>  
> @@ -1295,25 +1314,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;
> -			}
> -		}
>  	}
>  
>  	/*
> @@ -1427,7 +1432,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);
> @@ -1482,7 +1487,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 dc0ee664d443..f8f237203ce9 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -28,20 +28,24 @@
>  class CameraMetadata;
>  
>  struct CameraStream {

Can you turn it into a class while at it ?

Also clearly not a candidate for this patch, do you think it would be
useful to rename the HAL-related classes with a HAL prefix (e.g.
HALCamera instead of CameraDevice, HALStream instead of CameraStream) to
avoid confusing them with the libcamera classes ? We're familiar with
this code so it's probably not too confusing for us, but it may be for a
newcomer.

> -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> +public:
> +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);

i is not a very clear name, index would be better. I would also name the
existing parameters (that could be a separate change).

>  	~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.
> +	 */

Should this be moved to the .cpp file ? Documentation rules are a bit
more relaxed here though, as it's internal doc.

> +	unsigned int index_;
>  };
>  
>  class CameraDevice : protected libcamera::Loggable

-- 
Regards,

Laurent Pinchart