* [PATCH - Intervals 1/1] interval: Interpret (x x+1] correctly and return x+1
@ 2018-10-18 10:50 twischer
2018-10-18 10:57 ` Takashi Iwai
0 siblings, 1 reply; 2+ messages in thread
From: twischer @ 2018-10-18 10:50 UTC (permalink / raw)
To: patch; +Cc: Timo Wischer, alsa-devel
From: Timo Wischer <twischer@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) similar 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@de.adit-jv.com>
diff --git a/src/pcm/interval_inline.h b/src/pcm/interval_inline.h
index a68e292..5e59d72 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->openmax)
+ return i->max;
return i->min;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH - Intervals 1/1] interval: Interpret (x x+1] correctly and return x+1
2018-10-18 10:50 [PATCH - Intervals 1/1] interval: Interpret (x x+1] correctly and return x+1 twischer
@ 2018-10-18 10:57 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2018-10-18 10:57 UTC (permalink / raw)
To: twischer; +Cc: alsa-devel
On Thu, 18 Oct 2018 12:50:26 +0200,
<twischer@de.adit-jv.com> wrote:
>
> From: Timo Wischer <twischer@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) similar 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@de.adit-jv.com>
>
> diff --git a/src/pcm/interval_inline.h b/src/pcm/interval_inline.h
> index a68e292..5e59d72 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)));
> }
This change looks reasonable, but....
> INTERVAL_INLINE int snd_interval_value(const snd_interval_t *i)
> {
> assert(snd_interval_single(i));
> + if (!i->openmax)
> + return i->max;
> return i->min;
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.
thanks,
Takashi
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-10-18 10:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-18 10:50 [PATCH - Intervals 1/1] interval: Interpret (x x+1] correctly and return x+1 twischer
2018-10-18 10:57 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.