[libcamera-devel,v1] gstreamer: Simplify camera config generation call
diff mbox series

Message ID 20230502175842.70629-1-pobrn@protonmail.com
State Rejected
Headers show
Series
  • [libcamera-devel,v1] gstreamer: Simplify camera config generation call
Related show

Commit Message

Barnabás Pőcze May 2, 2023, 5:58 p.m. UTC 
Instead of explicitly creating a vector of StreamRole,
simply use an initializer list as the argument of
`Camera::generateConfiguration()`.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/gstreamer/gstlibcameraprovider.cpp | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--
2.40.1

Comments

Umang Jain May 2, 2023, 9:13 p.m. UTC | #1 
Hi Barnabas,

Thank you for the patch

On 5/2/23 11:28 PM, Barnabás Pőcze via libcamera-devel wrote:
> Instead of explicitly creating a vector of StreamRole,
> simply use an initializer list as the argument of
> `Camera::generateConfiguration()`.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>   src/gstreamer/gstlibcameraprovider.cpp | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> 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..
> +	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;
>   	}
> --
> 2.40.1
>
>
Barnabás Pőcze May 3, 2023, 12:26 a.m. UTC | #2 
Hi


2023. május 2., kedd 23:13 keltezéssel, Umang Jain <umang.jain@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 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?


> > +	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
Umang Jain May 3, 2023, 4:31 a.m. UTC | #3 
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@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
Barnabás Pőcze May 3, 2023, 12:31 p.m. UTC | #4 
Hi


2023. május 3., szerda 6:31 keltezéssel, Umang Jain <umang.jain@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.

Good point. Please disregard this patch.


> [...]


Regards,
Barnabás Pőcze

Patch
diff mbox series 

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()) {
+	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;
 	}