GNOME Bugzilla – Bug 619485
matroskademux: skip buffers before a late keyframe (QoS)
Last modified: 2010-06-01 09:23:35 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?
Created attachment 161830 [details] [review] patch 1/3
Created attachment 161831 [details] [review] patch 2/3
Created attachment 161832 [details] [review] patch 3/3
(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.
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.
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.
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.
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 :)
Created attachment 161847 [details] [review] combined patch Previous 4 patched combined to reduce reverted cruft in history.
Thanks, I'll push this after next -good release. Feel free to base all new matroskademux patches on top of this one ;)
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.
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?
(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...
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 :)
Yes, please do attach the patch so I can see what's wrong (and cherry-pick it).
Created attachment 162362 [details] [review] matroskademux: Don't compare running times with stream times when doing QoS
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.
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 :)
Created attachment 162440 [details] [review] matroskademux: Don't compare running times with stream times when doing QoS
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.