After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764631 - GstAudioDecoder produce invalid timestamps when PLC and delay
GstAudioDecoder produce invalid timestamps when PLC and delay
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 763058
 
 
Reported: 2016-04-05 10:42 UTC by Mikhail Fludkov
Modified: 2016-06-16 10:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
temporary fix+tests (8.90 KB, patch)
2016-04-05 10:42 UTC, Mikhail Fludkov
none Details | Review
fix+test (9.49 KB, patch)
2016-04-06 12:28 UTC, Mikhail Fludkov
none Details | Review
fix+test (9.49 KB, patch)
2016-05-04 10:17 UTC, Mikhail Fludkov
committed Details | Review

Description Mikhail Fludkov 2016-04-05 10:42:55 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.
Comment 1 Mikhail Fludkov 2016-04-05 11:13:27 UTC
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 2 Vincent Penquerc'h 2016-04-06 10:00:37 UTC
Comment on attachment 325414 [details] [review]
temporary fix+tests

Looks ok.
Comment 3 Mikhail Fludkov 2016-04-06 11:13:58 UTC
(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.
Comment 4 Mikhail Fludkov 2016-04-06 12:28:11 UTC
Created attachment 325477 [details] [review]
fix+test

Here is updated patch with the proper fix. It was much easier than I expected :)
Comment 5 Mikhail Fludkov 2016-05-04 10:17:16 UTC
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?
Comment 6 Vincent Penquerc'h 2016-06-16 10:07:06 UTC
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.
Comment 7 Tim-Philipp Müller 2016-06-16 10:14:06 UTC
Please git commit --amend commits to include the bug url before pushing :)