On Mon, 2013-08-12 at 13:18 +0200, David Henningsson wrote:
> On 08/12/2013 12:27 PM, Tanu Kaskinen wrote:
> > On Mon, 2013-08-12 at 09:20 +0200, David Henningsson wrote:
> >> On 08/09/2013 08:57 AM, Tanu Kaskinen wrote:
> >>> ---
> >>> src/pulsecore/sink.c | 4 ++--
> >>> src/pulsecore/source.c | 4 ++--
> >>> 2 files changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> >>> index a4ad21a..ae6ae8f 100644
> >>> --- a/src/pulsecore/sink.c
> >>> +++ b/src/pulsecore/sink.c
> >>> @@ -1412,8 +1412,8 @@ bool pa_sink_update_rate(pa_sink *s, uint32_t rate, bool passthrough) {
> >>> return false;
> >>>
> >>> if (!passthrough) {
> >>> - pa_assert(default_rate % 4000 || default_rate % 11025);
> >>> - pa_assert(alternate_rate % 4000 || alternate_rate % 11025);
> >>> + pa_assert((default_rate % 4000 == 0) || (default_rate % 11025 == 0));
> >>> + pa_assert((alternate_rate % 4000 == 0) || (alternate_rate % 11025 == 0));
> >>
> >> Looks correct, even though I don't understand why we need these
> >> constraints in the first place. (Why can't we just allow any sample rate
> >> as default or alternate?)
> >
> > I don't know. I would guess that this is a simple way to optimize
> > multiple common cases (anything that is divisible by 4000 or 11025),
> > while the general solution would be to optimize only two cases (the
> > exact default and alternate rates).
> >
>
> I remember a weird USB interface (in some launchpad bug) that only
> accepted a non-standard rate, I think it was around 46 kHz or something.
>
> In case there is only one rate, I'm not sure that the rate can ever
> update, but if it does (or in case a device would support two
> non-standard rates), you have effectively enabled an assertion crash
> that should perhaps not have been there in the first place. So I'm a bit
> worried about the above.
daemon.conf doesn't allow rates that aren't multiples of 4000 or 11025,
so there shouldn't be risk of crashing with the current code.
It should be possible to have the best of the both worlds, though: we
could support arbitrary default and alternate sample rate, and if those
happen to be multiples of 4000 and 11025, then we could enable the
additional optimizations. I don't know if it's my job to implement that,
though - this patch just fixes the assertions to match the (assumed)
original intent.
--
Tanu
