[vlc-devel] [PATCH 02/12] chromecast: use a local sout_stream_id_sys_t containing the es_format_t

[Steve Lhomme]

On Mon, Apr 25, 2016 at 6:00 PM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> Le 2016-04-25 17:46, Steve Lhomme a écrit :
>>
>> ---
>>  modules/stream_out/chromecast/cast.cpp | 65
>> ++++++++++++++++++++++++++++++++--
>>  1 file changed, 63 insertions(+), 2 deletions(-)
>>
>> diff --git a/modules/stream_out/chromecast/cast.cpp
>> b/modules/stream_out/chromecast/cast.cpp
>> index d05ca1c..ef7f428 100644
>> --- a/modules/stream_out/chromecast/cast.cpp
>> +++ b/modules/stream_out/chromecast/cast.cpp
>> @@ -44,16 +44,23 @@ struct sout_stream_sys_t
>>          , b_has_video(has_video)
>>      {
>>          assert(p_intf != NULL);
>> +        vlc_mutex_init( &es_lock );
>>      }
>>
>>      ~sout_stream_sys_t()
>>      {
>>          sout_StreamChainDelete(p_out, p_out);
>> +        vlc_mutex_destroy( &es_lock );
>>      }
>>
>>      sout_stream_t * const p_out;
>>      intf_sys_t * const p_intf;
>>      const bool b_has_video;
>> +
>> +    sout_stream_id_sys_t *GetSubId( sout_stream_t*, sout_stream_id_sys_t*
>> );
>> +
>> +    vlc_mutex_t                        es_lock;
>> +    std::vector<sout_stream_id_sys_t*> streams;
>
>
> As a general rule, VLC module instances (i.e. VLC objects, basically) do not
> need to be reentrant - all callbacks must be serialized by the owner. So
> that mutex does not seem to make much sense.

The stream Add()/Del() callbacks are called by the decoders thread.
There's more than one at the same time. So at the very list the vector
needs to protect from concurrent editing.

The Send() callback is also called by the decoder threads. And before
we setup the Chromecast and the muxer/transcoder+HTTP server, we need
to wait 'long enough' to have all those threads setting the tracks
they need. That also requires concurrent protection.

I don't know if Send is guaranteed not to be called during an Add/Del phase.

> (There are exceptions, of course, such as object variable callbacks.)
>
>
>>  };
>>
>>  #define SOUT_CFG_PREFIX "sout-chromecast-"
>> @@ -100,6 +107,12 @@ vlc_module_begin ()
>>  vlc_module_end ()
>>
>>
>> +struct sout_stream_id_sys_t
>> +{
>> +    es_format_t           fmt;
>> +    sout_stream_id_sys_t  *p_sub_id;
>> +};
>> +
>>
>>
>>
>> /*****************************************************************************
>>   * Sout callbacks
>>
>>
>>
>> *****************************************************************************/
>> @@ -112,7 +125,17 @@ static sout_stream_id_sys_t *Add(sout_stream_t
>> *p_stream, const es_format_t *p_f
>>          if (p_fmt->i_cat != AUDIO_ES)
>>              return NULL;
>>      }
>> -    return sout_StreamIdAdd(p_sys->p_out, p_fmt);
>> +
>> +    sout_stream_id_sys_t *p_sys_id = (sout_stream_id_sys_t *)malloc(
>> sizeof(sout_stream_id_sys_t) );
>> +    if (p_sys_id != NULL)
>> +    {
>> +        es_format_Copy( &p_sys_id->fmt, p_fmt );
>> +        p_sys_id->p_sub_id = NULL;
>> +
>> +        vlc_mutex_locker locker( &p_sys->es_lock );
>> +        p_sys->streams.push_back( p_sys_id );
>> +    }
>> +    return p_sys_id;
>>  }
>>
>>
>> @@ -120,15 +143,49 @@ static void Del(sout_stream_t *p_stream,
>> sout_stream_id_sys_t *id)
>>  {
>>      sout_stream_sys_t *p_sys = p_stream->p_sys;
>>
>> -    sout_StreamIdDel(p_sys->p_out, id);
>> +    vlc_mutex_locker locker( &p_sys->es_lock );
>> +    for (size_t i=0; i<p_sys->streams.size(); i++)
>> +    {
>> +        if ( p_sys->streams[i] == id )
>> +        {
>> +            if ( p_sys->streams[i]->p_sub_id != NULL )
>> +                sout_StreamIdDel( p_sys->p_out,
>> p_sys->streams[i]->p_sub_id );
>> +
>> +            es_format_Clean( &p_sys->streams[i]->fmt );
>> +            free( p_sys->streams[i] );
>> +            p_sys->streams.erase( p_sys->streams.begin() +  i );
>> +            break;
>> +        }
>> +    }
>>  }
>>
>> +sout_stream_id_sys_t *sout_stream_sys_t::GetSubId( sout_stream_t
>> *p_stream,
>> +
>> sout_stream_id_sys_t *id )
>> +{
>> +    size_t i;
>> +
>> +    assert( p_stream->p_sys == this );
>> +
>> +    vlc_mutex_locker locker( &es_lock );
>> +    for (i = 0; i < streams.size(); ++i)
>> +    {
>> +        if ( id == (sout_stream_id_sys_t*) streams[i] )
>> +            return streams[i]->p_sub_id;
>> +    }
>> +
>> +    msg_Err( p_stream, "unknown stream ID" );
>> +    return NULL;
>> +}
>
>
> So, either this method is intrinsically serialized with the plugin
> callbacks, and the lock is not necessary. Or it is not, and this won't work
> anyway because <id> would be potentially invalid.

Indeed, the lock should be moved up so that Send()/Flush() are
guaranteed the id is not freed while processing.

>>  static int Send(sout_stream_t *p_stream, sout_stream_id_sys_t *id,
>>                  block_t *p_buffer)
>>  {
>>      sout_stream_sys_t *p_sys = p_stream->p_sys;
>>
>> +    id = p_sys->GetSubId( p_stream, id );
>> +    if ( id == NULL )
>> +        return VLC_EGENERIC;
>> +
>>      return sout_StreamIdSend(p_sys->p_out, id, p_buffer);
>>  }
>>
>> @@ -136,6 +193,10 @@ static void Flush( sout_stream_t *p_stream,
>> sout_stream_id_sys_t *id )
>>  {
>>      sout_stream_sys_t *p_sys = p_stream->p_sys;
>>
>> +    id = p_sys->GetSubId( p_stream, id );
>> +    if ( id == NULL )
>> +        return;
>> +
>>      sout_StreamFlush( p_sys->p_out, id );
>>  }
>
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel