[RFC,v1] package/gst1-plugins-bad: add webrtcbin option

Message ID 20190310175109.13444-1-ps.report@gmx.net
State Changes Requested
Headers show
Series
  • [RFC,v1] package/gst1-plugins-bad: add webrtcbin option
Related show

Commit Message

Peter Seiderer March 10, 2019, 5:51 p.m.
Signed-off-by: Peter Seiderer <ps.report@gmx.net>
---
Notes:
  - just compile tested yet
  - named webrtcbin option (instead of webrtc), because of the
    BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC entry in Config.in.legacy
    (see [1] for history)

[1] https://git.buildroot.net/buildroot/commit/?id=4c06d2490a07f0b88f42c56c7409899fd2f5608a
---
 package/gstreamer1/gst1-plugins-bad/Config.in         | 11 +++++++++++
 .../gstreamer1/gst1-plugins-bad/gst1-plugins-bad.mk   |  7 +++++++
 2 files changed, 18 insertions(+)

Comments

Thomas Petazzoni March 12, 2019, 9:27 p.m. | #1
Hello,

+Arnout in Cc, our Config.in.legacy/backward-compatibility guru.

On Sun, 10 Mar 2019 18:51:09 +0100
Peter Seiderer <ps.report@gmx.net> wrote:

> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> ---
> Notes:
>   - just compile tested yet
>   - named webrtcbin option (instead of webrtc), because of the
>     BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC entry in Config.in.legacy
>     (see [1] for history)

Would it be a big problem to reuse the
BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC option ?

Best regards,

Thomas
Arnout Vandecappelle March 13, 2019, 9:05 a.m. | #2
On 12/03/2019 22:27, Thomas Petazzoni wrote:
> Hello,
> 
> +Arnout in Cc, our Config.in.legacy/backward-compatibility guru.
> 
> On Sun, 10 Mar 2019 18:51:09 +0100
> Peter Seiderer <ps.report@gmx.net> wrote:
> 
>> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
>> ---
>> Notes:
>>   - just compile tested yet
>>   - named webrtcbin option (instead of webrtc), because of the
>>     BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC entry in Config.in.legacy
>>     (see [1] for history)
> 
> Would it be a big problem to reuse the
> BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC option ?

 I believe the plugin got a different name, and is used in a different way. So
reusing the same option could lead to things that stop working - if it's a
different name, at least there is still the legacy warning.

 That said, in -bad API changes sometimes happen anyway, so maybe we shouldn't
worry too much about that.

 Regards,
 Arnout
Peter Seiderer March 13, 2019, 7:17 p.m. | #3
Hello Arnout,

On Wed, 13 Mar 2019 10:05:19 +0100, Arnout Vandecappelle <arnout@mind.be> wrote:

> On 12/03/2019 22:27, Thomas Petazzoni wrote:
> > Hello,
> > 
> > +Arnout in Cc, our Config.in.legacy/backward-compatibility guru.
> > 
> > On Sun, 10 Mar 2019 18:51:09 +0100
> > Peter Seiderer <ps.report@gmx.net> wrote:
> >   
> >> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> >> ---
> >> Notes:
> >>   - just compile tested yet
> >>   - named webrtcbin option (instead of webrtc), because of the
> >>     BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC entry in Config.in.legacy
> >>     (see [1] for history)  
> > 
> > Would it be a big problem to reuse the
> > BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC option ?  
> 
>  I believe the plugin got a different name, and is used in a different way. So
> reusing the same option could lead to things that stop working - if it's a
> different name, at least there is still the legacy warning.
> 
>  That said, in -bad API changes sometimes happen anyway, so maybe we shouldn't
> worry too much about that.

The history is there is a webrtcdsp plugin in gst1-plugins-bad which
was (false) named simple webrtc in buildroot and was renamed (with a legacy
entry) to webrtcdsp. Since gstreamer-1.12.x there is an 'real' additional/new webrtc
plugin in gst1-plugins-bad (nothing to do with -bad API changes, just a
buildroot history problem)...

Two possibilities:

A) add a new buildroot option with the right name (and delete the legacy entry?)

B) add a new buildroot option with the wrong name webrtcbin instead of webrtc,
   but avoid the legacy entry problem (as the suggested RFC patch does)


Regards,
Peter

> 
>  Regards,
>  Arnout
>
Arnout Vandecappelle March 14, 2019, 7:27 a.m. | #4
On 13/03/2019 20:17, Peter Seiderer wrote:
> Hello Arnout,
> 
> On Wed, 13 Mar 2019 10:05:19 +0100, Arnout Vandecappelle <arnout@mind.be> wrote:
> 
>> On 12/03/2019 22:27, Thomas Petazzoni wrote:
>>> Hello,
>>>
>>> +Arnout in Cc, our Config.in.legacy/backward-compatibility guru.
>>>
>>> On Sun, 10 Mar 2019 18:51:09 +0100
>>> Peter Seiderer <ps.report@gmx.net> wrote:
>>>   
>>>> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
>>>> ---
>>>> Notes:
>>>>   - just compile tested yet
>>>>   - named webrtcbin option (instead of webrtc), because of the
>>>>     BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC entry in Config.in.legacy
>>>>     (see [1] for history)  
>>>
>>> Would it be a big problem to reuse the
>>> BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTC option ?  
>>
>>  I believe the plugin got a different name, and is used in a different way. So
>> reusing the same option could lead to things that stop working - if it's a
>> different name, at least there is still the legacy warning.
>>
>>  That said, in -bad API changes sometimes happen anyway, so maybe we shouldn't
>> worry too much about that.
> 
> The history is there is a webrtcdsp plugin in gst1-plugins-bad which
> was (false) named simple webrtc in buildroot and was renamed (with a legacy
> entry) to webrtcdsp. Since gstreamer-1.12.x there is an 'real' additional/new webrtc
> plugin in gst1-plugins-bad (nothing to do with -bad API changes, just a
> buildroot history problem)...

 OK, I minsunderstood...

 The Config.in option was introduced in 2017.02, and renamed for 2017.08.

 However, the rename was also backported to 2017.02.4.

 In other words, the only people who could be hit by this issue are people that
used 2017.02 and never upgraded even to 2017.02.4.

 I think it's OK if we risk breaking something for those people. Especially
because the problem is very obvious and very easy to fix.


> 
> Two possibilities:
> 
> A) add a new buildroot option with the right name (and delete the legacy entry?)

 In other words, go for this option (and indeed delete the legacy entry).

 Regards,
 Arnout

> 
> B) add a new buildroot option with the wrong name webrtcbin instead of webrtc,
>    but avoid the legacy entry problem (as the suggested RFC patch does)
> 
> 
> Regards,
> Peter
> 
>>
>>  Regards,
>>  Arnout
>>
>
Thomas Petazzoni March 14, 2019, 7:54 a.m. | #5
On Thu, 14 Mar 2019 08:27:56 +0100
Arnout Vandecappelle <arnout@mind.be> wrote:

>  The Config.in option was introduced in 2017.02, and renamed for 2017.08.
> 
>  However, the rename was also backported to 2017.02.4.
> 
>  In other words, the only people who could be hit by this issue are people that
> used 2017.02 and never upgraded even to 2017.02.4.
> 
>  I think it's OK if we risk breaking something for those people. Especially
> because the problem is very obvious and very easy to fix.

Yes, I agree: I believe it's better to have the right name for the
option moving forward, and the problem with re-using the name of the
previous option is not too bad.

So, I'm happy to see that both of us agree on the direction to take
with this.

Thanks

Thomas
Peter Seiderer March 16, 2019, 9:55 p.m. | #6
Hello Arnout, Thomas,

On Thu, 14 Mar 2019 08:54:45 +0100, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Thu, 14 Mar 2019 08:27:56 +0100
> Arnout Vandecappelle <arnout@mind.be> wrote:
> 
> >  The Config.in option was introduced in 2017.02, and renamed for 2017.08.
> > 
> >  However, the rename was also backported to 2017.02.4.
> > 
> >  In other words, the only people who could be hit by this issue are people that
> > used 2017.02 and never upgraded even to 2017.02.4.
> > 
> >  I think it's OK if we risk breaking something for those people. Especially
> > because the problem is very obvious and very easy to fix.  
> 
> Yes, I agree: I believe it's better to have the right name for the
> option moving forward, and the problem with re-using the name of the
> previous option is not too bad.
> 
> So, I'm happy to see that both of us agree on the direction to take
> with this.

To both: thanks for review and advice, updated patch follows...

Regards,
Peter

> 
> Thanks
> 
> Thomas

Patch

diff --git a/package/gstreamer1/gst1-plugins-bad/Config.in b/package/gstreamer1/gst1-plugins-bad/Config.in
index 869f0a9d45..5d0480cfb0 100644
--- a/package/gstreamer1/gst1-plugins-bad/Config.in
+++ b/package/gstreamer1/gst1-plugins-bad/Config.in
@@ -558,6 +558,17 @@  config BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBP
 	help
 	  Webp image format plugin
 
+config BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTCBIN
+	bool "webrtcbin"
+	depends on !BR2_STATIC_LIBS # libnice -> gnutls
+	select BR2_PACKAGE_GST1_PLUGINS_BASE # libgstsdp
+	select BR2_PACKAGE_LIBNICE
+	help
+	  WebRTC plugins (webrtcbin - a bin for webrtc connections)
+
+comment "webrtcbin needs a toolchain w/ dynamic library"
+	depends on BR2_STATIC_LIBS
+
 config BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTCDSP
 	bool "webrtcdsp"
 	# All depends from webrtc-audio-processing
diff --git a/package/gstreamer1/gst1-plugins-bad/gst1-plugins-bad.mk b/package/gstreamer1/gst1-plugins-bad/gst1-plugins-bad.mk
index f5b081f972..2220554a2d 100644
--- a/package/gstreamer1/gst1-plugins-bad/gst1-plugins-bad.mk
+++ b/package/gstreamer1/gst1-plugins-bad/gst1-plugins-bad.mk
@@ -703,6 +703,13 @@  else
 GST1_PLUGINS_BAD_CONF_OPTS += --disable-webp
 endif
 
+ifeq ($(BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTCBIN),y)
+GST1_PLUGINS_BAD_CONF_OPTS += --enable-webrtc
+GST1_PLUGINS_BAD_DEPENDENCIES += gst1-plugins-base libnice
+else
+GST1_PLUGINS_BAD_CONF_OPTS += --disable-webrtc
+endif
+
 ifeq ($(BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_WEBRTCDSP),y)
 GST1_PLUGINS_BAD_CONF_OPTS += --enable-webrtcdsp
 GST1_PLUGINS_BAD_DEPENDENCIES += webrtc-audio-processing