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

[Jacopo Mondi]

Hi Kieran
   sorry, I'm going to be picky.

On Fri, Jul 03, 2020 at 01:39:17PM +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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 22 ++++++++++++++++++++--
>  src/android/camera_device.h   | 10 ++++++++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4e77a92dbea5..28334751a26e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -952,6 +952,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>
> +	/*
> +	 * Clear and remove any existing configuration from previous calls, and
> +	 * ensure the required entries are available without further
> +	 * re-allcoation.
> +	 */
> +	streams_.clear();
> +	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;
> +
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>
> @@ -974,6 +988,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfiguration.pixelFormat = format;
>
>  		config_->addConfiguration(streamConfiguration);
> +
> +		/* Maintain internal state of all stream mappings. */

I don't want to be rude, but these are the comments I would prefer not
to see in code, and I'm the first one with a tendency to over-comment
trivial operations (like recording an index) with comments that might
even be misleading.  "maintain the internal state of all stream
mappings" makes me look around for some kind of 'internal state' until
I don't realize we're just recording an index for later use. I would
prefer to know where the index is use and why, with a slightly longer:

                /*
                 * Record the index of the StreamConfiguration
                 * associated to the camera3 stream. As not all
                 * camera3 streams maps to a libcamera stream, the
                 * index might be repeated.
                 */

> +		streams_[i].index = streamIndex++;
>  	}
>
>  	switch (config_->validate()) {
> @@ -991,10 +1008,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];
> -		StreamConfiguration &streamConfiguration = config_->at(i);
> +		CameraStream *cameraStream = &streams_[i];
> +		StreamConfiguration &cfg = config_->at(cameraStream->index);
>
>  		/* Use the bufferCount confirmed by the validation process. */
> -		stream->max_buffers = streamConfiguration.bufferCount;
> +		stream->max_buffers = cfg.bufferCount;
>  	}
>
>  	/*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index d7834d94f439..8e306c1f6d8b 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -25,6 +25,15 @@
>
>  class CameraMetadata;
>
> +struct CameraStream {
> +	/*
> +	 * The index represents the index for libcamera stream parameters. This

What are "stream parameters" ?

> +	 * will differ from any index of the halStream, particularly for HAL

I'm not sure I get what "from any index of the halStream" means.
I also think you removed 'halStream' from the patch

I would:
"Index of the libcamera StreamConfiguration associated with an Android
requested stream. Not all streams requested by the Android framework
are 1-to-1 mapped to a libcamera Stream as some of them, such as JPEG,
might be produced by the libcamera HAL by processing data produced by
an existing libcamera Stream. This implies different Android stream
might be associated with the same libcamera StreamConfiguration index."

Feel free to adjust if you think it's worth taking this in.

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

Thanks
  j


> +	 * only streams such as MJPEG.
> +	 */
> +	unsigned int index;
> +};
> +
>  class CameraDevice : protected libcamera::Loggable
>  {
>  public:
> @@ -90,6 +99,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