[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 10:59:57 CEST 2016
This patches is replaced by https://patches.videolan.org/patch/13092/
There's no more lock.
On Tue, Apr 26, 2016 at 9:46 AM, Steve Lhomme <robux4 at gmail.com> wrote:
> 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