[vlc-devel] [PATCH 3/4] sout: lock sout_stream_t individually

[Steve Lhomme]

On 2020-10-12 9:36, Rémi Denis-Courmont wrote:
> Hi,
> 
> It's needed in two cases, the duplicate module and the core (what the 
> next patch removes). We can add it in duplicate, with a lot of 
> boilerplate. But given that the ES output has a similar internal locking 
> for essentially the same motivation (concurrent ES processed in 
> different threads), it seems logical to move it from sout instance to 
> sout stream instead and share the code.
> 
> In the common non-contended case, vlc_mutex_lock is just one atomic op 
> extra.

Personally I prefer not to lock when I don't have to. It's one less 
possible dead lock to worry about. But I have no objection if there's 
always a lock involved anyway.

> Le 12 octobre 2020 09:57:20 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a 
> écrit :
> 
>     This lock looks like in belongs in the duplicate module rather that do a
>     lock for all sout_stream_t users.
> 
>     On 2020-10-11 16:30, Rémi Denis-Courmont wrote:
> 
>         The stream output is called from multiple threads without
>         synchronisation, notably at least one thread per ES decoder.
>         Normally,
>         the stream output instance (sout_instance_t) lock will serialise all
>         calls to the first stream output module instance (sout_stream_t)
>         in the
>         stream output chain.
> 
>         This is not practical if there is also concurrency within the chain.
>         One notable case is when the select/duplicate module is used in
>         filter
>         mode and one of the select/duplicate target is threaded, e.g.:
> 
>         #duplicate{dst=transcode{...},dst=transcode{...}}:std{...}
> 
>         In that example, the standard output can be called from both
>         transcode
>         filters.
> 
>         To solve this, add a lock for each element in the stream output
>         chain.
>         Alternatively, we could drop filter mode from select/duplicate,
>         leaving
>         only the (more common) output mode where duplicate is the last
>         element
>         in the chain.
>         ------------------------------------------------------------------------
>         src/stream_output/stream_output.c | 44
>         +++++++++++++++++++++++++++----
>         1 file changed, 39 insertions(+), 5 deletions(-)
> 
>         diff --git a/src/stream_output/stream_output.c
>         b/src/stream_output/stream_output.c
>         index e0769dc749..069fe57346 100644
>         --- a/src/stream_output/stream_output.c
>         +++ b/src/stream_output/stream_output.c
>         @@ -736,38 +736,71 @@ static void mrl_Clean( mrl_t *p_mrl )
> 
>         struct sout_stream_private {
>         sout_stream_t stream;
>         + vlc_mutex_t lock;
>         module_t *module;
>         };
> 
>         #define sout_stream_priv(s) \
>         container_of(s, struct sout_stream_private, stream)
> 
>         +static void sout_StreamLock(sout_stream_t *s)
>         +{
>         + vlc_mutex_lock(&sout_stream_priv(s)->lock);
>         +}
>         +
>         +static void sout_StreamUnlock(sout_stream_t *s)
>         +{
>         + vlc_mutex_unlock(&sout_stream_priv(s)->lock);
>         +}
>         +
>         void *sout_StreamIdAdd(sout_stream_t *s, const es_format_t *fmt)
>         {
>         - return s->ops->add(s, fmt);
>         + void *id;
>         +
>         + sout_StreamLock(s);
>         + id = s->ops->add(s, fmt);
>         + sout_StreamUnlock(s);
>         + return id;
>         }
> 
>         void sout_StreamIdDel(sout_stream_t *s, void *id)
>         {
>         + sout_StreamLock(s);
>         s->ops->del(s, id);
>         + sout_StreamUnlock(s);
>         }
> 
>         int sout_StreamIdSend(sout_stream_t *s, void *id, block_t *b)
>         {
>         - return s->ops->send(s, id, b);
>         + int val;
>         +
>         + sout_StreamLock(s);
>         + val = s->ops->send(s, id, b);
>         + sout_StreamUnlock(s);
>         + return val;
>         }
> 
>         void sout_StreamFlush(sout_stream_t *s, void *id)
>         {
>         if (s->ops->flush != NULL)
>         + {
>         + sout_StreamLock(s);
>         s->ops->flush(s, id);
>         + sout_StreamUnlock(s);
>         + }
>         }
> 
>         int sout_StreamControlVa(sout_stream_t *s, int i_query, va_list
>         args)
>         {
>         - if (s->ops->control == NULL)
>         - return VLC_EGENERIC;
>         - return s->ops->control(s, i_query, args);
>         + int val = VLC_EGENERIC;
>         +
>         + if (s->ops->control != NULL)
>         + {
>         + sout_StreamLock(s);
>         + val = s->ops->control(s, i_query, args);
>         + sout_StreamUnlock(s);
>         + }
>         + return val;
>         }
> 
>         /* Destroy a "stream_out" module */
>         @@ -823,6 +856,7 @@ static sout_stream_t *sout_StreamNew(
>         vlc_object_t *parent, char *psz_name,
>         if (unlikely(priv == NULL))
>         return NULL;
> 
>         + vlc_mutex_init(&priv->lock);
>         p_stream = &priv->stream;
>         p_stream->psz_name = psz_name;
>         p_stream->p_cfg = p_cfg;
>         -- 
>         2.28.0
>         ------------------------------------------------------------------------
>         vlc-devel mailing list
>         To unsubscribe or modify your subscription options:
>         https://mailman.videolan.org/listinfo/vlc-devel
> 
>     ------------------------------------------------------------------------
>     vlc-devel mailing list
>     To unsubscribe or modify your subscription options:
>     https://mailman.videolan.org/listinfo/vlc-devel
> 
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser 
> ma brièveté.
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>