Bug 47156 - Crash on resampler
Summary: Crash on resampler
Status: RESOLVED FIXED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Tanu Kaskinen
QA Contact: pulseaudio-bugs
URL:
Whiteboard: triaged
Keywords:
Depends on:
Blocks: 45812 49714
  Show dependency treegraph
 
Reported: 2012-03-09 07:29 UTC by Cristian Morales Vega
Modified: 2012-05-11 06:23 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Drop the assert (863 bytes, patch)
2012-03-13 16:37 UTC, Colin Guthrie
Details | Splinter Review
[PATCH 1/3] resampler: Use pa_xnew0() to avoid manual zeroing. (3.24 KB, patch)
2012-04-22 11:34 UTC, Tanu Kaskinen
Details | Splinter Review
[PATCH 2/3] resampler: Rename buf3 and buf4 to buf4 and buf5. (4.71 KB, patch)
2012-04-22 11:35 UTC, Tanu Kaskinen
Details | Splinter Review
[PATCH 3/3] resampler: Add support for resamplers that consume less data than asked. (6.67 KB, patch)
2012-04-22 11:36 UTC, Tanu Kaskinen
Details | Splinter Review

Description Cristian Morales Vega 2012-03-09 07:29:45 UTC
Ubuntu has a bug report that seems everybody forgot (https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/885347). But it seems to be a real problem -> http://article.gmane.org/gmane.comp.audio.src.general/109
Comment 1 Colin Guthrie 2012-03-13 16:37:09 UTC
Created attachment 58405 [details] [review]
Drop the assert

Yup. Looking at the code, it would appear we can just drop the assert.

The return value in out_n_frames should still maintain proper integrity of the data.
Comment 2 David Henningsson 2012-03-23 05:36:33 UTC
I tend to agree that skipping the assert seems the easiest way out. 

There is a question though, what happens to the lost/unused sample? Will just skipping it lead to degrading quality? Ideally, it should be in the next block we send to libsamplerate, but fixing this seems tricky.

Also, it seems to happen very rarely, but maybe that's because we ship the speex resampler by default.

So either just skipping the assert, or possibly replace it with debug output telling us that x samples were skipped.
Comment 3 Tanu Kaskinen 2012-04-22 08:17:32 UTC
I'd very much like to avoid skipping any audio, and fixing this properly seems to be feasible. I'm working on a patch that stores the leftover data inside the resampler and adds the leftover to the input in the next pa_resampler_run() invocation.
Comment 4 Tanu Kaskinen 2012-04-22 11:34:54 UTC
Created attachment 60459 [details] [review]
[PATCH 1/3] resampler: Use pa_xnew0() to avoid manual zeroing.
Comment 5 Tanu Kaskinen 2012-04-22 11:35:31 UTC
Created attachment 60460 [details] [review]
[PATCH 2/3] resampler: Rename buf3 and buf4 to buf4 and buf5.
Comment 6 Tanu Kaskinen 2012-04-22 11:36:21 UTC
Created attachment 60461 [details] [review]
[PATCH 3/3] resampler: Add support for resamplers that consume less data than asked.
Comment 7 Tanu Kaskinen 2012-04-25 08:38:01 UTC
Updated patch set sent to the mailing list: http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/13091
Comment 8 Tanu Kaskinen 2012-05-09 21:57:59 UTC
The speex resampler may suffer from this same bug. There's a similar assertion for speex.
Comment 9 Arun Raghavan 2012-05-09 22:34:42 UTC
(In reply to comment #8)
> The speex resampler may suffer from this same bug. There's a similar assertion
> for speex.

Does the speex API actually imply that it may not consume all input data? The documentation suggests that this can only happen during an error:

"""
Unless an error occurs, either all input samples will be read or all output samples will be written to (or both).
"""

from: http://www.speex.org/docs/manual/speex-manual/node7.html#SECTION00760000000000000000
Comment 10 Tanu Kaskinen 2012-05-09 23:23:05 UTC
Patch set v3 sent to the mailing list: http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/13224
Comment 11 Tanu Kaskinen 2012-05-09 23:24:13 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > The speex resampler may suffer from this same bug. There's a similar assertion
> > for speex.
> 
> Does the speex API actually imply that it may not consume all input data? The
> documentation suggests that this can only happen during an error:
> 
> """
> Unless an error occurs, either all input samples will be read or all output
> samples will be written to (or both).
> """
> 
> from:
> http://www.speex.org/docs/manual/speex-manual/node7.html#SECTION00760000000000000000

Thanks for checking the documentation. You are right. The speex changes need to be reverted.
Comment 12 Arun Raghavan 2012-05-11 06:23:11 UTC
Committed Tanu's patches for this.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.