okay, sorry that last patch wasn't quite right :(. I think this one makes more sense I thought I had worked out what needed to be done, but it turns out that there was already an output latency function that wasn't exposed.  This patch exposes it.  It ivolves changes to speex_resampler_wrapper.h also

diff --git a/gst/audioresample/gstaudioresample.c b/gst/audioresample/gstaudioresample.c
index 0cae09e..b6c2ed4 100644
--- a/gst/audioresample/gstaudioresample.c
+++ b/gst/audioresample/gstaudioresample.c
@@ -929,6 +929,10 @@ gst_audio_resample_process (GstAudioResample * resample, GstBuffer * inbuf,
   gint err = RESAMPLER_ERR_SUCCESS;
   guint8 *in_tmp = NULL, *out_tmp = NULL;
   gboolean need_convert = (resample->funcs->width != resample->width);
+  gboolean skip_gap;
+  guint32 in_latency, out_latency;
+
+  in_latency = resample->funcs->get_input_latency (resample->state);

   in_len = GST_BUFFER_SIZE (inbuf) / resample->channels;
   out_len = GST_BUFFER_SIZE (outbuf) / resample->channels;
@@ -939,6 +943,18 @@ gst_audio_resample_process (GstAudioResample * resample, GstBuffer * inbuf,
   in_processed = in_len;
   out_processed = out_len;

+  out_latency =
+      resample->funcs->get_output_latency (resample->state) +
+      (in_processed % out_processed);
+
+  if (resample->inrate < resample->outrate) {
+    skip_gap = in_len > in_latency
+        && GST_BUFFER_FLAG_IS_SET (inbuf, GST_BUFFER_FLAG_GAP);
+  } else {
+    skip_gap = out_len > out_latency
+        && GST_BUFFER_FLAG_IS_SET (inbuf, GST_BUFFER_FLAG_GAP);
+  }
+
   if (need_convert) {
     guint in_size_tmp =
         in_len * resample->channels * (resample->funcs->width / 8);
@@ -963,13 +979,20 @@ gst_audio_resample_process (GstAudioResample * resample, GstBuffer * inbuf,
     }
   }

-  if (need_convert) {
-    err = resample->funcs->process (resample->state,
-        in_tmp, &in_processed, out_tmp, &out_processed);
+  /* don't bother with the resample filter if it is a gap */
+  if (skip_gap) {
+    GST_BUFFER_FLAG_SET (outbuf, GST_BUFFER_FLAG_GAP);
+    memset (GST_BUFFER_DATA (outbuf), 0, GST_BUFFER_SIZE (outbuf));
+    err = 0;
   } else {
-    err = resample->funcs->process (resample->state,
-        (const guint8 *) GST_BUFFER_DATA (inbuf), &in_processed,
-        (guint8 *) GST_BUFFER_DATA (outbuf), &out_processed);
+    if (need_convert) {
+      err = resample->funcs->process (resample->state,
+          in_tmp, &in_processed, out_tmp, &out_processed);
+    } else {
+      err = resample->funcs->process (resample->state,
+          (const guint8 *) GST_BUFFER_DATA (inbuf), &in_processed,
+          (guint8 *) GST_BUFFER_DATA (outbuf), &out_processed);
+    }
   }

   if (G_UNLIKELY (in_len != in_processed))
@@ -994,6 +1017,12 @@ gst_audio_resample_process (GstAudioResample * resample, GstBuffer * inbuf,
     return GST_FLOW_ERROR;
   } else {

+    if (skip_gap) {
+      out_processed -= out_latency;
+      gst_audio_resample_push_drain (resample);
+      gst_audio_resample_reset_state (resample);
+    }
+
     if (need_convert)
       gst_audio_resample_convert_buffer (resample, out_tmp,
           GST_BUFFER_DATA (outbuf), out_processed, TRUE);


diff --git a/gst/audioresample/speex_resampler_wrapper.h b/gst/audioresample/speex_resampler_wrapper.h
index 36d444f..e15eb2c 100644
--- a/gst/audioresample/speex_resampler_wrapper.h
+++ b/gst/audioresample/speex_resampler_wrapper.h
@@ -52,6 +52,7 @@ typedef struct {
   void (*get_ratio) (SpeexResamplerState * st,
     guint32 * ratio_num, guint32 * ratio_den);
   int (*get_input_latency) (SpeexResamplerState * st);
+  int (*get_output_latency) (SpeexResamplerState * st);
   int (*set_quality) (SpeexResamplerState * st, gint quality);
   int (*reset_mem) (SpeexResamplerState * st);
   int (*skip_zeros) (SpeexResamplerState * st);
@@ -71,6 +72,7 @@ void resample_float_resampler_get_rate (SpeexResamplerState * st,
 void resample_float_resampler_get_ratio (SpeexResamplerState * st,
     guint32 * ratio_num, guint32 * ratio_den);
 int resample_float_resampler_get_input_latency (SpeexResamplerState * st);
+int resample_float_resampler_get_output_latency (SpeexResamplerState *st);
 int resample_float_resampler_set_quality (SpeexResamplerState * st, gint quality);
 int resample_float_resampler_reset_mem (SpeexResamplerState * st);
 int resample_float_resampler_skip_zeros (SpeexResamplerState * st);
@@ -85,6 +87,7 @@ static const SpeexResampleFuncs float_funcs =
   resample_float_resampler_get_rate,
   resample_float_resampler_get_ratio,
   resample_float_resampler_get_input_latency,
+  resample_float_resampler_get_output_latency,
   resample_float_resampler_set_quality,
   resample_float_resampler_reset_mem,
   resample_float_resampler_skip_zeros,
@@ -104,6 +107,7 @@ void resample_double_resampler_get_rate (SpeexResamplerState * st,
 void resample_double_resampler_get_ratio (SpeexResamplerState * st,
     guint32 * ratio_num, guint32 * ratio_den);
 int resample_double_resampler_get_input_latency (SpeexResamplerState * st);
+int resample_double_resampler_get_output_latency (SpeexResamplerState *st);
 int resample_double_resampler_set_quality (SpeexResamplerState * st, gint quality);
 int resample_double_resampler_reset_mem (SpeexResamplerState * st);
 int resample_double_resampler_skip_zeros (SpeexResamplerState * st);
@@ -118,6 +122,7 @@ static const SpeexResampleFuncs double_funcs =
   resample_double_resampler_get_rate,
   resample_double_resampler_get_ratio,
   resample_double_resampler_get_input_latency,
+  resample_double_resampler_get_output_latency,
   resample_double_resampler_set_quality,
   resample_double_resampler_reset_mem,
   resample_double_resampler_skip_zeros,
@@ -137,6 +142,7 @@ void resample_int_resampler_get_rate (SpeexResamplerState * st,
 void resample_int_resampler_get_ratio (SpeexResamplerState * st,
     guint32 * ratio_num, guint32 * ratio_den);
 int resample_int_resampler_get_input_latency (SpeexResamplerState * st);
+int resample_int_resampler_get_output_latency (SpeexResamplerState * st);
 int resample_int_resampler_set_quality (SpeexResamplerState * st, gint quality);
 int resample_int_resampler_reset_mem (SpeexResamplerState * st);
 int resample_int_resampler_skip_zeros (SpeexResamplerState * st);
@@ -151,6 +157,7 @@ static const SpeexResampleFuncs int_funcs =
   resample_int_resampler_get_rate,
   resample_int_resampler_get_ratio,
   resample_int_resampler_get_input_latency,
+  resample_int_resampler_get_output_latency,
   resample_int_resampler_set_quality,
   resample_int_resampler_reset_mem,
   resample_int_resampler_skip_zeros,

Created attachment 138091 [details]
Demo of behaviour in gaps

This shows an example of the resampler's input and output time series with the proposed patch applied.  Gaps have been inserted periodically into the data stream, and to make things difficult the gap buffer boundaries are not commensurate with the output sample period (they are, of course, commensurate with the input sample period since there is always an integer number of samples in a buffer).

The graph shows all non-gap samples, with connect-the-dots lines between them (so there are big diagonals through the gaps).  Blue is the input, red is the down-sampled output.  The resampler performs well:  there is no phase shift accumulated in the gaps and the output settles down to an accurate approximation of the input very soon after the end of the gap interval.

Sometimes the resampler begins generating output a little in advance of the end of the gap intervals.  Sometimes so far in advance that in this demo it fills in the gap entirely.  The cause of this, and a solution, is still being investigated, but without the patch the resampler runs continuously anyway so it's still an improvement.

Could you attach your current patch as file to this bugreport? Best as git format-patch style patch ;)

I'm going to review it in the next days then.

Created attachment 138671 [details] [review]
A patch for adding GAP support to the audioresample plugin

I didn't have time yet, but it might be nice to add a flag to turn this on as a property to this element so that people can choose not to use it.  I can work on it if you like.

I'm not 100% conviced by this patch ;)

You're setting the output to zeroes if the gap can be skipped but the resampler internal filter line will still have the old input data. Won't this result in suboptimal output if processing continues because it doesn't interpolate from silence but from the older input data?

Then you drain the resampler. Draining the resampler will result in suboptimal output in the last few samples. Is this important here or will these last few samples always be silence (from the GAP buffer)?

And then I wonder if draining the resampler and then continue using it later won't introduce additional latency.

Created attachment 148849 [details] [review]
This is the most recent patch allowing gap support for up sampling.

Created attachment 148850 [details]
A plot showing the resampler ouput just before a gap

Here the red trace is the up-sampled data, and the black trace is the original data.  A gap was added to the original data when the green trace crossed -0.75.  You can see the resampled data turns on cleanly (the plotting program connected the dots to a previous non gap buffer which is why it doesn't go to zero).  This boundary was after several gap/nongap buffers and there appears to be no accumulated phase drift.

Created attachment 148851 [details]
A plot showing the resampler ouput just after a gap

Here the red trace is the up-sampled data, and the black trace is the original data.  A gap was added to the original data when the green trace crossed -0.75.  You can see the resampled data turns off cleanly (the plotting program connected the dots to a previous non gap buffer which is why it doesn't go to zero).  This boundary was after several gap/nongap buffers and there appears to be no accumulated phase drift.

(In reply to comment #5)
> I'm not 100% conviced by this patch ;)
> 
> You're setting the output to zeroes if the gap can be skipped but the resampler
> internal filter line will still have the old input data. Won't this result in
> suboptimal output if processing continues because it doesn't interpolate from
> silence but from the older input data?

I think this is an interesting point.  To me it isn't clear what the right thing to do is.  If a user's pipeline previously memsets the gaps to zero, then it is not a problem.  If they do not, then perhaps the desired behavior is to use what was in the gap.  It may contain the correct information for the resampler to use.  In our application it does.  What do you think?   


> Then you drain the resampler. Draining the resampler will result in suboptimal
> output in the last few samples. Is this important here or will these last few
> samples always be silence (from the GAP buffer)?
> 
> And then I wonder if draining the resampler and then continue using it later
> won't introduce additional latency.

Reopening as requested information might have been provided.

The gap-buffer should be all zero. Otherwise it would not work for non-gap-aware plugins.

Created attachment 169879 [details] [review]
New patch, supporting both upsampling and downsampling, and with a new treatment of the filter transient just before and after a gap.

This new patch supports both upsampling and downsampling, and arbitrary input and output sample rates.

It also treats the filter transient before and after an input gap a little differently than before.  It preserves the transient, and marks it as non-gap.  The result should be that if you downsample, then upsample, you will loose less information.

Review of attachment 169879 [details] [review]:

Looks good in general

::: gst/audioresample/gstaudioresample.c
@@ +198,3 @@
+      g_param_spec_boolean ("gap-aware", "Gap Aware", "true = skip buffers " \
+          "flagged as gaps;  false = process all buffers", FALSE,
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT));

Please don't add a property for this. The gap-aware mode should be the default and only mode. If it works correctly there's no reason to disable it.

Created attachment 170220 [details] [review]
Updated patch; "gap-aware" property removed.

As requested, the attached patch makes audioresample always gap-aware and removes the property that I had added.

Created attachment 170221 [details]
Gap behavior on upsampling

This figure shows the element's response to gaps upon upsampling.  The pink areas are gaps.  Note that the output gaps are shrunk to preserve the filter transients.  The input signal that I used here (pink noise) does not go to zero at the gap boundaries, so the filter response oscillates a bit as expected.

Created attachment 170222 [details]
Gap behavior on downsampling

Shows filter's response to gaps (marked pink) on downsampling.

Unfortunately this patch makes the unit test for audioresample fail (tests/check/elements/audioresample):


$ make elements/audioresample.check
  CC     elements_audioresample-audioresample.o
  CCLD   elements/audioresample
Running suite(s): audioresample
62%: Checks: 8, Failures: 3, Errors: 0
elements/audioresample.c:156:F:general:test_perfect_stream:0: 'timestamp' (0) is not equal to 'GST_BUFFER_TIMESTAMP (buffer)' (18446744073708218283)
elements/audioresample.c:310:F:general:test_discont_stream:0: 'ints' (0) is not equal to 'GST_BUFFER_TIMESTAMP (outbuffer)' (18446744073708218283)
elements/audioresample.c:811:F:general:test_timestamp_drift:0: expected output timestamp 0:00:00.244140625 (244140625) got output timestamp 0:00:00.236328125 (236328125)
Running suite(s): audioresample
0:00:00.146490264  3264      0x19a33e0 WARN           basetransform gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample> pad-alloc failed: wrong-state
0:00:00.146539083  3264      0x19a33e0 WARN           basetransform gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could not get buffer from pool: wrong-state
0:00:00.157771447  3264 0x2b90e8003270 WARN           basetransform gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample> pad-alloc failed: wrong-state
0:00:00.157823269  3264 0x2b90e8003270 WARN           basetransform gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could not get buffer from pool: wrong-state
0:00:00.199778672  3264 0x2b90e8003270 WARN           basetransform gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample> pad-alloc failed: wrong-state
0:00:00.199800882  3264 0x2b90e8003270 WARN           basetransform gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could not get buffer from pool: wrong-state
0:00:00.222552028  3264 0x2b90e8003270 WARN           basetransform gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample> pad-alloc failed: wrong-state
0:00:00.222586111  3264 0x2b90e8003270 WARN           basetransform gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could not get buffer from pool: wrong-state
0:00:00.231833794  3264      0x19a33e0 WARN           basetransform gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample> pad-alloc failed: wrong-state
0:00:00.231868854  3264      0x19a33e0 WARN           basetransform gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could not get buffer from pool: wrong-state
0:00:00.283478911  3267      0x191a010 WARN           audioresample gstaudioresample.c:1014:gst_audio_resample_check_discont:<audioresample> encountered timestamp discontinuity of 19992 samples = 0:00:00.399840000
62%: Checks: 8, Failures: 3, Errors: 0
elements/audioresample.c:156:F:general:test_perfect_stream:0: 'timestamp' (0) is not equal to 'GST_BUFFER_TIMESTAMP (buffer)' (18446744073708218283)
elements/audioresample.c:310:F:general:test_discont_stream:0: 'ints' (0) is not equal to 'GST_BUFFER_TIMESTAMP (outbuffer)' (18446744073708218283)
elements/audioresample.c:811:F:general:test_timestamp_drift:0: expected output timestamp 0:00:00.244140625 (244140625) got output timestamp 0:00:00.236328125 (236328125)
make: *** [elements/audioresample.check] Error 3

Review of attachment 170220 [details] [review]:

Nice work! I added a few comments and it would be cool if you could change why the tests fail.
Also, please also drop the "common" change from the patch.

::: gst/audioresample/gstaudioresample.c
@@ +790,3 @@
+  guint out_len, out_processed;
+  guint num, den;
+  void* buf;

s/void*/gpointer/ or just use "guint8 *" as this is what the actual functions take

@@ +806,3 @@
+  resample->funcs->process (resample->state, NULL, &in_processed, buf, &out_processed);
+  g_free(buf);
+

wouldn't you need to zero buf? And if so, wouldn'ät it be easier to support passing NULL and make process writing zeros in that case?

@@ +807,3 @@
+  g_free(buf);
+
+  g_assert(in_len == in_processed);

that makes no sense here, as you just set this above.

::: gst/audioresample/gstaudioresample.h
@@ +67,3 @@
+  guint count_gap;
+  guint count_nongap;
+

these are not really counting gap/nongap, right? What about

guint num_gap_samples;
guint num_{non_gap,reg,norm}_samples;
(not sure about the 2nd)

::: gst/audioresample/speex_resampler.h
@@ +338,3 @@
+ * @param st Resampler state
+ */
+int speex_resampler_get_filt_len(SpeexResamplerState *st);

please use normal /* */ comments unless it is really a gtk-doc style formatted comment. The above one will cause a gtk-doc warning.

Here is why these three unit tests are failing.  The effect of this patch is to shrink each gap by half a filter-length on either side.  I decided to treat the first buffer of a new segment as if it had been preceded by a gap, so upon output there is an extra half-filter-length of non-gap data at the beginning of the stream.

The third unit test fails for that reason: the way in which it calculates the output latency no longer works.  Telling the unit test explicitly how much latency to expect results in the unit test passing; see the patch below.

The first and second unit test failures occur for the following reason: If the input stream's timestamps started from zero, the output stream's timestamps would have to start at minus a half filter length.  Since timestamps are unsigned, this results in integer overflow, which is why you see the timestamp 18446744073708218283.

Should I alter my patch so that the first half-filter-length of transient response at the beginning of a new segment is discarded?



The following patch fixes the third unit test failure; the first two unit test failures remain.

diff --git a/tests/check/elements/audioresample.c b/tests/check/elements/audioresample.c
index a9a2e3b..0fc025c 100644
--- a/tests/check/elements/audioresample.c
+++ b/tests/check/elements/audioresample.c
@@ -768,9 +768,6 @@ fakesink_handoff_cb (GstElement * object, GstBuffer * buffer, GstPad * pad,
   TimestampDriftCtx *ctx = user_data;
 
   ctx->out_buffer_count++;
-  if (ctx->latency == GST_CLOCK_TIME_NONE) {
-    ctx->latency = 1000 - GST_BUFFER_SIZE (buffer) / 8;
-  }
 
   /* Check if we have a perfectly timestampped stream */
   if (ctx->next_out_ts != GST_CLOCK_TIME_NONE)
@@ -838,6 +835,7 @@ GST_START_TEST (test_timestamp_drift)
   GstBus *bus;
   GMainLoop *loop;
   GstCaps *caps;
+  gint filter_length;
 
   pipeline = gst_pipeline_new ("pipeline");
   fail_unless (pipeline != NULL);
@@ -870,6 +868,8 @@ GST_START_TEST (test_timestamp_drift)
       ("audio/x-raw-float, channels=1, width=64, rate=4096");
   g_object_set (G_OBJECT (capsfilter2), "caps", caps, NULL);
   gst_caps_unref (caps);
+  g_object_get (G_OBJECT (audioresample), "filter-length", &filter_length, NULL);
+  ctx.latency = filter_length / 2;
 
   fakesink = gst_element_factory_make ("fakesink", "sink");
   fail_unless (fakesink != NULL);


(In reply to comment #17)
> Unfortunately this patch makes the unit test for audioresample fail
> (tests/check/elements/audioresample):
> 
> 
> $ make elements/audioresample.check
>   CC     elements_audioresample-audioresample.o
>   CCLD   elements/audioresample
> Running suite(s): audioresample
> 62%: Checks: 8, Failures: 3, Errors: 0
> elements/audioresample.c:156:F:general:test_perfect_stream:0: 'timestamp' (0)
> is not equal to 'GST_BUFFER_TIMESTAMP (buffer)' (18446744073708218283)
> elements/audioresample.c:310:F:general:test_discont_stream:0: 'ints' (0) is not
> equal to 'GST_BUFFER_TIMESTAMP (outbuffer)' (18446744073708218283)
> elements/audioresample.c:811:F:general:test_timestamp_drift:0: expected output
> timestamp 0:00:00.244140625 (244140625) got output timestamp 0:00:00.236328125
> (236328125)
> Running suite(s): audioresample
> 0:00:00.146490264  3264      0x19a33e0 WARN           basetransform
> gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample>
> pad-alloc failed: wrong-state
> 0:00:00.146539083  3264      0x19a33e0 WARN           basetransform
> gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could
> not get buffer from pool: wrong-state
> 0:00:00.157771447  3264 0x2b90e8003270 WARN           basetransform
> gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample>
> pad-alloc failed: wrong-state
> 0:00:00.157823269  3264 0x2b90e8003270 WARN           basetransform
> gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could
> not get buffer from pool: wrong-state
> 0:00:00.199778672  3264 0x2b90e8003270 WARN           basetransform
> gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample>
> pad-alloc failed: wrong-state
> 0:00:00.199800882  3264 0x2b90e8003270 WARN           basetransform
> gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could
> not get buffer from pool: wrong-state
> 0:00:00.222552028  3264 0x2b90e8003270 WARN           basetransform
> gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample>
> pad-alloc failed: wrong-state
> 0:00:00.222586111  3264 0x2b90e8003270 WARN           basetransform
> gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could
> not get buffer from pool: wrong-state
> 0:00:00.231833794  3264      0x19a33e0 WARN           basetransform
> gstbasetransform.c:1558:gst_base_transform_prepare_output_buffer:<audioresample>
> pad-alloc failed: wrong-state
> 0:00:00.231868854  3264      0x19a33e0 WARN           basetransform
> gstbasetransform.c:2219:gst_base_transform_handle_buffer:<audioresample> could
> not get buffer from pool: wrong-state
> 0:00:00.283478911  3267      0x191a010 WARN           audioresample
> gstaudioresample.c:1014:gst_audio_resample_check_discont:<audioresample>
> encountered timestamp discontinuity of 19992 samples = 0:00:00.399840000
> 62%: Checks: 8, Failures: 3, Errors: 0
> elements/audioresample.c:156:F:general:test_perfect_stream:0: 'timestamp' (0)
> is not equal to 'GST_BUFFER_TIMESTAMP (buffer)' (18446744073708218283)
> elements/audioresample.c:310:F:general:test_discont_stream:0: 'ints' (0) is not
> equal to 'GST_BUFFER_TIMESTAMP (outbuffer)' (18446744073708218283)
> elements/audioresample.c:811:F:general:test_timestamp_drift:0: expected output
> timestamp 0:00:00.244140625 (244140625) got output timestamp 0:00:00.236328125
> (236328125)
> make: *** [elements/audioresample.check] Error 3

(In reply to comment #19)
> Here is why these three unit tests are failing.  The effect of this patch is to
> shrink each gap by half a filter-length on either side.  I decided to treat the
> first buffer of a new segment as if it had been preceded by a gap, so upon
> output there is an extra half-filter-length of non-gap data at the beginning of
> the stream.

You shouldn't handle the first buffers after a newsegment event as gap and you should never output the half-filter-length samples at the beginning after initializing the resampler.
If you put 1024 samples into the resampler with filter length 8, you should output exactly ratio*1024 samples, produced from the 1024 samples and dropping ratio*4 at the beginning and ratio*4 samples at the end produced from some silence.

I have all of the unit tests passing now; I'll provide a new version shortly.  I noticed that an earlier version of my patch was accidentally pushed on master, then reverted, so I'll provide a set of patches.  The first will revert the revert, the second will make the unit tests pass, and the remainder will address Stefan's comments individually.

(In reply to comment #18)
> wouldn't you need to zero buf? And if so, wouldn'ät it be easier to support
> passing NULL and make process writing zeros in that case?
> 
> @@ +807,3 @@
> +  g_free(buf);
> +
> +  g_assert(in_len == in_processed);
> 
> that makes no sense here, as you just set this above.

Actually, buf is storage for the output of resample->funcs->process, which is just discarded in gst_audio_resample_dump_drain.

Any news on this?

(In reply to comment #22)
> Any news on this?

Yes: I have an updated patch with which the unit tests pass.  I've attached the additional commits in a tarball.

However, the new patches are acting up with some of our in-house elements, and I want to make sure it's our in-house elements that are at fault and not audioresample.

Created attachment 176302 [details]
new commits relative to master

What exactly is the wrong behaviour you see with your in-house elements?

(In reply to comment #25)
> What exactly is the wrong behaviour you see with your in-house elements?

I am scrutinizing how this patch affects recovery from a discontinuity.

(In reply to comment #25)
> What exactly is the wrong behaviour you see with your in-house elements?

Sebastian: actually, I suspect that our in-house elements are to blame.  Can you confirm that the unit tests pass now?

Yes, the unit test passes now :)

(In reply to comment #28)
> Yes, the unit test passes now :)

Okay.  I'll do some more tests with our application that has very gappy streams.  It will take a few hours to run, but I'll get back to you today about it.

Ok, thanks :) Could you also update your patches to apply cleanly against latest GIT? Other than that this is ready to be committed IMO

(In reply to comment #30)
> Ok, thanks :) Could you also update your patches to apply cleanly against
> latest GIT? Other than that this is ready to be committed IMO

Yep.  Will do.

Doh!

It turns out that I was having an issue with a stream whose buffer offsets started from a value greater than zero.  I had forgotten to subtract off the initial offset in the if statement shown here:

diff --git a/gst/audioresample/gstaudioresample.c b/gst/audioresample/gstaudioresample.c
index 4623179..93ab0ea 100644
--- a/gst/audioresample/gstaudioresample.c
+++ b/gst/audioresample/gstaudioresample.c
@@ -1052,7 +1052,7 @@ gst_audio_resample_process (GstAudioResample * resample, GstBuffer * inbuf,
     {
       guint num, den;
       resample->funcs->get_ratio (resample->state, &num, &den);
-      if (resample->next_in_offset + in_len >= filt_len / 2)
+      if (resample->next_in_offset - resample->in_offset0 + in_len >= filt_len / 2)
         out_processed =
             gst_util_uint64_scale_int_ceil (resample->next_in_offset -
             resample->in_offset0 + in_len - filt_len / 2, den,

I'll incorporate this into the appropriate patch in that tarball, continue testing, and have another reply shortly.

Created attachment 176580 [details]
new set of commits relative to master

I finished reworking my patches so that they will apply cleanly.

However, in the process I discovered that commit a7cf16528930205967ad0f778b217e8761c836d6 on master introduced a bug in the buffer durations that I have fixed as patch number '0007' in the tarball.

I am now re-running my gap-rich test application, hopefully for the last time.

(In reply to comment #33)
> I am now re-running my gap-rich test application, hopefully for the last time.

Everything looks good to me.

commit 5bfe1baab393d2a0b16329abda1bd1a7f38d4aec
Author: Leo Singer <leo.singer@ligo.org>
Date:   Fri Dec 17 00:49:26 2010 -0800

    audioresample: corrected buffer duration calculation to account for nonzero initial time
    
    Since we calculate timestamps by:
    
      timestamp = t0 + (out samples) / (out rate)
    
    and durations by:
    
      duration = ((out samples) + (processed samples)) / (out rate) - timestamp
    
    if t0 is nonzero, this would simplify to
    
      duration = t0 + (processed samples) / (out rate).
    
    This duration is too large by the amount t0.  We should have done:
    
      duration = t0 + ((out samples) + (processed samples)) / (out rate) - timestamp
    
    so that
    
      duration = (processed samples) / (out rate).


commit 25a154be5f971e86b37c6d4130176d4583625a6d
Author: Leo Singer <leo.singer@ligo.org>
Date:   Thu Dec 16 20:40:33 2010 -0800

    audioresample: changed num_gap_samples, num_nongap_samples from guint32 to guint64 so th

commit d6d2aa44ab5c1cb1a0b83d07d3acf56a312d61a1
Author: Leo Singer <leo.singer@ligo.org>
Date:   Thu Dec 16 20:38:31 2010 -0800

    audioresample: push half a history length, instead of a full history length, at end-of-s

commit aac8b216787083030dd41fe434e11d21c197bda4
Author: Leo Singer <leo.singer@ligo.org>
Date:   Thu Dec 16 20:34:13 2010 -0800

    audioresample: renamed count_gap, count_nongap to more descriptive num_gap_samples, num_

commit 6832b38527fe7af96029a88d5f579eae10c51ba9
Author: Leo Singer <leo.singer@ligo.org>
Date:   Thu Dec 16 20:32:07 2010 -0800

    audioresample: replaced void* with gpointer

commit 87f242273700bfb4ea183056749c73dd9f372e8a
Author: Leo Singer <leo.singer@ligo.org>
Date:   Thu Dec 16 20:30:24 2010 -0800

    audioresample: initial filter transient discarded; unit tests passing


commit b4cd3329a96b934c227b76c6dec955190a4dc3d8
Author: Leo Singer <leo.singer@ligo.org>
Date:   Thu Dec 16 20:09:58 2010 -0800

    Revert "Revert "audioresample: Add GAP flag support""
    
    This reverts commit 35c76b3409dde7f2dcc8232388a47a1b99b661a7.
    
    Conflicts:
    
        gst/audioresample/gstaudioresample.c
        gst/audioresample/gstaudioresample.h