Created attachment 280173 [details]
before the patch, F64LE format

Created attachment 280174 [details]
before the patch, S32LE format

Created attachment 280175 [details]
after the patch, F64LE format

Created attachment 280176 [details]
after the patch, S32LE format

Thanks for the patch and the bug report. What would be superb is a unit test for this in tests/check/elements/audioresample.c - how difficult would you think this would be to do?

Created attachment 280192 [details] [review]
add sample count check

I don't know how to define a unit test to detect the problem fixed in this bug report, but this patch here at least adds a safety check that detects a mismatch between the input and output sample counts (accounting for latency and sample rate conversion).  there's no check that the contents of the samples are correct, but any dropping of input samples or double-counting of input samples will be detected by this.

(In reply to comment #6)
> Created an attachment (id=280192) [details] [review]
> add sample count check
> 
> I don't know how to define a unit test to detect the problem fixed in this bug
> report, but this patch here at least adds a safety check that detects a
> mismatch between the input and output sample counts (accounting for latency and
> sample rate conversion).  there's no check that the contents of the samples are
> correct, but any dropping of input samples or double-counting of input samples
> will be detected by this.

So ... thinking about this some more, I'd be concerned that this check will fail if the quality is changed mid stream.  Increasing the quality will increase the latency and, eventually, this test will pass again but there might be an intermediate period when the output appears to have generated more samples than there should have been (because they were generated using the earlier, shorter, kernel).

The problem with a unit test approach is that special buffer sizes are needed to trigger the failure, so the test would have to involve "all" (however that gets defined) buffer sizes.  Leaving an assert in the code to be tripped in the wild is a more certain way to detect problems like this --- crowd-source the unit test --- but it seems that even getting that right is difficult.

Ping.  Any word on getting this patch included upstream?  Thanks.

You could write a unit test that just feeds audioresample with randomly sized buffers manually, instead of creating a pipeline that feeds it from audiotestsrc. That should allow to trigger the bug

Comment on attachment 280192 [details] [review]
add sample count check

Rejected as this will cause problems when the quality changes mid-stream

Pushed but it would be good if you could provide a unit test to prevent that we introduce changes later that break it again :) Keeping this bug open for now.


commit 684cf44ee3c8ccdbcc2f5711f050ba57d9909183
Author: Kipp Cannon <kipp.cannon@ligo.org>
Date:   Tue Jul 8 12:37:41 2014 -0400

    audioresample: don't skip input samples
    
    when downsampling, the output buffer can be filled before all the input
    samples are consumed.  this is correct:  when downsampling, several input
    samples are needed for each output sample, so when only a small number of
    input samples are available the number of output samples produced can be 0.
    
    the resampler, however, was discarding those extra input samples instead of
    clocking them into its filter history for the next iteration.  this patch
    fixes this by removing the check that the output buffer is full.  the code
    now always loops until all input samples are consumed, and relies on the
    calling code to have provided a suitably sized location for the output.
    note that there are already other checks in place in the calling code to
    ensure that this is the case.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=732908

This has now caused the resampler to lock up during draining here because it wasn't able to consume one last input sample.

All output samples were written, so *ochunk = olen = 0, but one last input sample remained, so *ichunk = ilen = 0.

speex_resampler_process_native reduced *ichunk to 0, so the while(ilen) never terminated.

(In reply to comment #12)
Ah, ilen = 1, of course.

Created attachment 294495 [details] [review]
proposed patch

How about this?

audioresample: Try to prevent endless looping
   
Speex may decide not to consume any samples because it can't write any. I've seen a hang during draining caused by the resample loop never terminating. In that case, resampling happened as normal until olen was 0 but ilen was still 1. _process_native then reduced ichunk to 0, so ilen never decreased below 1 and the loop never terminated.
    
Instead of reverting 684cf44 ({audioresample: don't skip input samples), break only if all output samples have been produced and speex refuses to consume any more input samples.

Kipp, can you check if that patch still works with your use case?

(In reply to comment #15)
> Kipp, can you check if that patch still works with your use case?

Hi,

Re-ran my tests with the proposed patch applied and it looks OK to me.  Thanks.

Thanks for testing! :)

commit a636c39638e7d667d71ec5a1070c3e468cf45a73
Author: Jan Alexander Steffens (heftig) <jsteffens@make.tv>
Date:   Tue Jan 13 16:07:06 2015 +0100

    audioresample: Try to prevent endless looping
    
    Speex may decide not to consume any samples because it can't write any. I've
    seen a hang during draining caused by the resample loop never terminating.
    In that case, resampling happened as normal until olen was 0 but ilen was
    still 1. _process_native then reduced ichunk to 0, so ilen never decreased
    below 1 and the loop never terminated.
    
    Instead of reverting 684cf44 ({audioresample: don't skip input samples),
    break only if all output samples have been produced and speex refuses
    to consume any more input samples.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=732908