Created attachment 308628 [details] [review]
opusdec: fix in-band FEC

In-band FEC reconstruction code path was never taken.

1) last_buffer is usually not NULL but a zero-length buffer. In my tests with 'identity drop-probability=0.1', we'd never enter the FEC reconstruction code path.

2) opus_*decode manual states:

"In the case of PLC (data==NULL) or FEC (decode_fec=1), then frame_size needs to be exactly the duration of audio that is missing, otherwise the decoder will not be in the optimal state to decode the next incoming packet."

Therefore, I calculate the number of missing samples from gap buffer's duration.

For some reason, this function expects buffer to be NULL, which complicates the code. GstAudioDecoder sends zero-length buffers (not NULL buffers) for gap events. In which case can we expect a NULL buffer? Due to the possibility of getting NULL buffers I've added the 'last_samples' field. If buffer is never NULL, I'd happily delete it, and a bunch of other code.

You may use this for reference:
https://git.xiph.org/?p=opus.git;a=blob;f=src/opus_demo.c;h=9be3bf78db11ccad0a2f71a3f370078a864d8798;hb=refs/heads/master

Actually, this code seems to be broken if buffer == NULL is possible:

  /* That's the buffer we'll be sending to the opus decoder. */
  buf = (dec->use_inband_fec
      && gst_buffer_get_size (dec->last_buffer) >
      0) ? dec->last_buffer : buffer;

In this case, gst_buffer_get_size (dec->last_buffer) can be invalid.

Taking GST_BUFFER_DURATION of the last buffer doesn't work as well for gap events generated by RTP jitterbuffer (as it did for identity drop-probability).

Just storing the number of samples from the last (successful) packet decode seems to work, though.

There are fixes in https://bugzilla.gnome.org/show_bug.cgi?id=725167 which have not been pushed yet.

Oh, seems they were actually, I'd missed that, nevermind.

Oh my, I should pull more often :) Too bad I spent time working on this.

My request to stop treating buffer == NULL as legit stands though; i.e. let's turn this into a g_return_val_if_fail.

Why is this listed in gst-plugins-base? opus is located in gst-plugins-bad.

Ilya, there were fixes with PLC/FEC recently, I think this can be closed, unless you still see bugs in latest master.

Marking as obsolete as per comment #10, please re-open if you still have problems with recent versions, thanks.