[libcamera-devel] [RFC 5/7] android: camera_device: Rework CameraStream handling

[Kieran Bingham]

Hi Jacopo,

On 02/09/2020 14:08, 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:

I recall it was necessary to split operations between things
pre-validate, and post-validate though..


> 
> 1) Make the StreamConfiguration index a constructor parameter and a
>    private struct member.  This disallow the creation of CameraStream
>    without a StreamConfiguration index assigned.

I think your earlier index patches now make that possible (so I like this)

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

Sounds good - Need to check in detail, as earlier this is where I
thought I saw a key functional change... diggging....

Nope - the change I had in mind was good...

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

I really like this.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/android/camera_device.cpp | 73 +++++++++++++++++++----------------
>  src/android/camera_device.h   | 18 +++++----
>  2 files changed, 50 insertions(+), 41 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a917404016e7..15dc12556cf5 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);

ah yes, this is better indeed.

That index from addConfiguration really helps out.


>  	}
>  
>  	/* 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;
>  
> -		/* 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
> +				       << " using libcamera stream " << j;
> +
> +			index = j;
> +			match = true;
>  		}
>  
>  		/*


Sigh - this is where I hate e-mail based reviews. This is missing what
would be reallly helpful context right here ;-)


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

I think your previous patches have rendered this format field unused?

(But I 'like' having it ..., and I think it still has use/value when we
deal with other stream conversions later ... we'll see)

> +		Size size = cfg.size;
> +
> +		CameraStream &cameraStream = streams_.emplace_back(format, 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";
> +			return ret;

Nice, I'm not sure why I had the encoder so late before, but I think
it's fine/better here.

Maybe it was so we only created the encoders after we've validated the
configurations ?

But maybe it's not a problem though.



>  		}
>  	}
>  
> @@ -1296,24 +1315,10 @@ 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);
> +		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 230e89b011e6..975c312c1c12 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
--
Kieran