| Message ID | 20190310175109.13444-1-ps.report@gmx.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,v1] package/gst1-plugins-bad: add webrtcbin option | expand |
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
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
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 >
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 >> >
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
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
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
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(+)