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

Message ID 20200702213654.2129054-7-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • android: Multi-stream support
Related show

Commit Message

Kieran Bingham July 2, 2020, 9:36 p.m. UTC
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>
---
 src/android/camera_device.cpp | 18 ++++++++++++++++--
 src/android/camera_device.h   |  6 ++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

Comments

Niklas Söderlund July 2, 2020, 11:31 p.m. UTC | #1
Hi Kieran,

Thanks for your work.

On 2020-07-02 22:36:51 +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>
> ---
>  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);

Should you not also empty the vector before reserving memory? I'm 
thinking of if something below fails and exits we could be stuch with a 
streams_ that is half of B and half of A. Or maybe streams_ should be 
emptied on error instead?

> +
> +	/*
> +	 * 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];
>  
> @@ -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;
> +
>  		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++;
>  	}
>  
>  	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];
> +

nit: I would drop the extra newline, with or without fixed,

> +		StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex);
>  
>  		/* 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 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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 3, 2020, 12:37 a.m. UTC | #2
Hi Kieran,

Thank you for the patch.

On Fri, Jul 03, 2020 at 01:31:09AM +0200, Niklas Söderlund wrote:
> On 2020-07-02 22:36:51 +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>
> > ---
> >  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);
> 
> Should you not also empty the vector before reserving memory? I'm 
> thinking of if something below fails and exits we could be stuch with a 
> streams_ that is half of B and half of A. Or maybe streams_ should be 
> emptied on error instead?

reserve() will only reserve memory to avoid a memory allocation for
every entry added to the vector, entries are still populated when adding
them. Said different, reserve() doesn't change size().

> > +
> > +	/*
> > +	 * 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];
> >  
> > @@ -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;
> > +
> >  		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++;
> >  	}
> >  
> >  	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];
> > +
> 
> nit: I would drop the extra newline, with or without fixed,
> 
> > +		StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex);
> >  
> >  		/* 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 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;

I'd name this halStream, the rationale being that prefixing names with
hal instead of android will keep them shorter.

> > +	unsigned int libcameraIndex;

And I'd simply use index here (no prefix for the libcamera side), but I
won't insist too much. The rationale is that otherwise I fear a
libcamera prefix will spread in many places. A short comment along the
lines of "The index of the stream in the CameraConfiguration" could also
possibly be useful.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +};
> > +
> >  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_;
Jacopo Mondi July 3, 2020, 9:35 a.m. UTC | #3
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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham July 3, 2020, 9:49 a.m. UTC | #4
Hi all,

On 03/07/2020 01:37, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Jul 03, 2020 at 01:31:09AM +0200, Niklas Söderlund wrote:
>> On 2020-07-02 22:36:51 +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>
>>> ---
>>>  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);
>>
>> Should you not also empty the vector before reserving memory? I'm 
>> thinking of if something below fails and exits we could be stuch with a 
>> streams_ that is half of B and half of A. Or maybe streams_ should be 
>> emptied on error instead?
> 
> reserve() will only reserve memory to avoid a memory allocation for
> every entry added to the vector, entries are still populated when adding
> them. Said different, reserve() doesn't change size().
> 
>>> +
>>> +	/*
>>> +	 * 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];
>>>  
>>> @@ -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;
>>> +
>>>  		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++;
>>>  	}
>>>  
>>>  	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];
>>> +
>>
>> nit: I would drop the extra newline, with or without fixed,

Niklas, was there a tag to go in here? :-D

>>
>>> +		StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex);
>>>  
>>>  		/* 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 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;
> 
> I'd name this halStream, the rationale being that prefixing names with
> hal instead of android will keep them shorter.

I like halStream

> 
>>> +	unsigned int libcameraIndex;
> 
> And I'd simply use index here (no prefix for the libcamera side), but I
> won't insist too much. The rationale is that otherwise I fear a
> libcamera prefix will spread in many places. A short comment along the
> lines of "The index of the stream in the CameraConfiguration" could also
> possibly be useful.


And I'm happy with just index too, but it does /require/ the comment
then, because the index is not required to match the index of the halStream.

I'll update both.


> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks,


> 
>>> +};
>>> +
>>>  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_;
>
Kieran Bingham July 3, 2020, 9:55 a.m. UTC | #5
Hi Jacopo,

On 03/07/2020 10:35, Jacopo Mondi wrote:
> 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@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

Drop the newline? Or something else?

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


No, we can't store a pointer to the StreamConfiguration, because it's
not valid to do so.


At this point, we have 'added' the configuration to the config_, but it
makes a copy, so /this/ streamConfiguration is the wrong pointer to store.

Further more, it could then be suggested that we just obtain the
'correct' pointer - by using config_->at(n);.

But we can't do that either, because we are in a loop, adding configs to
a vector, and the vector can re-allocate - so the pointers could change.

Thus, I am storing an index.


Should that be added in a comment or is it ok?



> 
>>  	}
>>
>>  	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 ?

Because even the HAL only streams need to have the stream->max_buffers
populated ? In the case of a hal-only stream, it gets populated with the
max_buffers of the relevant /source/ stream which will feed that hal
only stream:

 Stream #1 YUV : MaxBuffers=4
 Stream #2 MJPEG : MaxBuffers=maxBuffersOfStream(1);

If we iterate over the camera configuration, we would not set the
max_buffers for the hal-only streams, nor perform any other
per-android-stream actions that may be required post-validation.


> 
> 
>>  	}
>>
>>  	/*
>> 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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi July 3, 2020, 10:30 a.m. UTC | #6
Hi Kieran

On Fri, Jul 03, 2020 at 10:55:22AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 03/07/2020 10:35, Jacopo Mondi wrote:
> > 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@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
>
> Drop the newline? Or something else?
>

yeah, new line

> >
> >>  	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 ?
>
>
> No, we can't store a pointer to the StreamConfiguration, because it's
> not valid to do so.
>
>
> At this point, we have 'added' the configuration to the config_, but it
> makes a copy, so /this/ streamConfiguration is the wrong pointer to store.
>

Right, indeed stream configurations are copied into the camera.
Didn't you have a patch to make CameraConfiguration::addConfiguration
return a pointer to the stored StreamConfiguration ?


> Further more, it could then be suggested that we just obtain the
> 'correct' pointer - by using config_->at(n);.
>
> But we can't do that either, because we are in a loop, adding configs to
> a vector, and the vector can re-allocate - so the pointers could change.
>
> Thus, I am storing an index.

Makes sense, then please ignore my comment.

>
>
> Should that be added in a comment or is it ok?

If you can capture this in a few lines, why not.

My question on the usage of .androidStream remains though

>
>
>
> >
> >>  	}
> >>
> >>  	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 ?
>
> Because even the HAL only streams need to have the stream->max_buffers
> populated ? In the case of a hal-only stream, it gets populated with the
> max_buffers of the relevant /source/ stream which will feed that hal
> only stream:
>
>  Stream #1 YUV : MaxBuffers=4
>  Stream #2 MJPEG : MaxBuffers=maxBuffersOfStream(1);
>
> If we iterate over the camera configuration, we would not set the
> max_buffers for the hal-only streams, nor perform any other
> per-android-stream actions that may be required post-validation.

Right, we need to iterate over all the android provided streams to
fill their max_buffers field.

Now that I look at the code again, I see that in the first loop we store
an increasing streamIndex, which is exactly equal to i.


	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
                ...

		config_->addConfiguration(streamConfiguration);
		streams_[i].libcameraIndex = streamIndex++;
	}

        ...

In the second loop we use that index, which is exactly equal to the
loop counter

	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->libcameraIndex);

		/* Use the bufferCount confirmed by the validation process. */
		stream->max_buffers = cfg.bufferCount;
	}

Re-phrasing: why do you need that libcamerIdex at all now ?

How do you plan to map HAL-only streams to the streamIndex ? There
will be one index in the stream_ vector for each of the android
stream, will some entries be repeated ? As HAL-only streams will
'point' to the StreamConfiguration of the libcamera Stream that feeds
them ?

>
>
> >
> >
> >>  	}
> >>
> >>  	/*
> >> 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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran
Kieran Bingham July 3, 2020, 10:36 a.m. UTC | #7
Hi Jacopo,

On 03/07/2020 11:30, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Fri, Jul 03, 2020 at 10:55:22AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 03/07/2020 10:35, Jacopo Mondi wrote:
>>> 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@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
>>
>> Drop the newline? Or something else?
>>
> 
> yeah, new line


Can I disagree here? - I put that there intentionally to not 'hide' the
streamIndex variable against the for loop, and not associate the comment
added with it to the loop.

The comment applies to the variable, not the loop.


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


Hrm, you might be right. Perhaps maintaining the correct indexing for
the streams_[] to camera3_stream_t, and (later) putting the pointer in
is enough.

Wow - this struct is certainly getting small for now then, just storing
a single index ...

And yet, I still want to make it a class... hrm ... maybe class can come
later.



>>>
>>>>  		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 ?
>>
>>
>> No, we can't store a pointer to the StreamConfiguration, because it's
>> not valid to do so.
>>
>>
>> At this point, we have 'added' the configuration to the config_, but it
>> makes a copy, so /this/ streamConfiguration is the wrong pointer to store.
>>
> 
> Right, indeed stream configurations are copied into the camera.
> Didn't you have a patch to make CameraConfiguration::addConfiguration
> return a pointer to the stored StreamConfiguration ?
> 
> 
>> Further more, it could then be suggested that we just obtain the
>> 'correct' pointer - by using config_->at(n);.
>>
>> But we can't do that either, because we are in a loop, adding configs to
>> a vector, and the vector can re-allocate - so the pointers could change.
>>
>> Thus, I am storing an index.
> 
> Makes sense, then please ignore my comment.
> 
>>
>>
>> Should that be added in a comment or is it ok?
> 
> If you can capture this in a few lines, why not.
> 
> My question on the usage of .androidStream remains though

I could store just the index in stream->priv as I think Laurent
suggested (with casting) at this stage, and later add a struct, but I
think I'll keep the struct for now.

--
Kieran


>>>>  	}
>>>>
>>>>  	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 ?
>>
>> Because even the HAL only streams need to have the stream->max_buffers
>> populated ? In the case of a hal-only stream, it gets populated with the
>> max_buffers of the relevant /source/ stream which will feed that hal
>> only stream:
>>
>>  Stream #1 YUV : MaxBuffers=4
>>  Stream #2 MJPEG : MaxBuffers=maxBuffersOfStream(1);
>>
>> If we iterate over the camera configuration, we would not set the
>> max_buffers for the hal-only streams, nor perform any other
>> per-android-stream actions that may be required post-validation.
> 
> Right, we need to iterate over all the android provided streams to
> fill their max_buffers field.
> 
> Now that I look at the code again, I see that in the first loop we store
> an increasing streamIndex, which is exactly equal to i.
> 
> 
> 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>                 ...
> 
> 		config_->addConfiguration(streamConfiguration);
> 		streams_[i].libcameraIndex = streamIndex++;
> 	}
> 
>         ...
> 
> In the second loop we use that index, which is exactly equal to the
> loop counter
> 
> 	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->libcameraIndex);
> 
> 		/* Use the bufferCount confirmed by the validation process. */
> 		stream->max_buffers = cfg.bufferCount;
> 	}
> 
> Re-phrasing: why do you need that libcamerIdex at all now ?
> 
> How do you plan to map HAL-only streams to the streamIndex ? There
> will be one index in the stream_ vector for each of the android
> stream, will some entries be repeated ? As HAL-only streams will
> 'point' to the StreamConfiguration of the libcamera Stream that feeds
> them ?
> 
>>
>>
>>>
>>>
>>>>  	}
>>>>
>>>>  	/*
>>>> 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@lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran
Jacopo Mondi July 3, 2020, 10:47 a.m. UTC | #8
Hi Kieran

On Fri, Jul 03, 2020 at 11:36:56AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 03/07/2020 11:30, Jacopo Mondi wrote:
> > Hi Kieran
> >
> > On Fri, Jul 03, 2020 at 10:55:22AM +0100, Kieran Bingham wrote:
> >> Hi Jacopo,
> >>
> >> On 03/07/2020 10:35, Jacopo Mondi wrote:
> >>> 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@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
> >>
> >> Drop the newline? Or something else?
> >>
> >
> > yeah, new line
>
>
> Can I disagree here? - I put that there intentionally to not 'hide' the
> streamIndex variable against the for loop, and not associate the comment
> added with it to the loop.
>
> The comment applies to the variable, not the loop.

Your code, your choice :D

>
>
> >
> >>>
> >>>>  	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 ?
>
>
> Hrm, you might be right. Perhaps maintaining the correct indexing for
> the streams_[] to camera3_stream_t, and (later) putting the pointer in
> is enough.
>
> Wow - this struct is certainly getting small for now then, just storing
> a single index ...
>
> And yet, I still want to make it a class... hrm ... maybe class can come
> later.

Surely a class for single unsigned int would be 'slightly' and
overkill :)

>
>
>
> >>>
> >>>>  		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 ?
> >>
> >>
> >> No, we can't store a pointer to the StreamConfiguration, because it's
> >> not valid to do so.
> >>
> >>
> >> At this point, we have 'added' the configuration to the config_, but it
> >> makes a copy, so /this/ streamConfiguration is the wrong pointer to store.
> >>
> >
> > Right, indeed stream configurations are copied into the camera.
> > Didn't you have a patch to make CameraConfiguration::addConfiguration
> > return a pointer to the stored StreamConfiguration ?
> >
> >
> >> Further more, it could then be suggested that we just obtain the
> >> 'correct' pointer - by using config_->at(n);.
> >>
> >> But we can't do that either, because we are in a loop, adding configs to
> >> a vector, and the vector can re-allocate - so the pointers could change.
> >>
> >> Thus, I am storing an index.
> >
> > Makes sense, then please ignore my comment.
> >
> >>
> >>
> >> Should that be added in a comment or is it ok?
> >
> > If you can capture this in a few lines, why not.
> >
> > My question on the usage of .androidStream remains though
>
> I could store just the index in stream->priv as I think Laurent
> suggested (with casting) at this stage, and later add a struct, but I
> think I'll keep the struct for now.
>

If "later" is after this patch series, I tend to disagree, but I won't
bother too much

>
> >>>>  	}
> >>>>
> >>>>  	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 ?
> >>
> >> Because even the HAL only streams need to have the stream->max_buffers
> >> populated ? In the case of a hal-only stream, it gets populated with the
> >> max_buffers of the relevant /source/ stream which will feed that hal
> >> only stream:
> >>
> >>  Stream #1 YUV : MaxBuffers=4
> >>  Stream #2 MJPEG : MaxBuffers=maxBuffersOfStream(1);
> >>
> >> If we iterate over the camera configuration, we would not set the
> >> max_buffers for the hal-only streams, nor perform any other
> >> per-android-stream actions that may be required post-validation.
> >
> > Right, we need to iterate over all the android provided streams to
> > fill their max_buffers field.
> >
> > Now that I look at the code again, I see that in the first loop we store
> > an increasing streamIndex, which is exactly equal to i.
> >
> >
> > 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >                 ...
> >
> > 		config_->addConfiguration(streamConfiguration);
> > 		streams_[i].libcameraIndex = streamIndex++;
> > 	}
> >
> >         ...
> >
> > In the second loop we use that index, which is exactly equal to the
> > loop counter
> >
> > 	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->libcameraIndex);
> >
> > 		/* Use the bufferCount confirmed by the validation process. */
> > 		stream->max_buffers = cfg.bufferCount;
> > 	}
> >
> > Re-phrasing: why do you need that libcamerIdex at all now ?
> >
> > How do you plan to map HAL-only streams to the streamIndex ? There
> > will be one index in the stream_ vector for each of the android
> > stream, will some entries be repeated ? As HAL-only streams will
> > 'point' to the StreamConfiguration of the libcamera Stream that feeds
> > them ?
> >

Missed this part ?
Kieran Bingham July 3, 2020, 11:03 a.m. UTC | #9
Hi Jacopo,

On 03/07/2020 11:47, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Fri, Jul 03, 2020 at 11:36:56AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 03/07/2020 11:30, Jacopo Mondi wrote:
>>> Hi Kieran
>>>
>>> On Fri, Jul 03, 2020 at 10:55:22AM +0100, Kieran Bingham wrote:
>>>> Hi Jacopo,
>>>>
>>>> On 03/07/2020 10:35, Jacopo Mondi wrote:
>>>>> 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@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
>>>>
>>>> Drop the newline? Or something else?
>>>>
>>>
>>> yeah, new line
>>
>>
>> Can I disagree here? - I put that there intentionally to not 'hide' the
>> streamIndex variable against the for loop, and not associate the comment
>> added with it to the loop.
>>
>> The comment applies to the variable, not the loop.
> 
> Your code, your choice :D
> 
>>
>>
>>>
>>>>>
>>>>>>  	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 ?
>>
>>
>> Hrm, you might be right. Perhaps maintaining the correct indexing for
>> the streams_[] to camera3_stream_t, and (later) putting the pointer in
>> is enough.
>>
>> Wow - this struct is certainly getting small for now then, just storing
>> a single index ...
>>
>> And yet, I still want to make it a class... hrm ... maybe class can come
>> later.
> 
> Surely a class for single unsigned int would be 'slightly' and
> overkill :)

Of course, but this 'object' is the HAL Stream representation, and will
soon have an optional JPEG compressor... (or be subclassed to a JPEG
stream?, or .... we'll see).

And I don't know what will happen for RAW ... I hope Niklas has some
ideas forming...


>>>>>
>>>>>>  		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 ?
>>>>
>>>>
>>>> No, we can't store a pointer to the StreamConfiguration, because it's
>>>> not valid to do so.
>>>>
>>>>
>>>> At this point, we have 'added' the configuration to the config_, but it
>>>> makes a copy, so /this/ streamConfiguration is the wrong pointer to store.
>>>>
>>>
>>> Right, indeed stream configurations are copied into the camera.
>>> Didn't you have a patch to make CameraConfiguration::addConfiguration
>>> return a pointer to the stored StreamConfiguration ?
>>>
>>>
>>>> Further more, it could then be suggested that we just obtain the
>>>> 'correct' pointer - by using config_->at(n);.
>>>>
>>>> But we can't do that either, because we are in a loop, adding configs to
>>>> a vector, and the vector can re-allocate - so the pointers could change.
>>>>
>>>> Thus, I am storing an index.
>>>
>>> Makes sense, then please ignore my comment.
>>>
>>>>
>>>>
>>>> Should that be added in a comment or is it ok?
>>>
>>> If you can capture this in a few lines, why not.
>>>
>>> My question on the usage of .androidStream remains though
>>
>> I could store just the index in stream->priv as I think Laurent
>> suggested (with casting) at this stage, and later add a struct, but I
>> think I'll keep the struct for now.
>>
> 
> If "later" is after this patch series, I tend to disagree, but I won't
> bother too much
> 
>>
>>>>>>  	}
>>>>>>
>>>>>>  	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 ?
>>>>
>>>> Because even the HAL only streams need to have the stream->max_buffers
>>>> populated ? In the case of a hal-only stream, it gets populated with the
>>>> max_buffers of the relevant /source/ stream which will feed that hal
>>>> only stream:
>>>>
>>>>  Stream #1 YUV : MaxBuffers=4
>>>>  Stream #2 MJPEG : MaxBuffers=maxBuffersOfStream(1);
>>>>
>>>> If we iterate over the camera configuration, we would not set the
>>>> max_buffers for the hal-only streams, nor perform any other
>>>> per-android-stream actions that may be required post-validation.
>>>
>>> Right, we need to iterate over all the android provided streams to
>>> fill their max_buffers field.
>>>
>>> Now that I look at the code again, I see that in the first loop we store
>>> an increasing streamIndex, which is exactly equal to i.
>>>
>>>
>>> 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>>>                 ...
>>>
>>> 		config_->addConfiguration(streamConfiguration);
>>> 		streams_[i].libcameraIndex = streamIndex++;
>>> 	}
>>>
>>>         ...
>>>
>>> In the second loop we use that index, which is exactly equal to the
>>> loop counter
>>>
>>> 	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->libcameraIndex);
>>>
>>> 		/* Use the bufferCount confirmed by the validation process. */
>>> 		stream->max_buffers = cfg.bufferCount;
>>> 	}
>>>
>>> Re-phrasing: why do you need that libcamerIdex at all now ?
>>>
>>> How do you plan to map HAL-only streams to the streamIndex ? There
>>> will be one index in the stream_ vector for each of the android
>>> stream, will some entries be repeated ? As HAL-only streams will
>>> 'point' to the StreamConfiguration of the libcamera Stream that feeds
>>> them ?
>>>
> 
> Missed this part ?

Yes.

The {streamIndex,libcameraIndex,index} stored for each
{camera3_stream_t,CameraStream} points to the relevant index that can
correctly map to a libcamera StreamConfiguration and thus a Stream.

If only a JPEG stream is requested, then an NV12 stream will be added to
the libcamera config_, and the HAL stream #1 will use libcamera stream #1.

If an NV12 and MJPEG stream are requested (and they are the same size),
then MJPEG will use the NV12 stream index.

If the NV12 and MJPEG streams are not compatible, a new stream will be
added to libcamera:


JPEG only:
    HAL			libcamera
  Stream #1 JPEG -  Stream #1 NV12 (640x320)

JPEG+YUV(SameSize):
    HAL			libcamera
  Stream #1 NV12 -  Stream #1 NV12 (640x320)
  Stream #2 JPEG -  Stream #1 ^^^^ (640x320)

JPEG+YUV(DifferentSize, or incompatible format):
    HAL			libcamera
  Stream #1 NV12 -  Stream #1 NV12 (640x320)
  Stream #2 JPEG -  Stream #2 NV12 (1920x1080)

Patch

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;
+
 	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;
+
 		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++;
 	}
 
 	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;
 	}
 
 	/*
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_;