[pulseaudio-discuss] [PATCH v4 05/23] echo-cancel: Relax restrictions on webrtc AEC stream config

[Tanu Kaskinen]

On Mon, 2016-02-22 at 11:53 +0530, Arun Raghavan wrote:
> On Sat, 2016-02-20 at 15:35 +0200, Tanu Kaskinen wrote:
> > On Wed, 2016-02-17 at 19:46 +0530, arun at accosted.net wrote:
> > > From: Arun Raghavan <git at arunraghavan.net>
> > > 
> > > This exposes the range of configurations the library actually supports
> > > (8, 16, 32 and 48 kHz).
> > 
> > That doesn't seem to explain all changes in the code. I'll analyze all
> > the variables:
> > 
> > play_ss.format is handled as before, fixed to s16le.
> > 
> > play_ss.rate was earlier set to out_ss.rate, now it's set to one of the
> > four possible rates based on the initial rec_ss.rate value.
> > 
> > play_ss.channels and play_map were earlier set to out_map, now they are
> > unchanged.
> > 
> > rec_ss.format is handled as before, fixed to s16le.
> > 
> > rec_ss.rate was earlier set to out_ss.rate, now it's set to one of the
> > four possible rates based on the initial rec_ss.rate value.
> > 
> > rec_ss.channels and rec_map were earlier set to out_map, now they are
> > unchanged.
> > 
> > out_ss.format is handled as before, fixed to s16le.
> > 
> > out_ss.rate was earlier kept unchanged, now it's set to one of the four
> > possible rates based on the initial rec_ss.rate value.
> > 
> > out_ss.channels and out_map were earlier kept unchanged, now they are
> > set to the original value of rec_map.
> > 
> > In summary: no changes in sample format configuration, rate is based on
> > the original rec_ss instead of the original out_ss and limited to the
> > four possible rates (earlier there was no such limitation, so is the
> > commit message wrong saying "relax restrictions"?), channels are based
> > on rec_map instead of out_map.
> 
> Functionally, there is no difference. out_ss/map and rec_ss/map
> essentially had the same value, and I just used them interchangeably
> (which now IMO is confusing).
> 
> In the refactoring, I'm expressing the constraints in what I see to be
> a more natural way -- rec_ss expresses what we're feeding the
> canceller, so it makes sense to apply the constraints on what the
> canceller accepts there. This then propagates to the output spec.
> 
> This all seems fairly minor to me, but I can add it to the commit
> message as well if you want.

Thanks, I think that would be good.

Do you have comments about the question of whether it's correct to say
"relax restrictions" in the commit message, when it looks like you're
rather adding restrictions?

> > There is and there was no limitation on the channel count - but should
> > there be? I don't think the processing can handle very many channels,
> > at least before switching to deinterleaved processing.
> 
> The AudioProcessing API doesn't document constraints on the channel
> count (I suppose there are constraints internally, though, given what
> you'd posted earlier w.r.t. the AudioFrame buffer size). I think fixing
> that in this commit would be wasted effort since, as you point out, the
> deinterleaved processing does make that irrelevant.

I agree, no need to fix it in this patch, since the issue is not new,
and it will be anyway fixed later.

-- 
Tanu