[Bug 755169] dashdemux: can we have multiple seek events at the same time?

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=755169

--- Comment #28 from Florin Apostol <florin.apostol at oregan.net> ---
Review of attachment 311886:
 --> (https://bugzilla.gnome.org/review?bug=755169&attachment=311886)

1. for each function that must be called with the manifest lock taken, please
add a comment to say so. Until now we have gst_adaptive_demux_stream_free,
gst_adaptive_demux_stop_tasks, gst_adaptive_demux_expose_streams,
gst_adaptive_demux_advance_period, gst_adaptive_demux_has_next_period,
gst_adaptive_demux_combine_flows

2. GST_EVENT_RECONFIGURE in gst_adaptive_demux_src_event also needs to grab
demux->priv->api_lock. It uses the demux->streams that can be changed by seek
event.

3. gst_adaptive_demux_handle_message uses demux->streams. It needs to grab
demux->priv->api_lock and the manifest lock

4. gst_adaptive_demux_push_src_event iterates demux->streams. But the callers
of gst_adaptive_demux_push_src_event specifically unlock the manifest before
calling it. And because the first call is done from the
gst_adaptive_demux_src_event case GST_EVENT_SEEK before
gst_adaptive_demux_stop_tasks, the download_loop is running and allowed to
change the demux->streams, which could invalidate the iterator used in
gst_adaptive_demux_push_src_event. I believe the first call of
gst_adaptive_demux_push_src_event (when gst_event_new_flush_start is sent)
should be done with the manifest lock taken (I know this is dangerous and will
deadlock if the element receiving the flush start decides to call back into
adaptivedemux, but I cannot see any reason for it to do that). Alternatively,
the tasks should be stopped before sending the flush_start, but I can't
evaluate the consequences of that.

::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c
@@ +410,2 @@
       gst_adaptive_demux_reset (demux);
+      g_mutex_unlock (&demux->priv->api_lock);

api_lock should protect the whole gst_adaptive_demux_change_state function
(including the parent class call result = GST_ELEMENT_CLASS
(parent_class)->change_state (element, transition);)

If it does not, the parent class could use virtual functions to make changes in
demux object, so you did not properly serialized 2 change_state calls.

@@ +429,3 @@
   switch (event->type) {
     case GST_EVENT_FLUSH_STOP:
+      g_mutex_lock (&demux->priv->api_lock);

same as above, the whole function should be serialized
There might be other similar places with the same problem.

@@ +910,2 @@
   stream = g_malloc0 (demux->stream_struct_size);
+  g_print ("XX Alloc %p\n", stream);

change g_print to GST_DEBUG if you want to keep those messages.

@@ +1391,3 @@
     GstAdaptiveDemuxStream *stream = iter->data;

     gst_task_join (stream->download_task);

I don't think the copy is needed. download_loop was instructed to exit (by
demux->cancelled). As soon as download_loop will try to reacquire the manifest
lock (needed for any access to demux->streams), it will check for cancelled
flag and will exit. It will have no chance to change the demux->streams list.

And if by some chance it manages to change it in its way to exit, copying the
list introduces this problem:
here you join the task.
But later, when the stream is freed, the gst_mini_object_unref will call
gst_adaptive_demux_stream_free which will also join the task. So you cannot
allow the demux->streams list to change while the manifest lock is released
here. Which means the copy is unnecessary.

@@ +2184,1 @@
     /* Need to unlock as it might post messages to the bus */

here the manifest is unlocked while the streams are unreffed. But
gst_adaptive_demux_reset has the same code and there the manifest is not
unlocked. Also in all other places where gst_mini_object_unref is used the
manifest is not unlocked.
Personally I don't see why we should unlock. So what if it post messages on the
bus?
Can you explain the reason for the copy?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.