GNOME Bugzilla – Bug 797239
qtdemux: Crash when playing smooth-streaming
Last modified: 2018-10-20 11:40:16 UTC
Just stumbled across this crash. The stream list is empty, but n_streams says 1. gst-play-1.0 "http://playready.eastus2.cloudapp.azure.com/smoothstreaming/SSWSS720H264/SuperSpeedway_720.ism/Manifest" (gdb) bt
+ Trace 238693
This *might* have been introduced by: commit d72a7c038c6a1be3f865afa9b50f2878d02d87ba Author: Seungha Yang <seungha.yang@navercorp.com> Date: Tue May 8 20:26:41 2018 +0900 qtdemux: Try to expose whenever got new moov or new stream-start Whenever got new moov or new stream-start, demux will try to expose new pad by following rule. Comparing stream-id in the current moov with previous one, then * If matched stream-id is found from previous one, reuse existing pad (most common case) * Otherwise, expose new pad with new stream-start * No more used stream will be freed https://bugzilla.gnome.org/show_bug.cgi?id=684790
Do you know which version this was merged in ? The bug wasn't updated when it got marked resolved.
Answer is master, so 1.15.1 is target, it's updated now. I just tested on 1.14 and indeed it worked.
Created attachment 373841 [details] [review] qtdemux: Don't switch active streams and old streams ... ... before the old streams is not exposed yet for MSS stream. In case of DASH, newly configured streams will be exposed whenever demux got moov without delay. Meanwhile, since there is no moov box in MSS stream, the caps will act like moov. Then, there is delay for exposing new pads until demux got the first moof. So, following scenario is possible only for MSS but not for DASH, STREAM-START -> CAPS -> (configure stream but NOT EXPOSED YET) -> STREAM-START-> CAPS (configure stream again). In above scenario, we can reuse old stream without any stream reconfigure.
Review of attachment 373841 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +2458,3 @@ + demux->old_streams = + g_list_concat (demux->old_streams, demux->active_streams); + demux->active_streams = NULL; Shouldn't we also set n_streams to 0 here ? Otherwise it means active_streams and n_streams are out of sync
Is n_streams strictly the size of the list? If so, it might be good looking into using GQueue ?
(In reply to Nicolas Dufresne (ndufresne) from comment #6) > Is n_streams strictly the size of the list? If so, it might be good looking > into using GQueue ? because n_streams should be the size of the list, I feel the use of GQueue can make code a little simpler. I'll look at it more.
Created attachment 373853 [details] [review] qtdemux: Don't switch active streams and old streams ...
Or GPtrArray. Linked lists are usually the wrong data structure :)
Created attachment 373863 [details] [review] [1/4] qtdemux: Make function foreach method friendly
Created attachment 373864 [details] [review] [2/4] qtdemux: Make QtDemuxStream refcounted structure This a prework for porting GPtrArray. Refcounting will help the use of g_ptr_array_new_with_free_func() with QtDemuxStream structure
Created attachment 373865 [details] [review] [3/4] qtdemux: Use GPtrArray to store QtDemuxStream structure GPtrArray has less overhead than linked list and the length also can be auto updated by using it.
Created attachment 373866 [details] [review] [4/4] qtdemux: Don't switch active streams and old streams ...
Unit test (qtdemux/qtmux) and gst-validate-launch were passed with new patches.
perfect ! Thanks for that.
Seungha, unfortunately this uses glib API which isn't present in the required glib version we use: * g_ptr_array_find_with_equal_func : this was introduced in glib 2.54
(In reply to Edward Hervey from comment #16) > Seungha, unfortunately this uses glib API which isn't present in the > required glib version we use: > > * g_ptr_array_find_with_equal_func : this was introduced in glib 2.54 My bad :( I'll upload new patch soon
Created attachment 373975 [details] [review] qtdemux: Don't use g_ptr_array_find_with_equal_func() g_ptr_array_find_with_equal_func was introduced in glib 2.54 which is a higher version than our minimum required one.
Comment on attachment 373975 [details] [review] qtdemux: Don't use g_ptr_array_find_with_equal_func() >+/* g_ptr_array_find_with_equal_func is available since 2.54, >+ * replacement until we can depend unconditionally on the real one in GLib */ >+static gboolean >+qtdemux_ptr_array_find_with_equal_func (GPtrArray * haystack, >+ gconstpointer needle, GEqualFunc equal_func, guint * index_) >+{ >... >+} I think this should be done conditionally, like this: #if !GLIB_CHECK_VERSION(2,56,0) #define g_ptr_array_find_with_equal_func qtdemux_ptr_array_find_with_equal_func >+static gboolean >+g_ptr_array_find_with_equal_func (GPtrArray * haystack, >+ gconstpointer needle, GEqualFunc equal_func, guint * index_) >+{ >... >+} #endif which makes it easier to grep for it later when we bump the GLib req, and avoids the duplication if GLib is new enough.
Created attachment 373977 [details] [review] qtdemux: Fix broken build due to g_ptr_array_find_with_equal_func() Modify to use GLIB_CHECK_VERSION(2,54,0)
*** Bug 797309 has been marked as a duplicate of this bug. ***
Comment on attachment 373977 [details] [review] qtdemux: Fix broken build due to g_ptr_array_find_with_equal_func() Thanks! commit 7bce030be35532a1a2025bf5fea0d47dd822adb5 (HEAD -> master) Author: Seungha Yang <seungha.yang@navercorp.com> Date: Sat Oct 20 00:10:04 2018 +0900 qtdemux: Fix build with GLib versions < 2.54 g_ptr_array_find_with_equal_func was introduced in glib 2.54 which is a higher version than our minimum required one. https://bugzilla.gnome.org/show_bug.cgi?id=797239