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 749535 - downloadbuffer: deadlock issue in playbin
downloadbuffer: deadlock issue in playbin
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal normal
: 1.4.6
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-18 08:36 UTC by Eunhae Choi
Modified: 2015-06-03 11:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
downloadbuffer: send msg after release lock (4.68 KB, patch)
2015-05-18 08:43 UTC, Eunhae Choi
none Details | Review
downloadbuffer: release lock before posting msg (7.61 KB, patch)
2015-05-20 12:26 UTC, Eunhae Choi
none Details | Review
downloadbuffer: release lock before posting msg (2) (7.48 KB, patch)
2015-05-29 04:01 UTC, Eunhae Choi
none Details | Review
downloadbuffer: release lock before posting msg (3) (7.61 KB, patch)
2015-06-01 02:24 UTC, Eunhae Choi
committed Details | Review

Description Eunhae Choi 2015-05-18 08:36:11 UTC
gst-launch-1.0 playbin uri="http://clips.vorwaerts-gmbh.de/big_buck_bunny.mp4" flag=0x80

When I check the playin function with download flags, deadlock is occurred during buffering. 

I've pasted the deadlock callstack info in this page(http://ur1.ca/mgjbw).
Comment 1 Eunhae Choi 2015-05-18 08:43:45 UTC
Created attachment 303508 [details] [review]
downloadbuffer: send msg after release lock

buffering msg should be posted after releasing the download lock to avoid the reported deadlock.
Comment 2 Sebastian Dröge (slomo) 2015-05-18 08:47:31 UTC
Review of attachment 303508 [details] [review]:

I'm not sure this mutex releasing and taking again is correct everywhere. Maybe instead of that it would be cleaner to return the message from update_buffering() and have it be posted by the caller once the mutex is released for real.

::: plugins/elements/gstdownloadbuffer.c
@@ +1267,2 @@
             gst_structure_new ("GstCacheDownloadComplete",
                 "location", G_TYPE_STRING, dlbuf->temp_location, NULL)));

Here you're just posting another message while the mutex is locked :)
Comment 3 Eunhae Choi 2015-05-18 11:29:41 UTC
Thank you for your quick review.

You means, 1. update_buffering() is returened, 2. releasing the lock, 3. posting msg in the caller. am I right?

I think, to posting the msg I should reference the dlbuf so I need lock again.
Actually the update_buffering() is for posting msg. If I have misunderstanding, please let me know. 

And I saw the "download complete" msg posting code too but I did not know whether it can cause deadlock or not because this issue is apparently occured by buffering msg. So I just keep the code as before.

gst_structure_new ("GstCacheDownloadComplete",
                 "location", G_TYPE_STRING, dlbuf->temp_location, NULL)));
Comment 4 Sebastian Dröge (slomo) 2015-05-18 13:22:38 UTC
No, I meant:

msg = update_buffering();
[...]
unlock();
gst_element_post_message(msg);

[and not taking the lock again]
Comment 5 Eunhae Choi 2015-05-20 12:26:23 UTC
Created attachment 303659 [details] [review]
downloadbuffer: release lock before posting msg

I attached new patch according to your guide. thank you.
Comment 6 Thiago Sousa Santos 2015-05-28 08:49:42 UTC
Review of attachment 303659 [details] [review]:

Still some minor changes to fix.

Also, please limit lines in your commit messages to 80 chars for easier reading in logs.

Thanks!

::: plugins/elements/gstdownloadbuffer.c
@@ -411,3 @@
-{
-  dlbuf->cur_level.bytes = bytes;
-  update_time_level (dlbuf);

update_time_level now also updates the byte level because of this change. Please keep the 2 functions or rename the new update_time_level to update_levels.

@@ +1068,3 @@
+
+            GST_DOWNLOAD_BUFFER_MUTEX_LOCK_CHECK (dlbuf, dlbuf->sinkresult,
+                out_flushing);

Here you are still taking the lock again after posting the message. Ideally you only post the message further below in the code when the lock is released and doesn't need to be retaken.
Comment 7 Eunhae Choi 2015-05-28 14:21:44 UTC
I will make short commit message:) 

in the patch, I've renamed the update_time_level to update_levels. 
as you know, there was two functions - update_time_level, update_levels.
I merged it to one function - update_levels. This updates byte and time.

and to add signal I got lock again like before. do I need to remove the lock? 
Thank you.

@@ +1068,3 @@
+
+            GST_DOWNLOAD_BUFFER_MUTEX_LOCK_CHECK (dlbuf, dlbuf->sinkresult,
+                out_flushing);

            /* wakeup the waiter and let it recheck */
            GST_DOWNLOAD_BUFFER_SIGNAL_ADD (dlbuf, -1);
            break;
Comment 8 Thiago Sousa Santos 2015-05-28 14:32:08 UTC
(In reply to Eunhae Choi from comment #7)
> I will make short commit message:) 

No need to make it short, just break it into multiple lines.

> 
> in the patch, I've renamed the update_time_level to update_levels. 
> as you know, there was two functions - update_time_level, update_levels.
> I merged it to one function - update_levels. This updates byte and time.

Ah, true. I didn't notice that you had changed the name already.

> 
> and to add signal I got lock again like before. do I need to remove the
> lock? 
> Thank you.

Not exactly. At this part you are only unlocking to post the message and then you lock again. Instead you should only post the message after the unlock at the end of the block (at the end of the 'GST_EVENT_IS_SERIALIZED' if). This way you don't need to unlock and then lock again.

> 
> @@ +1068,3 @@
> +
> +            GST_DOWNLOAD_BUFFER_MUTEX_LOCK_CHECK (dlbuf, dlbuf->sinkresult,
> +                out_flushing);
> 
>             /* wakeup the waiter and let it recheck */
>             GST_DOWNLOAD_BUFFER_SIGNAL_ADD (dlbuf, -1);
>             break;
Comment 9 Eunhae Choi 2015-05-29 04:01:34 UTC
Created attachment 304210 [details] [review]
downloadbuffer: release lock before posting msg (2)

I attached new patch with consideration of your comment. Thank you.
Comment 10 Thiago Sousa Santos 2015-05-29 17:39:02 UTC
Review of attachment 304210 [details] [review]:

::: plugins/elements/gstdownloadbuffer.c
@@ +1064,3 @@
             /* wakeup the waiter and let it recheck */
             GST_DOWNLOAD_BUFFER_SIGNAL_ADD (dlbuf, -1);
+            GST_DOWNLOAD_BUFFER_MUTEX_UNLOCK (dlbuf);

This is not right as the same mutex will be unlocked again a few lines below around line 1082. You need to post the message after that unlock.
Comment 11 Eunhae Choi 2015-06-01 02:24:48 UTC
Created attachment 304335 [details] [review]
downloadbuffer: release lock before posting msg (3)

I fixed the code as your comment. Thank you.
Comment 12 Thiago Sousa Santos 2015-06-01 18:09:26 UTC
Thanks for the patch, merged.

commit e921ee132bec0b4dc38ce5ba7bb76e6a951bae84
Author: eunhae choi <eunhae1.choi@samsung.com>
Date:   Wed May 20 21:18:08 2015 +0900

    downloadbuffer: release lock before posting msg
    
    to avoid the deadlock in playbin2,
    send msg after release the download buffer lock.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749535
Comment 13 Luis de Bethencourt 2015-06-02 09:29:55 UTC
Well done Eunhae! Congrats :)
Comment 14 Tim-Philipp Müller 2015-06-03 11:09:39 UTC
This has been picked into the 1.4 branch.