[Bug 756000] gstrtmpsink: add unlock

[GStreamer (GNOME Bugzilla)]

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

--- Comment #7 from Håvard Graff (hgr) <havard.graff at gmail.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #6)
> Review of attachment 324976 [details] [review]:
> 
> ::: ext/rtmp/gstrtmpsink.c
> @@ +227,3 @@
> +        the socket comes up */
> +    while (sink->connecting && sink->rtmp->m_sb.sb_socket == -1) {
> +      g_thread_yield ();
> 
> Could be slightly cleaner and more efficient using a GCond here.

How do you envision that? We are waiting for an internal socket inside librtmp
to become initialized, and there is no signaling from librtmp to say this has
happened. If you have a clever trick I am very interested, as I don't like this
either, but it was the only thing I could think of to make the tests pass.

> 
> ::: ext/rtmp/gstrtmpsink.h
> @@ +53,3 @@
>    RTMP *rtmp;
>    gchar *rtmp_uri; /* copy of url for librtmp */
> +  GMutex rtmp_lock;
> 
> I believe with some effort, using the stream lock should be possible. This
> way, you would have unlock() that always do shutdown, and unlock_stop()
> (which have the state lock held) that always do RTMP_Close(). Any other
> blocking operation should be done where the stream lock is held already, if
> not, that's a bug.

I have never been a fan of touching the stream-lock in implementations, as in
my experience this is a certain path to deadlocks, and I have always considered
it an internal lock. I'd much rather have an additional "small" well-defined,
easily scoped lock then piggy-backing on one that might have unforeseen and
complex interactions.

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