[vlc-devel] [PATCH 02/12] chromecast: use a local sout_stream_id_sys_t containing the es_format_t
Steve Lhomme
robux4 at gmail.com
Tue Apr 26 09:46:41 CEST 2016
On Tue, Apr 26, 2016 at 9:29 AM, Steve Lhomme <robux4 at gmail.com> wrote:
> 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 Del() callback is not called from the decoder threads but from the
input thread. One more reason to protect the vector and also its
values. We do the sout_StreamIdDel() on the sub-sout right away so the
sout_stream_id_sys_t* from the sub-streams may be freed while we call
sout_StreamIdSend(). Although that should be the case with normal
sout. And looking at the upper code there is a lock in the stream
output object that guarantees none of these callbacks are called
concurrently. So I can remove my lock.
> 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
More information about the vlc-devel
mailing list