[alsa-devel] [PATCH - Intervals v2 1/1] interval: Interpret (x x+1] correctly and return x+1

[Takashi Iwai]

On Thu, 18 Oct 2018 14:28:47 +0200,
Timo Wischer wrote:
> 
> On 10/18/18 14:02, Takashi Iwai wrote:
> > On Thu, 18 Oct 2018 13:33:24 +0200,
> > <twischer at de.adit-jv.com> wrote:
> >> From: Timo Wischer <twischer at de.adit-jv.com>
> >>
> >> Without this change an interval of (x x+1] will be interpreted as an
> >> empty interval but the right value would be x+1.
> >> This leads to a failing snd_pcm_hw_params() call which returns -EINVAL.
> >>
> >> An example issue log is given in the following:
> >> snd_pcm_hw_params failed with err -22 (Invalid argument)
> >> ACCESS: MMAP_NONINTERLEAVED
> >> FORMAT: S16_LE
> >> SUBFORMAT: STD
> >> SAMPLE_BITS: 16
> >> FRAME_BITS: 16
> >> CHANNELS: 1
> >> RATE: 16000
> >> PERIOD_TIME: (15999 16000]
> >> PERIOD_SIZE: (255 256]
> >> PERIOD_BYTES: (510 512]
> >> PERIODS: [2 3)
> >> BUFFER_TIME: 32000
> >> BUFFER_SIZE: 512
> >> BUFFER_BYTES: 1024
> >>
> >> In case of (x x+1) we have to interpret it anyway as a single value of x to
> >> compensate rounding issues.
> >> For example the period size will result in an interval of (352 353) when
> >> the period time is 16ms and the sample rate 22050 Hz
> >> (16ms * 22,05 kHz = 352,8 frames). But 352 has to be chosen to allow a
> >> buffer size of 705 (32ms * 22,05 kHz = 705,6 frames) which has to be >= 2x
> >> period size to avoid Xruns. The buffer size will not end up with an
> >> interval of (705 706) simular to the period size because
> >> snd_pcm_rate_hw_refine_cchange() calls snd_interval_floor() for the buffer
> >> size. Therefore this value will be interpreted as an integer interval
> >> instead of a real interval further on.
> >>
> >> This issue seems to exist since the change of 9bb985c38 ("pcm:
> >> snd_interval_refine_first/last: exclude value only if also excluded
> >> before")
> >>
> >> Signed-off-by: Timo Wischer <twischer at de.adit-jv.com>
> >> ---
> >>> On 10/18/18 12:57, Takashi Iwai wrote:
> >>> This change looks risky.  The snd_interval_value() might be called
> >>> even if the interval isn't reduced to a single value.  Rather check
> >>> openmin instead.
> >> If I would do
> >> "if (i->openmin)"
> >> on an interval of (x x+1) x+1 would be returned. But this would result in
> >> the second issue which I have tried to describe in the commit message:
> >> x has to be returned otherwise it is not guaranteed that
> >> buffer_size >= 2x period_size. And this would result in continuously Xruns.
> >> Therefore I would like to prefer
> >> "if (i->openmin && !i->openmax)"
> > Hm, I overlooked that there is an assert() before that.  So a
> > single-value interval is a must at this execution, so it doesn't
> > matter which one to take.
> >
> > Didn't the assert() hit in your case with x+1, then?
> No. For the implementation of
> 
> snd_interval_value()
> (x x+1) is also a valid interval which has only a single value.
> This is also required because the ALSA neogation rules often end up with intervals like (x x+1)
> e.g. in case of rounding issues  32ms * 22,05 kHz = 705,6 frames -> (705 706)
> Therefore assert(snd_interval_single(i)) has to not fail in case of (705 706).

And this is a problematic case no matter which value to take.
Your change might "fix" your specific case, but it might break
others.  No matter which value (min or max) to take, it's basically
invalid.

If your fix is logically correct, it's fine.  But, judging from the
description, it looks like nothing but heuristic based on your
specific use cases.

IOW, if we have a good set of unit tests to cover most of possible use
cases and this is proven to improve all these, I'll happily take it.
But unfortunately there is no good test coverage, so far.


thanks,

Takashi

> 
> Best regards
> Timo
> 
> >
> >
> > Takashi
> >
> >> diff --git a/src/pcm/interval_inline.h b/src/pcm/interval_inline.h
> >> index a68e292..d9a30b2 100644
> >> --- a/src/pcm/interval_inline.h
> >> +++ b/src/pcm/interval_inline.h
> >> @@ -51,12 +51,14 @@ INTERVAL_INLINE int snd_interval_single(const snd_interval_t *i)
> >>   {
> >>   	assert(!snd_interval_empty(i));
> >>   	return (i->min == i->max ||
> >> -		(i->min + 1 == i->max && i->openmax));
> >> +		(i->min + 1 == i->max && (i->openmin || i->openmax)));
> >>   }
> >>     INTERVAL_INLINE int snd_interval_value(const snd_interval_t *i)
> >>   {
> >>   	assert(snd_interval_single(i));
> >> +	if (i->openmin && !i->openmax)
> >> +		return i->max;
> >>   	return i->min;
> >>   }
> >>   -- 
> >> 2.7.4
> >>
>