[libcamera-devel] [PATCH v2 6/9] android: camera_device: Maintain a vector of CameraStream

[Jacopo Mondi]

Hi Kieran,

On Thu, Jul 02, 2020 at 10:36:51PM +0100, Kieran Bingham wrote:
> Introduce a vector storing a CameraStream to track and maintain
> state between an Android stream (camera3_stream_t) and a libcamera
> Stream.
>
> Only the index of the libcamera stream is stored, to facilitate identifying
> the correct index for both the StreamConfiguration and Stream vectors.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 18 ++++++++++++++++--
>  src/android/camera_device.h   |  6 ++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 77083219d8a1..fc3962dac230 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -952,6 +952,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>
> +	streams_.reserve(stream_list->num_streams);
> +
> +	/*
> +	 * Track actually created streams, as there may not be a 1:1 mapping of
> +	 * camera3 streams to libcamera streams.
> +	 */
> +	unsigned int streamIndex = 0;
> +

I would drop this one

>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>
> @@ -967,6 +975,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		if (!format.isValid())
>  			return -EINVAL;
>
> +		/* Maintain internal state of all stream mappings. */
> +		streams_[i].androidStream = stream;
> +

Am I mistaken, or looking at the following patches, this is not used ?

>  		StreamConfiguration streamConfiguration;
>
>  		streamConfiguration.size.width = stream->width;
> @@ -974,6 +985,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfiguration.pixelFormat = format;
>
>  		config_->addConfiguration(streamConfiguration);
> +		streams_[i].libcameraIndex = streamIndex++;

In that case and the androidStream field is not used, we would just
need to store a pointer to the StreamConfiguration associated to an
android stream, don't we ?

>  	}
>
>  	switch (config_->validate()) {
> @@ -991,10 +1003,12 @@ 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];
> -		StreamConfiguration &streamConfiguration = config_->at(i);
> +		CameraStream *cameraStream = &streams_[i];
> +
> +		StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex);
>
>  		/* Use the bufferCount confirmed by the validation process. */
> -		stream->max_buffers = streamConfiguration.bufferCount;
> +		stream->max_buffers = cfg.bufferCount;

I'm not sure I get the purpose of this hunk.

If you're preparing to have less StreamConfiguration than android
streams (as some streams, as JPEG one, might be hal-only streams),
why don't you just iterate the camera configuration, as the only
purpose here is to collect the maximum number of buffers ?


>  	}
>
>  	/*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 5bd6cf416156..275760f0aa26 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -25,6 +25,11 @@
>
>  class CameraMetadata;
>
> +struct CameraStream {
> +	camera3_stream *androidStream;
> +	unsigned int libcameraIndex;
> +};
> +
>  class CameraDevice : protected libcamera::Loggable
>  {
>  public:
> @@ -89,6 +94,7 @@ private:
>
>  	std::vector<Camera3StreamConfiguration> streamConfigurations_;
>  	std::map<int, libcamera::PixelFormat> formatsMap_;
> +	std::vector<CameraStream> streams_;
>
>  	int facing_;
>  	int orientation_;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel