On 2 February 2016 at 18:12, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Tue, 2016-02-02 at 09:59 +0530, Arun Raghavan wrote:
>> On Mon, 2016-02-01 at 13:18 +0200, Tanu Kaskinen wrote:
>> > On Mon, 2016-01-18 at 13:06 +0530, arun at accosted.net wrote:
>> > > From: Arun Raghavan <git at arunraghavan.net>
>> > >
>> > > This is required to make sure the capture output has sufficient
>> > > energy
>> > > for the AGC to do its job.
>> >
>> > I think the word "value" in the first line of the commit message
>> > should
>> > be replaced with "volume". After reading just the commit message, I
>> > didn't know what the patch was about.
>> >
>> > > ---
>> > > src/modules/echo-cancel/echo-cancel.h | 1 +
>> > > src/modules/echo-cancel/webrtc.cc | 14 +++++++++++++-
>> > > 2 files changed, 14 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/src/modules/echo-cancel/echo-cancel.h
>> > > b/src/modules/echo-cancel/echo-cancel.h
>> > > index 142f8ac..b570095 100644
>> > > --- a/src/modules/echo-cancel/echo-cancel.h
>> > > +++ b/src/modules/echo-cancel/echo-cancel.h
>> > > @@ -68,6 +68,7 @@ struct pa_echo_canceller_params {
>> > > pa_sample_spec sample_spec;
>> > > bool agc;
>> > > bool trace;
>> > > + bool first;
>> > > } webrtc;
>> > > #endif
>> > > /* each canceller-specific structure goes here */
>> > > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
>> > > cancel/webrtc.cc
>> > > index c2585a8..6431651 100644
>> > > --- a/src/modules/echo-cancel/webrtc.cc
>> > > +++ b/src/modules/echo-cancel/webrtc.cc
>> > > @@ -52,6 +52,7 @@ PA_C_DECL_END
>> > > #define DEFAULT_TRACE false
>> > >
>> > > #define WEBRTC_AGC_MAX_VOLUME 255
>> > > +#define WEBRTC_AGC_START_VOLUME 85
>> > >
>> > > static const char* const valid_modargs[] = {
>> > > "high_pass_filter",
>> > > @@ -297,6 +298,7 @@ bool pa_webrtc_ec_init(pa_core *c,
>> > > pa_echo_canceller *ec,
>> > > ec->params.priv.webrtc.sample_spec = *out_ss;
>> > > ec->params.priv.webrtc.blocksize =
>> > > (uint64_t)pa_bytes_per_second(out_ss) * BLOCK_SIZE_US /
>> > > PA_USEC_PER_SEC;
>> > > *nframes = ec->params.priv.webrtc.blocksize /
>> > > pa_frame_size(out_ss);
>> > > + ec->params.priv.webrtc.first = true;
>> > >
>> > > pa_modargs_free(ma);
>> > > return true;
>> > > @@ -354,7 +356,17 @@ void pa_webrtc_ec_record(pa_echo_canceller
>> > > *ec, const uint8_t *rec, uint8_t *out
>> > > apm->ProcessStream(&out_frame);
>> > >
>> > > if (ec->params.priv.webrtc.agc) {
>> > > - new_volume = apm->gain_control()->stream_analog_level();
>> > > + if (PA_UNLIKELY(ec->params.priv.webrtc.first)) {
>> > > + /* We start at a sane default volume (taken from the
>> > > Chromium
>> > > + * condition on the experimental AGC in
>> > > audio_processing.h). This is
>> > > + * needed to make sure that there's enough energy in
>> > > the capture
>> > > + * signal for the AGC to work */
>> > > + ec->params.priv.webrtc.first = false;
>> > > + new_volume = WEBRTC_AGC_START_VOLUME;
>> >
>> > This is done only on initial loading of the module. Does AGC work ok
>> > after resuming a suspended source?
>>
>> After suspend/resume is not a problem since the source volume won't get
>> reset from whatever it was set to before. This is mostly to make sure
>> that when the AGC first starts, it is able to actually get going.
>
> Ok. Is the problem that if the initial volume is too low, AGC won't
> work, but too high initial volume is fine? What if the user sets the
Yes (but too high does mean the far end gets a burst of loud audio to
start with).
> source volume manually? Does AGC stop working? If not, why is that
> different from the initialization phase? (I'm just trying to understand
> the problem. If manual volume changes mess up AGC, I don't think that's
> something that this patch needs to care about.)
If the user modifies the volume, it does indeed stop working.
This actually is a bit of a regression in the library. I'm hoping to
do an update again in the coming month so will test again to see if
that fixes things.
-- Arun
