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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran