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

8586 diff mbox series

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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 22 ++++++++++++++++++++--
 src/android/camera_device.h   | 10 ++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

Hi Kieran,

Thanks for your work.

On 2020-07-03 13:39:17 +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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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. */
> +		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
> +	 * will differ from any index of the halStream, particularly for HAL
> +	 * 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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On 06/07/2020 09:22, Jacopo Mondi wrote:
> 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@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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


Don't worry - it's fine. I normally write comments before code, so
sometimes indeed I'll end up with possibly inappropriate comments left
that I haven't yet removed.

And in this series, these code blocks have bounced around so much in the
patches they might think they're on a trampoline ;)


Indeed, this code block started out with storing much more content and
internal state for JPEG, which has then been simplified to try to get
this multi-stream series separated...


> 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:

It's been simplified to a single index (for now).


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

Isn't that what's stated above at declaration of streamIndex?

The comment for /* maintain internal state of all stream mappings */
helps later on when there are several lines here, and it becomes a
distinct code block, but it has been simplified to a single line for
this patch.

I'll just remove the line, there's no need to add a big block again. The
single line was more of a separator...


And in fact, I later found out I had to split the block anyway, as I can
only increment the index after adding the Configuration, where JPEG
streams that don't create a configuration break out of the loop earlier,
so they have to update their internal streams_[i] structure before.


>> +		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" ?

StreamConfigurations, and Streams ... but I think actually the Stream*
is obtained from the StreamConfiguration anyway, so really it's just the
StreamConfigurations

> 
>> +	 * 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

Yes, I have now removed halStream, and I don't think I'll add it back
later any more.


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

I've changed this to:

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

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

Thanks,


> 
> 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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

On Mon, Jul 06, 2020 at 02:05:04PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/07/2020 09:22, Jacopo Mondi wrote:
> > 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@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
>
>
> Don't worry - it's fine. I normally write comments before code, so
> sometimes indeed I'll end up with possibly inappropriate comments left
> that I haven't yet removed.
>
> And in this series, these code blocks have bounced around so much in the
> patches they might think they're on a trampoline ;)
>
>
> Indeed, this code block started out with storing much more content and
> internal state for JPEG, which has then been simplified to try to get
> this multi-stream series separated...
>
>
> > 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:
>
> It's been simplified to a single index (for now).
>
>
> >
> >                 /*
> >                  * 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.
> >                  */
>
> Isn't that what's stated above at declaration of streamIndex?
>
> The comment for /* maintain internal state of all stream mappings */
> helps later on when there are several lines here, and it becomes a
> distinct code block, but it has been simplified to a single line for
> this patch.
>
> I'll just remove the line, there's no need to add a big block again. The
> single line was more of a separator...
>
>
> And in fact, I later found out I had to split the block anyway, as I can
> only increment the index after adding the Configuration, where JPEG
> streams that don't create a configuration break out of the loop earlier,
> so they have to update their internal streams_[i] structure before.
>
>
> >> +		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" ?
>
> StreamConfigurations, and Streams ... but I think actually the Stream*
> is obtained from the StreamConfiguration anyway, so really it's just the
> StreamConfigurations
>
> >
> >> +	 * 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
>
> Yes, I have now removed halStream, and I don't think I'll add it back
> later any more.
>
>
> > 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.
>
> I've changed this to:
>
>  /*
>   * 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.
>   */

Sounds perfect!
Thanks

>
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks,
>
>
> >
> > 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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran