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 619485 - matroskademux: skip buffers before a late keyframe (QoS)
matroskademux: skip buffers before a late keyframe (QoS)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 0.10.24
Assigned To: Sebastian Dröge (slomo)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-05-24 07:29 UTC by Philip Jägenstedt
Modified: 2010-06-01 09:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch 1/3 (2.48 KB, patch)
2010-05-24 07:30 UTC, Philip Jägenstedt
needs-work Details | Review
patch 2/3 (1.80 KB, patch)
2010-05-24 07:30 UTC, Philip Jägenstedt
needs-work Details | Review
patch 3/3 (2.91 KB, patch)
2010-05-24 07:31 UTC, Philip Jägenstedt
needs-work Details | Review
patch 4 (post review) (4.91 KB, patch)
2010-05-24 09:49 UTC, Philip Jägenstedt
none Details | Review
combined patch (5.38 KB, patch)
2010-05-24 10:43 UTC, Philip Jägenstedt
committed Details | Review
deadlock fix (1.82 KB, patch)
2010-05-25 03:42 UTC, Philip Jägenstedt
committed Details | Review
matroskademux: Don't compare running times with stream times when doing QoS (2.12 KB, patch)
2010-05-31 09:24 UTC, Sebastian Dröge (slomo)
none Details | Review
matroskademux: Don't compare running times with stream times when doing QoS (2.26 KB, patch)
2010-06-01 09:06 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Philip Jägenstedt 2010-05-24 07:29:24 UTC
See commit messages of the 3 patches. Not sure of proportion should be used for something, so I dropped it in the 2nd patch.

Other points of uncertainty:

* what happens in reverse playback?
* is the lock necessary?
* should earliest_position be reset in more places?
Comment 1 Philip Jägenstedt 2010-05-24 07:30:09 UTC
Created attachment 161830 [details] [review]
patch 1/3
Comment 2 Philip Jägenstedt 2010-05-24 07:30:49 UTC
Created attachment 161831 [details] [review]
patch 2/3
Comment 3 Philip Jägenstedt 2010-05-24 07:31:09 UTC
Created attachment 161832 [details] [review]
patch 3/3
Comment 4 Sebastian Dröge (slomo) 2010-05-24 07:50:52 UTC
(In reply to comment #3)
> Created an attachment (id=161832) [details] [review]
> patch 3 [details]/3

You should probably check here if the stream actually has an index table and if not search in the global one. IIRC the per-stream index table is only used if the cues have a track set.
Comment 5 Sebastian Dröge (slomo) 2010-05-24 07:53:41 UTC
Oh and you should also use per-stream QoS values. If there are multiple video streams connected to a sink for example, the QoS events can contain completely different values.

And the lock is necessary, yes and I think you reset the QoS values everywhere where this is required.
Comment 6 Philip Jägenstedt 2010-05-24 09:15:35 UTC
I check if the stream has an index here by checking stream->index_table in the first if. It's probably not a good idea to use the global one as a fallback, unless there's data to suggest that it usually (almost always?) contains the keyframes of the video track. In the WebM files I tested, they all seemed have a per-track index.
Comment 7 Philip Jägenstedt 2010-05-24 09:49:42 UTC
Created attachment 161841 [details] [review]
patch 4 (post review)

New patch on top of the previous 3. Let me know if you prefer a combined patch.
Comment 8 Sebastian Dröge (slomo) 2010-05-24 09:53:43 UTC
Review of attachment 161841 [details] [review]:

Looks good, now combine all patches and I'll push this after 0.10.23 release :)

::: gst/matroska/matroska-demux.c
@@ +2199,3 @@
       context->last_flow = GST_FLOW_OK;
+    if (context->type == GST_MATROSKA_TRACK_TYPE_VIDEO) {
+      GstMatroskaTrackVideoContext *videocontext =

Well, do that for all tracks and remove the condition... makes the code a bit better to read IMHO :)
Comment 9 Philip Jägenstedt 2010-05-24 10:43:25 UTC
Created attachment 161847 [details] [review]
combined patch

Previous 4 patched combined to reduce reverted cruft in history.
Comment 10 Sebastian Dröge (slomo) 2010-05-24 10:48:51 UTC
Thanks, I'll push this after next -good release. Feel free to base all new matroskademux patches on top of this one ;)
Comment 11 Sebastian Dröge (slomo) 2010-05-24 15:49:36 UTC
As said on IRC already, this currently deadlocks when trying to seek because the _reset_streams() function is called at once place with the object lock already taken. Most probably the correct solution would be, to remove the locking from _reset_streams() and always call it with the object lock from all places.
Comment 12 Philip Jägenstedt 2010-05-25 03:42:53 UTC
Created attachment 161906 [details] [review]
deadlock fix

This patch fixes the deadlock in the simplest possible way. I also considered adding a gst_matroska_demux_reset_streams_unlocked, but didn't.

I noticed that in GST_EVENT_FLUSH_STOP there are some other things (e.g. the segment) that are reset without holding the object lock. Is this really safe given that segment is also accessed from _query and _handle_seek_event?
Comment 13 Sebastian Dröge (slomo) 2010-05-25 09:22:38 UTC
(In reply to comment #12)

> I noticed that in GST_EVENT_FLUSH_STOP there are some other things (e.g. the
> segment) that are reset without holding the object lock. Is this really safe
> given that segment is also accessed from _query and _handle_seek_event?

No :) I guess we should open another bug for this...
Comment 14 Sebastian Dröge (slomo) 2010-05-30 13:14:31 UTC
The patch is not entirely correct, you must convert the running time from the QoS event to the position in the file. I've fixed this locally and will push after release. If you need the patch before that, please ask :)
Comment 15 Philip Jägenstedt 2010-05-31 04:04:09 UTC
Yes, please do attach the patch so I can see what's wrong (and cherry-pick it).
Comment 16 Sebastian Dröge (slomo) 2010-05-31 09:24:26 UTC
Created attachment 162362 [details] [review]
matroskademux: Don't compare running times with stream times when doing QoS
Comment 17 Philip Jägenstedt 2010-06-01 05:13:17 UTC
To be honest I don't understand what the difference between running time and stream time is, and the documentation for gst_segment_* isn't helping. Is it just luck that after my patch it seemed to be dropping frames at the right time?

In any case, the patch also looks wrong as it leaves running_time uninitialized, with a compiler warning and all.
Comment 18 Sebastian Dröge (slomo) 2010-06-01 09:04:45 UTC
http://lac.linuxaudio.org/2010/download/GStreamerAudioApps.pdf  slide 22 as a nice picture for the differences between the times and what the effect of mixing the two would be.

But you're right about running_time, sorry. New patch comes soon :)
Comment 19 Sebastian Dröge (slomo) 2010-06-01 09:06:44 UTC
Created attachment 162440 [details] [review]
matroskademux: Don't compare running times with stream times when doing QoS
Comment 20 Sebastian Dröge (slomo) 2010-06-01 09:23:05 UTC
commit 0d5ae784b1f63d4769dfa2cfee15ea685066daf0
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon May 31 07:49:21 2010 +0200

    matroskademux: Don't compare running times with stream times when doing QoS


commit 596331c6f0a9f598e9622c5bb4ec657c9d06b3d0
Author: Philip Jägenstedt <philipj@opera.com>
Date:   Tue May 25 05:36:46 2010 +0200

    matroskademux: fix deadlock introduced by video keyframe QoS

commit 80926a5596bbd3b35795ae135786c5ac29d46a5b
Author: Philip Jägenstedt <philipj@opera.com>
Date:   Sun May 23 09:32:08 2010 +0200

    matroskademux: skip buffers before a late keyframe (QoS)
    
    Before, vp8dec had no option but to decode all frames even if some/all
    of them would be late. With this change, performance when keyframes are
    frequent is helped a great deal. On my Thinkpad X60s, decoding a 20 s
    1080p sunflower encode with keyframes every 10 frames went from taking
    42 s with 5 frames shown to 21 s with 15 frames shown (still slow
    enough to count by hand). When keyframes are more sparse, you will
    still be able to catch up eventually, but the results won't be as
    noticable.