[Bug 756000] gstrtmpsink: add unlock

[GStreamer (GNOME Bugzilla)]

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

--- Comment #11 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
(In reply to Håvard Graff (hgr) from comment #7)
> (In reply to Nicolas Dufresne (stormer) from comment #6)
> > Review of attachment 324976 [details] [review] [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.

True, you could signal sink->connecting state, but probably not
rtmp->mb_sb.sb_socket.

> 
> > 
> > ::: 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 don't take the streaming lock, that's not the element job. But all
operation that may need to be cancelled, should happen under this lock only.
When you start adding extra lock, and doing blocking operation from
non-streaming threads, that's where you start having issues. If all the
operation on the RTMP library where done within the stream thread, they could
be protected by the stream lock, and you would not need to add new lock. The
only remaining call, that will happen from external thread is start()/stop(),
but the streaming thread should not be running at this point. And then you also
have to deal with _unlock(), which minially unblock the operations, while
unlock_stop() shall do the cleanup as it's called with the stream lock.

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