GNOME Bugzilla – Bug 780976
webrtcdsp: do not fail to start if webrtcechoprobe is not found
Last modified: 2017-05-02 13:00:58 UTC
Created attachment 349387 [details] [review] webrtcdsp: do not fail to start if webrtcechoprobe is not found echo-cancel : Enable or disable echo canceller, note that it will be disabled if no webrtcechoprobe has been found This is what the documentation of the property says. This patch implements this behavior.
Would it be safer for it to not fail only if the property if FALSE, that way, people won't have unexpected semi-silent failure?
As I mention to George today, clearly the doc and implementation has diverged. Implementing the doc is indeed error prone, since if it didn't find a probe, it will never find it later, hence echo-cancellation will never work. To me, the obvious approach is to make this property dynamic, so you could enable it at run-time. In that case, enabling while there is no probe should raise an error. Any other solution, fixing the doc, or this, is of course acceptable, but seems like the poor way. In any case, it's pretty wrong to rely on this in your application.
Review of attachment 349387 [details] [review]: ::: ext/webrtcdsp/gstwebrtcdsp.cpp @@ +521,3 @@ + GST_WARNING_OBJECT (self, "No echo probe with name %s found, " + "disabling echo cancellation", self->probe_name); + self->echo_cancel = FALSE; My concern here is that you are changing a value set by the user. Worst to that, you are not even notifying the property change.
Created attachment 349485 [details] [review] webrtcdsp: fix doc string of echo-cancel property Fair enough, let's just fix the doc string then. I'm not a fan of implementing the dynamic support myself yet.
Review of attachment 349485 [details] [review]: Thanks.
Sorry, I had forgotten about this one. commit cdfa0897e52696155237e56eda91485c1a4eb432 Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Fri Apr 7 17:13:52 2017 +0300 webrtcdsp: fix doc string of echo-cancel property If the echo probe element is not found, initialization actually fails instead of silently working with echo-cancel disabled. https://bugzilla.gnome.org/show_bug.cgi?id=780976