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.

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 :)

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)));

No, I meant:

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

[and not taking the lock again]

Created attachment 303659 [details] [review]
downloadbuffer: release lock before posting msg

I attached new patch according to your guide. thank you.

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.

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;

(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;

Created attachment 304210 [details] [review]
downloadbuffer: release lock before posting msg (2)

I attached new patch with consideration of your comment. Thank you.

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.

Created attachment 304335 [details] [review]
downloadbuffer: release lock before posting msg (3)

I fixed the code as your comment. Thank you.

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

Well done Eunhae! Congrats :)

This has been picked into the 1.4 branch.