| Message ID | 1402569264-2789-3-git-send-email-patrakov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Date 12.6.2014 12:34, Alexander E. Patrakov wrote: > This commit does not fix nonsense values returned by the rewind and > forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, > an attempt to rewind 1024 samples from the nearly-full buffer returns > 4090. > > Due to these nonsense values, the current rate plugin should be treated > as non-rewindable. That's why the new callbacks return 0. > > Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> I don't agree. The snd_pcm_rate_move_applptr() should be fixed instead this blocking attempt. Do you have any simple use case? Jaroslav > --- > src/pcm/pcm_rate.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c > index 2eb4b1b..58ed842 100644 > --- a/src/pcm/pcm_rate.c > +++ b/src/pcm/pcm_rate.c > @@ -689,6 +689,16 @@ static int snd_pcm_rate_reset(snd_pcm_t *pcm) > return 0; > } > > +static snd_pcm_sframes_t snd_pcm_rate_rewindable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) > +{ > + return 0; > +} > + > +static snd_pcm_sframes_t snd_pcm_rate_forwardable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) > +{ > + return 0; > +} > + > static snd_pcm_sframes_t snd_pcm_rate_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > { > snd_pcm_rate_t *rate = pcm->private_data; > @@ -1221,7 +1231,9 @@ static const snd_pcm_fast_ops_t snd_pcm_rate_fast_ops = { > .drop = snd_pcm_generic_drop, > .drain = snd_pcm_rate_drain, > .pause = snd_pcm_generic_pause, > + .rewindable = snd_pcm_rate_rewindable, > .rewind = snd_pcm_rate_rewind, > + .forwardable = snd_pcm_rate_forwardable, > .forward = snd_pcm_rate_forward, > .resume = snd_pcm_generic_resume, > .writei = snd_pcm_mmap_writei, >
13.06.2014 13:13, Jaroslav Kysela ?????: > Date 12.6.2014 12:34, Alexander E. Patrakov wrote: >> This commit does not fix nonsense values returned by the rewind and >> forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, >> an attempt to rewind 1024 samples from the nearly-full buffer returns >> 4090. >> >> Due to these nonsense values, the current rate plugin should be treated >> as non-rewindable. That's why the new callbacks return 0. >> >> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> > I don't agree. The snd_pcm_rate_move_applptr() should be fixed instead > this blocking attempt. Do you have any simple use case? I don't understand what you mean by a "simple use case". And anyway, due to reasons expressed in http://permalink.gmane.org/gmane.linux.alsa.devel/122179 , I insist that 0 is the only correct return value even if someone fixes snd_pcm_rate_move_applptr(), which is not trivial. Also, 0 is not a regression - it was "crash" previously, and I have not changed the currently-broken snd_pcm_rate_move_applptr logic (just for the impossible case if someone does rely on it). The full fix, after which I do agree to change 0 to something else, would involve: 1. Writing a rewindable resampler library, or a resampler library that offers a public API to save its state. 2. Dropping all other resampler implementations. 3. Extending the snd_pcm_rate_ops_t structure in order to forward either rewinds or save-requests to the resampler. 4. Using these new callbacks. This translates to several months, or maybe a year of work at the current rate. It has to be done, and I do plan to do this. But it should not block the crash-fix: a suboptimal but valid return value of snd_pcm_rewindable() is better than a crash inside this function.
Date 13.6.2014 10:44, Alexander E. Patrakov wrote: > 13.06.2014 13:13, Jaroslav Kysela ?????: >> Date 12.6.2014 12:34, Alexander E. Patrakov wrote: >>> This commit does not fix nonsense values returned by the rewind and >>> forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, >>> an attempt to rewind 1024 samples from the nearly-full buffer returns >>> 4090. >>> >>> Due to these nonsense values, the current rate plugin should be treated >>> as non-rewindable. That's why the new callbacks return 0. >>> >>> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> >> I don't agree. The snd_pcm_rate_move_applptr() should be fixed instead >> this blocking attempt. Do you have any simple use case? > > I don't understand what you mean by a "simple use case". I meant a test case (a simple test program). > And anyway, due to reasons expressed in > http://permalink.gmane.org/gmane.linux.alsa.devel/122179 , I insist that > 0 is the only correct return value even if someone fixes > snd_pcm_rate_move_applptr(), which is not trivial. Also, 0 is not a > regression - it was "crash" previously, and I have not changed the > currently-broken snd_pcm_rate_move_applptr logic (just for the > impossible case if someone does rely on it). > > The full fix, after which I do agree to change 0 to something else, > would involve: > > 1. Writing a rewindable resampler library, or a resampler library that > offers a public API to save its state. > 2. Dropping all other resampler implementations. > 3. Extending the snd_pcm_rate_ops_t structure in order to forward either > rewinds or save-requests to the resampler. > 4. Using these new callbacks. > > This translates to several months, or maybe a year of work at the > current rate. It has to be done, and I do plan to do this. But it should > not block the crash-fix: a suboptimal but valid return value of > snd_pcm_rewindable() is better than a crash inside this function. I see, I agree that the current code is/was tricky and probably broken as you noted. But along with your patch, the code in rewind/forward callbacks should be also removed. I put yours and this patch to the alsa-lib repository: http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=5256e150eb34cf599e79839feaff7398ed67a499 Thanks, Jaroslav
13.06.2014 15:19, Jaroslav Kysela wrote: > I see, I agree that the current code is/was tricky and probably broken > as you noted. > > But along with your patch, the code in rewind/forward callbacks should > be also removed. I put yours and this patch to the alsa-lib repository: > > http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=5256e150eb34cf599e79839feaff7398ed67a499 Thanks, that's indeed a logical step.
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 2eb4b1b..58ed842 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -689,6 +689,16 @@ static int snd_pcm_rate_reset(snd_pcm_t *pcm) return 0; } +static snd_pcm_sframes_t snd_pcm_rate_rewindable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{ + return 0; +} + +static snd_pcm_sframes_t snd_pcm_rate_forwardable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{ + return 0; +} + static snd_pcm_sframes_t snd_pcm_rate_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) { snd_pcm_rate_t *rate = pcm->private_data; @@ -1221,7 +1231,9 @@ static const snd_pcm_fast_ops_t snd_pcm_rate_fast_ops = { .drop = snd_pcm_generic_drop, .drain = snd_pcm_rate_drain, .pause = snd_pcm_generic_pause, + .rewindable = snd_pcm_rate_rewindable, .rewind = snd_pcm_rate_rewind, + .forwardable = snd_pcm_rate_forwardable, .forward = snd_pcm_rate_forward, .resume = snd_pcm_generic_resume, .writei = snd_pcm_mmap_writei,
This commit does not fix nonsense values returned by the rewind and forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, an attempt to rewind 1024 samples from the nearly-full buffer returns 4090. Due to these nonsense values, the current rate plugin should be treated as non-rewindable. That's why the new callbacks return 0. Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> --- src/pcm/pcm_rate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)