[libcamera-devel] [PATCH v1] gstreamer: Simplify camera config generation call

[Umang Jain]

Hi

On 5/3/23 5:56 AM, Barnabás Pőcze wrote:
> Hi
>
>
> 2023. május 2., kedd 23:13 keltezéssel, Umang Jain <umang.jain at ideasonboard.com> írta:
>
>> [...]
>>> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
>>> index 6eb0a0eb..4f325ad4 100644
>>> --- a/src/gstreamer/gstlibcameraprovider.cpp
>>> +++ b/src/gstreamer/gstlibcameraprovider.cpp
>>> @@ -128,11 +128,9 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
>>>    {
>>>    	g_autoptr(GstCaps) caps = gst_caps_new_empty();
>>>    	const gchar *name = camera->id().c_str();
>>> -	StreamRoles roles;
>>>
>>> -	roles.push_back(StreamRole::VideoRecording);
>>> -	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
>>> -	if (!config || config->size() != roles.size()) {
>> I would leave it as it is, as it's easier to add new roles (just a
>> .push_back() call) and the adjoining pieces automatically makes it fall
>> into place..
> That is a fair point, I did not really explain the motivation. I would like to
> post a second version of https://patchwork.libcamera.org/patch/18285/ that would
> also remove the `StreamRoles` alias altogether.

If there's are adjoining changes, I suggest clubbing all that in a 
single series and posting.

For e.g. I would remove `StreamRoles` from every place in the codebase 
and then remove the `StreamRoles` definition / implementation from the 
concerned class. This provides a more holisitic view.
>
> If this current version is undesirable, then I can incorporate this change into
> the second version of the aforementioned patch series as follows:
>
> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> index 6eb0a0eb..332837e4 100644
> --- a/src/gstreamer/gstlibcameraprovider.cpp
> +++ b/src/gstreamer/gstlibcameraprovider.cpp
> @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)
>   static GstDevice *
>   gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
>   {
> +	static const StreamRole roles[] = { StreamRole::VideoRecording };
>   	g_autoptr(GstCaps) caps = gst_caps_new_empty();
>   	const gchar *name = camera->id().c_str();
> -	StreamRoles roles;
>   
> -	roles.push_back(StreamRole::VideoRecording);
>   	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> -	if (!config || config->size() != roles.size()) {
> +	if (!config || config->size() != std::size(roles)) {
>   		GST_ERROR("Failed to generate a default configuration for %s", name);
>   		return nullptr;
>   	}
>
> would this be then acceptable?

This looks okay to me. But might be connected to the wider StreamRoles 
discussion?
>
>
>>> +	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration({ StreamRole::VideoRecording });
>>> +	if (!config || config->size() != 1) {
>>>    		GST_ERROR("Failed to generate a default configuration for %s", name);
>>>    		return nullptr;
>>>    	}
>> [...]
>
> Regards,
> Barnabás Pőcze