GNOME Bugzilla – Bug 764631
GstAudioDecoder produce invalid timestamps when PLC and delay
Last modified: 2016-06-16 10:14:06 UTC
Created attachment 325414 [details] [review] temporary fix+tests Elements inherited from GstAudioDecoder, supporting PLC and introducing delay produce invalid timestamps. Good example is opusdec with in-band FEC enabled. After receiving GAP event it delays the audio concealment until the next buffer arrives. The next buffer will have DISCONT flag set which will make GstAudioDecoder to reset it's internal state, thus forgetting the timestamp of GAP event. As a result the concealed audio will have the timestamp of the next buffer (with DISCONT flag) but not the timestamp from the event. See https://bugzilla.gnome.org/show_bug.cgi?id=763058 for the opusdec test case The patch is what we have in our repo as a workaround for the issue. But it does not feel as a complete solution, so I suggest to discuss how it can be fixed properly.
The idea behind the patch was that if the GstAudioDecoder subclass configured to do PLC, we should not reset the state if the buffer with DISCONT flag comes right after GAP event, because it is expected. It fixes the described timestamping issue. And it removes DISCONT flag from outgoing buffers, wich also makes sence, because concealed audio supposed to fill the gap and eliminate discontinuity. If we agree to go forward with the fix, I'll modify the patch to work in situations when there is no delay (see TODO comment in audiodecoder_plc_on_gap_event test).
Comment on attachment 325414 [details] [review] temporary fix+tests Looks ok.
(In reply to Vincent Penquerc'h from comment #2) > Comment on attachment 325414 [details] [review] [review] > temporary fix+tests > > Looks ok. Good. So I'll continue the work on the fix and provide a proper patch soon.
Created attachment 325477 [details] [review] fix+test Here is updated patch with the proper fix. It was much easier than I expected :)
Created attachment 327279 [details] [review] fix+test I've rebased my patch on top of the latest master. Did somebody had a chance to look at the updated patch?
Pushed, thanks: commit 8d4f79b640071b24aa14f0c3e97c03bdb62d002c Author: Mikhail Fludkov <misha@pexip.com> Date: Tue Apr 5 12:41:45 2016 +0200 audiodecoder: fix invalid timestamps when PLC and delay Elements inherited from GstAudioDecoder, supporting PLC and introducing delay produce invalid timestamps. Good example is opusdec with in-band FEC enabled. After receiving GAP event it delays the audio concealment until the next buffer arrives. The next buffer will have DISCONT flag set which will make GstAudioDecoder to reset it's internal state, thus forgetting the timestamp of GAP event. As a result the concealed audio will have the timestamp of the next buffer (with DISCONT flag) but not the timestamp from the event.
Please git commit --amend commits to include the bug url before pushing :)