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

[Kieran Bingham]

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 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);
>>
>> 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 at 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_;
> 

-- 
Regards
--
Kieran