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 735663 - dashdemux: synchronize with the download loop thread before signalling it
dashdemux: synchronize with the download loop thread before signalling it
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-08-29 11:06 UTC by George Kiagiadakis
Modified: 2014-09-19 06:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] dashdemux: synchronize with the download loop thread to signal it to continue (1.43 KB, patch)
2014-08-29 11:06 UTC, George Kiagiadakis
needs-work Details | Review
[PATCH V2] dashdemux: synchronize with the download loop thread to signal it to continue (2.81 KB, patch)
2014-09-05 10:30 UTC, Matthieu Bouron
none Details | Review
[PATCH]dashdemux: properly wait fragment downloads (5.75 KB, patch)
2014-09-05 13:38 UTC, Matthieu Bouron
reviewed Details | Review
[PATCH]dashdemux: properly wait fragment downloads (6.02 KB, patch)
2014-09-09 11:09 UTC, Matthieu Bouron
reviewed Details | Review
dashdemux: synchronize with the download loop thread to signal it to continue (6.35 KB, patch)
2014-09-12 05:46 UTC, Thiago Sousa Santos
reviewed Details | Review
hlsdemux: synchronize with the download loop thread to signal it to continue (4.92 KB, patch)
2014-09-12 05:47 UTC, Thiago Sousa Santos
reviewed Details | Review
mssdemux: synchronize with the download loop thread to signal it to continue (5.43 KB, patch)
2014-09-12 05:47 UTC, Thiago Sousa Santos
reviewed Details | Review
dashdemux: synchronize with the download loop thread to signal it to continue (15.44 KB, patch)
2014-09-13 16:34 UTC, Thiago Sousa Santos
committed Details | Review

Description George Kiagiadakis 2014-08-29 11:06:56 UTC
Created attachment 284805 [details] [review]
[PATCH] dashdemux: synchronize with the download loop thread to  signal it to continue

There is an issue when dashdemux is working with local files that sometimes the downloader src element (filesrc) manages to download the whole file before the "download loop" thread (which has set filesrc to PLAYING) manages to reach its g_cond_wait() call where it waits for filesrc to send EOS (or ERROR). In that case, the g_cond_signal() doesn't have any effect and afterwards the "download loop" thread enters g_cond_wait() and waits forever.

This patch solves the issue by locking the mutex associated with the GCond before g_cond_signal(), which ensures that g_cond_wait() has been reached.
Comment 1 Sebastian Dröge (slomo) 2014-08-29 11:11:43 UTC
The same has to be fixed in hlsdemux and mssdemux too then, they have the same code.
Comment 2 Matthieu Bouron 2014-09-05 10:30:57 UTC
Created attachment 285470 [details] [review]
[PATCH V2] dashdemux: synchronize with the download loop thread to signal it to continue

Patch updated with the following changes:

When a stream src error message is handled, the download fragment cond is signaled asynchrounously since we cannot hold the related lock from this function as it can be called when the element changes its state and it can result in a deadlock when gst_dash_demux_download_uri sync the stream src element state with its parent.

Anyway i'm not sure if it's right to handle that asynchronously.
Comment 3 Matthieu Bouron 2014-09-05 13:38:12 UTC
Created attachment 285496 [details] [review]
[PATCH]dashdemux: properly wait fragment downloads

Patch updated with the following changes:

We do not rely entirely anymore on having the fragmemt download condition being signaled because it could happen before g_cond_wait is called.
We cannot use the fragment download lock in the handle_message as it can be called when gst_dash_demux_download_uri set the state of the src element.
I dropped the previous version of the patch since we do not have the guarantee that the source function will be executed (using g_idle_add).

The code is a bit raw but does the job. Maybe waiting for the condition to be signaled can be dropped as we can only rely on the new variable fragment_download_signaled with its related lock.

Comments welcome :)
Comment 4 Thiago Sousa Santos 2014-09-09 05:34:16 UTC
Review of attachment 285496 [details] [review]:

Very good catch!

I suggested some minor corrections below. See if you agree.

::: ext/dash/gstdashdemux.c
@@ +432,3 @@
 
+          g_mutex_lock (&stream->fragment_download_signaled_lock);
+          stream->fragment_download_signaled = TRUE;

I think the signaling has to be done after this variable is set, inside the same lock.

@@ +1089,3 @@
     gst_element_set_state (stream->src, GST_STATE_READY);
     g_cond_signal (&stream->fragment_download_cond);
+    g_mutex_lock (&stream->fragment_download_signaled_lock);

Same as above.

@@ +2014,3 @@
     g_cond_signal (&stream->fragment_download_cond);
+    g_mutex_lock (&stream->fragment_download_signaled_lock);
+    stream->fragment_download_signaled = TRUE;

Same here.

@@ +2034,3 @@
+
+      g_mutex_lock (&stream->fragment_download_signaled_lock);
+      stream->fragment_download_signaled = TRUE;

Same.

@@ +2217,3 @@
           "Waiting for fragment download to finish: %s", uri);
+
+      end_time = g_get_monotonic_time () + 5 * G_TIME_SPAN_MILLISECOND;

If at all, this should be configurable via a property.

I'd prefer to have this as a separate patch. Let's focus here on just adding the boolean flag and making sure it properly works.

::: ext/dash/gstdashdemux.h
@@ +87,3 @@
   GMutex fragment_download_lock;
   GCond fragment_download_cond;
+  GMutex fragment_download_signaled_lock;

Any reason to add a new lock? Can't use the fragment_download_lock for the signaling and the variable? I think it makes sense to use the same.
Comment 5 Matthieu Bouron 2014-09-09 11:08:46 UTC
(In reply to comment #4)
> Review of attachment 285496 [details] [review]:
> 
> Very good catch!
> 
> I suggested some minor corrections below. See if you agree.
> 
> ::: ext/dash/gstdashdemux.c
> @@ +432,3 @@
> 
> +          g_mutex_lock (&stream->fragment_download_signaled_lock);
> +          stream->fragment_download_signaled = TRUE;
> 
> I think the signaling has to be done after this variable is set, inside the
> same lock.

Updated in the new version of the patch.

> 
> @@ +1089,3 @@
>      gst_element_set_state (stream->src, GST_STATE_READY);
>      g_cond_signal (&stream->fragment_download_cond);
> +    g_mutex_lock (&stream->fragment_download_signaled_lock);
> 
> Same as above.
> 
> @@ +2014,3 @@
>      g_cond_signal (&stream->fragment_download_cond);
> +    g_mutex_lock (&stream->fragment_download_signaled_lock);
> +    stream->fragment_download_signaled = TRUE;
> 
> Same here.
> 
> @@ +2034,3 @@
> +
> +      g_mutex_lock (&stream->fragment_download_signaled_lock);
> +      stream->fragment_download_signaled = TRUE;
> 
> Same.
> 
> @@ +2217,3 @@
>            "Waiting for fragment download to finish: %s", uri);
> +
> +      end_time = g_get_monotonic_time () + 5 * G_TIME_SPAN_MILLISECOND;
> 
> If at all, this should be configurable via a property.
> 
> I'd prefer to have this as a separate patch. Let's focus here on just adding
> the boolean flag and making sure it properly works.
> 
> ::: ext/dash/gstdashdemux.h
> @@ +87,3 @@
>    GMutex fragment_download_lock;
>    GCond fragment_download_cond;
> +  GMutex fragment_download_signaled_lock;
> 
> Any reason to add a new lock? Can't use the fragment_download_lock for the
> signaling and the variable? I think it makes sense to use the same.

Thanks for the review.

We need to introduce a new lock as gst_dash_demux_download_uri is called when fragment_download_lock is already held. gst_dash_demux_download_uri calls gst_element_sync_state_with_parent/gst_element_set_state which is likely to call gst_dash_demux_handle_message. If the same lock is used > deadlock.
Comment 6 Matthieu Bouron 2014-09-09 11:09:33 UTC
Created attachment 285728 [details] [review]
[PATCH]dashdemux: properly wait fragment downloads
Comment 7 Thiago Sousa Santos 2014-09-12 05:46:53 UTC
Created attachment 285983 [details] [review]
dashdemux: synchronize with the download loop thread to signal it to continue

Can you check if this version also fixes the issue for you? It reduces the locking area and doesn't require an additional mutex
Comment 8 Thiago Sousa Santos 2014-09-12 05:47:12 UTC
Created attachment 285984 [details] [review]
hlsdemux: synchronize with the download loop thread to signal it to continue

Same for hlsdemux
Comment 9 Thiago Sousa Santos 2014-09-12 05:47:26 UTC
Created attachment 285985 [details] [review]
mssdemux: synchronize with the download loop thread to signal it to continue

And mssdemux
Comment 10 Sebastian Dröge (slomo) 2014-09-12 14:22:29 UTC
Review of attachment 285728 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ +430,1 @@
           stream->last_ret = GST_FLOW_CUSTOM_ERROR;

Shouldn't this also be protected by the mutex?

@@ +431,3 @@
+
+          g_mutex_lock (&stream->fragment_download_signaled_lock);
+          stream->fragment_download_signaled = TRUE;

Maybe "download_finished" instead? Also why not just store the flow return here to distinguish between erorrs and other things?
Comment 11 Sebastian Dröge (slomo) 2014-09-12 14:26:24 UTC
Comment on attachment 285985 [details] [review]
mssdemux: synchronize with the download loop thread to signal it to continue

Same comment about the name of the variable, otherwise seems ok
Comment 12 Thiago Sousa Santos 2014-09-13 16:34:36 UTC
Created attachment 286138 [details] [review]
dashdemux: synchronize with the download loop thread to signal it to continue

Updated according to comments. Please test and let me know if it works for your case so I can merge and update the patches for hls and mss
Comment 13 Matthieu Bouron 2014-09-18 11:29:37 UTC
Review of attachment 286138 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ -2005,3 @@
   }
 
-  stream->last_ret = ret;

Shouldn't this be kept with some locking, to assign stream->last_ret to GST_FLOW_NOT_LINKED when this is the case ?
Comment 14 Matthieu Bouron 2014-09-18 13:50:33 UTC
(In reply to comment #13)
> Review of attachment 286138 [details] [review]:
> 
> ::: ext/dash/gstdashdemux.c
> @@ -2005,3 @@
>    }
> 
> -  stream->last_ret = ret;
> 
> Shouldn't this be kept with some locking, to assign stream->last_ret to
> GST_FLOW_NOT_LINKED when this is the case ?

I've test the patch and it seems to work fine with the previous change i've mentionned. If that can help you here is my branch that includes all the remaining dash fixes rebased on top of master:

http://cgit.collabora.com/git/user/mateo/gst-plugins-bad.git/log/?h=dash-fixes
Comment 15 Thiago Sousa Santos 2014-09-18 17:22:32 UTC
We don't want to set that everytime to avoid taking the lock on the chain function. So we only set it if it is != _OK, otherwise it will continue being _OK till the end.

commit e289ab07c13c9f958f04e396285b567b570120d1
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Fri Sep 12 02:36:47 2014 -0300

    mssdemux: synchronize with the download loop thread to signal it to continue
    
    If EOS or ERROR happens before the download loop thread has reached its
    g_cond_wait() call, then the g_cond_signal doesn't have any effect and
    the download loop thread stucks later.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=735663

commit f4546b64ea6fd89eb3471258a880b850555d98c5
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Fri Sep 12 02:35:44 2014 -0300

    hlsdemux: synchronize with the download loop thread to signal it to continue
    
    If EOS or ERROR happens before the download loop thread has reached its
    g_cond_wait() call, then the g_cond_signal doesn't have any effect and
    the download loop thread stucks later.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=735663

commit 55032ae5fee1d6253632f4ce7f2912b3d36e3a66
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Fri Aug 29 12:38:12 2014 +0200

    dashdemux: synchronize with the download loop thread to signal it to continue
    
    If EOS or ERROR happens before the download loop thread has reached its
    g_cond_wait() call, then the g_cond_signal doesn't have any effect and
    the download loop thread stucks later.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=735663
Comment 16 Sebastian Dröge (slomo) 2014-09-19 06:46:23 UTC
This sounds like it should go into 1.4 after the 1.4.2 release. Ok?