Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks

[Jaroslav Kysela <perex@perex.cz>]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel