[libcamera-devel] [PATCH v5 1/3] android: camera_device: Introduce Camera3StreamConfig

[Umang Jain]

Hi Kieran

On 12/9/20 4:01 PM, Kieran Bingham wrote:
> Hi Hiro,
>
> On 09/12/2020 05:54, Hirokazu Honda wrote:
>> Camera3StreamConfig is a new class to store camera3_stream and
>> types with associated StreamConfiguration.
>>
>> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Perhaps this could be squashed with 2/3 which uses it - but w're at v5
> here, so I don't think it's worth it now.
>
> It looks like this series is likely ready for merging, so no need to
> repost just to squash.
>
>> ---
>>   src/android/camera_device.cpp | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 675af570..09269d95 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
>>   	},
>>   };
>>
>> +/*
>> + * \struct Camera3StreamConfig
>> + * \brief Data to store StreamConfiguration associated with camera3_stream(s).
>> + * \var streams List of streams requested by Android HAL client.
>> + * \var types List of CameraStream::Type associated with streams.
>> + * \var config StreamConfiguration for streams.
>> + */
>> +struct Camera3StreamConfig {
>> +	std::vector<camera3_stream_t*> streams;
>> +	std::vector<CameraStream::Type> types;
> As far as i can tell, in 2/3 both streams and types are always used at
> the same index?
>
> If that's the case, I would have preferred to see them grouped directly
> so that it is enforced.
>
> That could either be by putting a std::pair in the vector (though I'm
> not sure if that gets a bit ugly) or grouping them together in a struct
> ... but then we have yet another type...
>
> So - I'm not going to say that's needed right now - but I'm just
> cautious of adding lots of multiple data types that require the same
> indexing, rather than putting the grouped data together, and stored
> inside a single data structure...
This is my concern constantly while reviewing the series, but I have 
failed to come-up with a solution so I should not complain. :) I got the 
idea and it seems enough people have eyes on this, so probably this can 
go ahead.
> In this case, I don't think it matters as it's just semantics for
> accessing the data.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>> +	StreamConfiguration config;
>> +};
>>   } /* namespace */
>>
>>   LOG_DECLARE_CATEGORY(HAL)
>> --
>> 2.29.2.576.ga3fc446d84-goog
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>