[vlc-devel] [RFC PATCHv2 05/18] es_out: fix group_id and i_id collision

Thomas Guillem thomas at gllm.fr
Wed Feb 19 09:41:15 CET 2020



On Tue, Feb 18, 2020, at 18:16, Rémi Denis-Courmont wrote:
> Hi,
> 
> Identification with a tuple of a (reference-counted) pointer and an ID seems like the worse of both worlds. It's not serial, but it's not a single value and its uniqueness depends on the demuxer.
> 
> Why can't we identify an ES by its ID pointer, which already exists (though it might be missing a reference count)??

We can already identify an ES track by its ID pointer, yes it is refcounted and it is currently used by all intf/control modules.
I plan to do the same for LibVLC.

But anyway, we will always need to identify a track with a string for the following use case:

 - Selecting a precise track via command line, with --video-track-id "video/1" or --sub-track-id "myfile.srt/sub/0"
 - Remembering the user track selection preference in a database, to restore the previous state later. Therefore the track id need to be a string or a int.

Indeed,
selecting a track via its ID pointer is 100% sure,
selecting a track via its string id is a little error prone (I would say more than 99% of success now).

The only 3 collision issues we could have now (once this set is merged):
 - 2 sub slaves having the same name: In that case, the second source will have an other ID but it will now depend on slave addition order. cf. my comment on patch 04/18.
 - Forged mkv/mp4/ts with tracks having the same track id
 - ts adding a track, removing it, and adding an other track with the same ID

Collision will be expected (because you can never trust the input) but won't be critical. The only problems you will have is that your prefered track might be not saved on a second VLC run.

If we really don't want collision (for the last 2 cases), I can "-[0-9]" to my tracks ID having the same ID than previous ones (but then, it will depend on addition order).

Example: video/245 and video/245-0

> 
> Le 18 février 2020 18:11:18 GMT+02:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>> A specified program (group > 0) is identified by its group id and by its
>> context pointer. The default program (group = 0) can be used by more than one
>> contexts.
>> 
>> An ES track is now identified by its i_id and by its context pointer. In case
>> of automatic track ids, the id counter will be handled by each contexts (or
>> input source). This mean that the id counter starts at 0 for each new source.
>> 
>> A slave specifying its group won't be able to be played alongside the master
>> source by default (except if we play more than one programs at a time). src/input/es_out.c | 106 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 75 insertions(+), 31 deletions(-)
>> 
>> diff --git a/src/input/es_out.c b/src/input/es_out.c
>> index 1c98d85add4..4ae27b63734 100644
>> --- a/src/input/es_out.c
>> +++ b/src/input/es_out.c
>> @@ -61,6 +61,9 @@
>>   *****************************************************************************/
>>  typedef struct
>>  {
>> +    /* Program context */
>> +    es_out_ctx_t *ctx;
>> +
>>      /* Program ID */
>>      int i_id;
>>  
>> @@ -98,6 +101,10 @@ struct es_out_id_t
>>      /* weak reference, used by input_decoder_callbacks and vlc_clock_cbs */
>>      es_out_t *out;
>>  
>> +    /* Context used to create this ES. Equals to p_pgrm->ctx when not using a
>> +     * default program id. */
>> +    es_out_ctx_t *ctx;
>> +
>>      /* ES ID */
>>      es_out_pgrm_t *p_pgrm;
>>  
>> @@ -165,6 +172,9 @@ typedef struct
>>  {
>>      input_thread_t *p_input;
>>  
>> +    /* Main source context */
>> +    es_out_ctx_t *main_ctx;
>> +
>>      /* */
>>      vlc_mutex_t   lock;
>>  
>> @@ -481,6 +491,13 @@ es_out_t *input_EsOutNew( input_thread_t *p_input, float rate )
>>  
>>      p_sys->out.cbs = &es_out_cbs;
>>  
>> +    p_sys->main_ctx = es_out_ctx_New(NULL);
>> +    if( !p_sys->main_ctx )
>> +    {
>> +        free( p_sys );
>> +        return NULL;
>> +    }
>> +
>>      vlc_mutex_init( &p_sys->lock );
>>      p_sys->p_input = p_input;
>>  
>> @@ -578,6 +595,7 @@ static void EsRelease(es_out_id_t *es)
>>          free(es->psz_language);
>>          free(es->psz_language_code);
>>          es_format_Clean(&es->fmt);
>> +        es_out_ctx_Release(es->ctx);
>>          free(es);
>>      }
>>  }
>> @@ -600,6 +618,8 @@ static void EsOutDelete( es_out_t *out )
>>  
>>      vlc_mutex_destroy( &p_sys->lock );
>>  
>> +    es_out_ctx_Release( p_sys->main_ctx );
>> +
>>      free( p_sys );
>>  }
>>  
>> @@ -609,6 +629,7 @@ static void ProgramDelete( es_out_pgrm_t *p_pgrm )
>>      vlc_clock_main_Delete( p_pgrm->p_main_clock );
>>      if( p_pgrm->p_meta )
>>          vlc_meta_Delete( p_pgrm->p_meta );
>> +    es_out_ctx_Release( p_pgrm->ctx );
>>  
>>      free( p_pgrm );
>>  }
>> @@ -1245,10 +1266,11 @@ static void EsOutSendEsEvent(es_out_t *out, es_out_id_t *es, int action)
>>      });
>>  }
>>  
>> -static bool EsOutIsProgramVisible( es_out_t *out, int i_group )
>> +static bool EsOutIsProgramVisible( es_out_t *out, es_out_ctx_t *ctx, int i_group )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>> -    return p_sys->i_group_id == 0 || p_sys->i_group_id == i_group;
>> +    return p_sys->i_group_id == 0
>> +        || (p_sys->i_group_id == i_group && p_sys->p_pgrm->ctx == ctx);
>>  }
>>  
>>  /* EsOutProgramSelect:
>> @@ -1333,7 +1355,7 @@ static void EsOutProgramSelect( es_out_t *out, es_out_pgrm_t *p_pgrm )
>>  /* EsOutAddProgram:
>>   *  Add a program
>>   */
>> -static es_out_pgrm_t *EsOutProgramAdd( es_out_t *out, int i_group )
>> +static es_out_pgrm_t *EsOutProgramAdd( es_out_t *out, es_out_ctx_t *ctx, int i_group )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>>      input_thread_t    *p_input = p_sys->p_input;
>> @@ -1343,6 +1365,7 @@ static es_out_pgrm_t *EsOutProgramAdd( es_out_t *out, int i_group )
>>          return NULL;
>>  
>>      /* Init */
>> +    p_pgrm->ctx = es_out_ctx_Hold( ctx );
>>      p_pgrm->i_id = i_group;
>>      p_pgrm->i_es = 0;
>>      p_pgrm->b_selected = false;
>> @@ -1376,7 +1399,7 @@ static es_out_pgrm_t *EsOutProgramAdd( es_out_t *out, int i_group )
>>      vlc_list_append(&p_pgrm->node, &p_sys->programs);
>>  
>>      /* Update "program" variable */
>> -    if( EsOutIsProgramVisible( out, i_group ) )
>> +    if( EsOutIsProgramVisible( out, ctx, i_group ) )
>>          input_SendEventProgramAdd( p_input, i_group, NULL );
>>  
>>      if( i_group == p_sys->i_group_id || ( !p_sys->p_pgrm && p_sys->i_group_id == 0 ) )
>> @@ -1387,34 +1410,37 @@ static es_out_pgrm_t *EsOutProgramAdd( es_out_t *out, int i_group )
>>  
>>  /* EsOutProgramSearch
>>   */
>> -static es_out_pgrm_t *EsOutProgramSearch( es_out_t *p_out, int i_group )
>> +static es_out_pgrm_t *EsOutProgramSearch( es_out_t *p_out, es_out_ctx_t *ctx,
>> +                                          int i_group )
>>  {
>>      es_out_sys_t *p_sys = container_of(p_out, es_out_sys_t, out);
>>      es_out_pgrm_t *pgrm;
>>  
>>      vlc_list_foreach(pgrm, &p_sys->programs, node)
>> -        if (pgrm->i_id == i_group)
>> +        if (pgrm->i_id == i_group && (pgrm->ctx == ctx || i_group == 0))
>>              return pgrm;
>> +
>>      return NULL;
>>  }
>>  
>>  /* EsOutProgramInsert
>>   */
>> -static es_out_pgrm_t *EsOutProgramInsert( es_out_t *p_out, int i_group )
>> +static es_out_pgrm_t *EsOutProgramInsert( es_out_t *p_out, es_out_ctx_t *ctx,
>> +                                          int i_group )
>>  {
>> -    es_out_pgrm_t *pgrm = EsOutProgramSearch( p_out, i_group );
>> -    return pgrm ? pgrm : EsOutProgramAdd( p_out, i_group );
>> +    es_out_pgrm_t *pgrm = EsOutProgramSearch( p_out, ctx, i_group );
>> +    return pgrm ? pgrm : EsOutProgramAdd( p_out, ctx, i_group );
>>  }
>>  
>>  /* EsOutDelProgram:
>>   *  Delete a program
>>   */
>> -static int EsOutProgramDel( es_out_t *out, int i_group )
>> +static int EsOutProgramDel( es_out_t *out, es_out_ctx_t *ctx, int i_group )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>>      input_thread_t    *p_input = p_sys->p_input;
>>  
>> -    es_out_pgrm_t *p_pgrm = EsOutProgramSearch( out, i_group );
>> +    es_out_pgrm_t *p_pgrm = EsOutProgramSearch( out, ctx, i_group );
>>      if( p_pgrm == NULL )
>>          return VLC_EGENERIC;
>>  
>> @@ -1483,7 +1509,8 @@ static char *EsInfoCategoryName( es_out_id_t* es )
>>      return psz_category;
>>  }
>>  
>> -static void EsOutProgramMeta( es_out_t *out, int i_group, const vlc_meta_t *p_meta )
>> +static void EsOutProgramMeta( es_out_t *out, es_out_ctx_t *ctx,
>> +                              int i_group, const vlc_meta_t *p_meta )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>>      es_out_pgrm_t     *p_pgrm;
>> @@ -1511,9 +1538,9 @@ static void EsOutProgramMeta( es_out_t *out, int i_group, const vlc_meta_t *p_me
>>      }
>>  
>>      /* Find program */
>> -    if( !EsOutIsProgramVisible( out, i_group ) )
>> +    if( !EsOutIsProgramVisible( out, ctx, i_group ) )
>>          return;
>> -    p_pgrm = EsOutProgramInsert( out, i_group );
>> +    p_pgrm = EsOutProgramInsert( out, ctx, i_group );
>>      if( !p_pgrm )
>>          return;
>>  
>> @@ -1612,7 +1639,8 @@ static void EsOutProgramMeta( es_out_t *out, int i_group, const vlc_meta_t *p_me
>>          input_SendEventMetaInfo( p_input );
>>  }
>>  
>> -static void EsOutProgramEpgEvent( es_out_t *out, int i_group, const vlc_epg_event_t *p_event )
>> +static void EsOutProgramEpgEvent( es_out_t *out, es_out_ctx_t *ctx,
>> +                                  int i_group, const vlc_epg_event_t *p_event )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>>      input_thread_t    *p_input = p_sys->p_input;
>> @@ -1620,16 +1648,17 @@ static void EsOutProgramEpgEvent( es_out_t *out, int i_group, const vlc_epg_even
>>      es_out_pgrm_t     *p_pgrm;
>>  
>>      /* Find program */
>> -    if( !EsOutIsProgramVisible( out, i_group ) )
>> +    if( !EsOutIsProgramVisible( out, ctx, i_group ) )
>>          return;
>> -    p_pgrm = EsOutProgramInsert( out, i_group );
>> +    p_pgrm = EsOutProgramInsert( out, ctx, i_group );
>>      if( !p_pgrm )
>>          return;
>>  
>>      input_item_SetEpgEvent( p_item, p_event );
>>  }
>>  
>> -static void EsOutProgramEpg( es_out_t *out, int i_group, const vlc_epg_t *p_epg )
>> +static void EsOutProgramEpg( es_out_t *out, es_out_ctx_t *ctx,
>> +                             int i_group, const vlc_epg_t *p_epg )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>>      input_thread_t    *p_input = p_sys->p_input;
>> @@ -1638,9 +1667,9 @@ static void EsOutProgramEpg( es_out_t *out, int i_group, const vlc_epg_t *p_epg
>>      char *psz_cat;
>>  
>>      /* Find program */
>> -    if( !EsOutIsProgramVisible( out, i_group ) )
>> +    if( !EsOutIsProgramVisible( out, ctx, i_group ) )
>>          return;
>> -    p_pgrm = EsOutProgramInsert( out, i_group );
>> +    p_pgrm = EsOutProgramInsert( out, ctx, i_group );
>>      if( !p_pgrm )
>>          return;
>>  
>> @@ -1874,7 +1903,8 @@ static void EsOutFillEsFmt(es_out_t *out, es_format_t *fmt)
>>      }
>>  }
>>  
>> -static es_out_id_t *EsOutAddLocked( es_out_t *out, const es_format_t *fmt,
>> +static es_out_id_t *EsOutAddLocked( es_out_t *out, es_out_ctx_t *ctx,
>> +                                    const es_format_t *fmt,
>>                                      es_out_id_t *p_master )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>> @@ -1893,26 +1923,32 @@ static es_out_id_t *EsOutAddLocked( es_out_t *out, const es_format_t *fmt,
>>          return NULL;
>>  
>>      es->out = out;
>> +    es->ctx = es_out_ctx_Hold( ctx );
>>  
>>      if( es_format_Copy( &es->fmt, fmt ) != VLC_SUCCESS )
>>      {
>>          free( es );
>>          return NULL;
>>      }
>> +
>>      if( es->fmt.i_id < 0 )
>> -        es->fmt.i_id = p_sys->i_id;
>> +        es->fmt.i_id = es_out_ctx_GetNewEsID( ctx );
>>      if( !es->fmt.i_original_fourcc )
>>          es->fmt.i_original_fourcc = es->fmt.i_codec;
>>  
>>      /* Search the program */
>> -    p_pgrm = EsOutProgramInsert( out, fmt->i_group );
>> +    p_pgrm = EsOutProgramInsert( out, ctx, fmt->i_group );
>>      if( !p_pgrm )
>>      {
>>          es_format_Clean( &es->fmt );
>> +        es_out_ctx_Release( es->ctx );
>>          free( es );
>>          return NULL;
>>      }
>>  
>> +    /* The group 0 is the default one and can be used by different contexts */
>> +    assert( fmt->i_group == 0 || p_pgrm->ctx == es->ctx );
>> +
>>      /* Get the number of ES already added in order to get the position of the es */
>>      es->i_pos = 0;
>>      es_out_id_t *it;
>> @@ -1989,8 +2025,12 @@ static es_out_id_t *EsOutAdd( es_out_t *out, es_out_ctx_t *ctx, const es_format_
>>  {
>>      VLC_UNUSED(ctx);
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>> +
>> +    if( !ctx )
>> +        ctx = p_sys->main_ctx;
>> +
>>      vlc_mutex_lock( &p_sys->lock );
>> -    es_out_id_t *es = EsOutAddLocked( out, fmt, NULL );
>> +    es_out_id_t *es = EsOutAddLocked( out, ctx, fmt, NULL );
>>      vlc_mutex_unlock( &p_sys->lock );
>>      return es;
>>  }
>> @@ -2481,7 +2521,7 @@ static void EsOutCreateCCChannels( es_out_t *out, vlc_fourcc_t codec, uint64_t i
>>              fmt.psz_description = NULL;
>>  
>>          es_out_id_t **pp_es = &parent->cc.pp_es[i];
>> -        *pp_es = EsOutAddLocked( out, &fmt, parent );
>> +        *pp_es = EsOutAddLocked( out, parent->p_pgrm->ctx, &fmt, parent );
>>          es_format_Clean( &fmt );
>>  
>>          /* */
>> @@ -2750,6 +2790,7 @@ static int EsOutVaControlLocked( es_out_t *out, es_out_ctx_t *ctx,
>>                                   int i_query, va_list args )
>>  {
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>> +    assert( ctx ); /* == p_sys->main_ctx if the given ctx is NULL */
>>  
>>      switch( i_query )
>>      {
>> @@ -3024,12 +3065,12 @@ static int EsOutVaControlLocked( es_out_t *out, es_out_ctx_t *ctx,
>>          {
>>              p_pgrm = p_sys->p_pgrm;
>>              if( !p_pgrm )
>> -                p_pgrm = EsOutProgramAdd( out, i_group );   /* Create it */
>> +                p_pgrm = EsOutProgramAdd( out, ctx, i_group );   /* Create it */
>>          }
>>          else
>>          {
>>              i_group = va_arg( args, int );
>> -            p_pgrm = EsOutProgramInsert( out, i_group );
>> +            p_pgrm = EsOutProgramInsert( out, ctx, i_group );
>>          }
>>          if( !p_pgrm )
>>              return VLC_EGENERIC;
>> @@ -3202,7 +3243,7 @@ static int EsOutVaControlLocked( es_out_t *out, es_out_ctx_t *ctx,
>>          int i_group = va_arg( args, int );
>>          const vlc_meta_t *p_meta = va_arg( args, const vlc_meta_t * );
>>  
>> -        EsOutProgramMeta( out, i_group, p_meta );
>> +        EsOutProgramMeta( out, ctx, i_group, p_meta );
>>          return VLC_SUCCESS;
>>      }
>>      case ES_OUT_SET_GROUP_EPG:
>> @@ -3210,7 +3251,7 @@ static int EsOutVaControlLocked( es_out_t *out, es_out_ctx_t *ctx,
>>          int i_group = va_arg( args, int );
>>          const vlc_epg_t *p_epg = va_arg( args, const vlc_epg_t * );
>>  
>> -        EsOutProgramEpg( out, i_group, p_epg );
>> +        EsOutProgramEpg( out, ctx, i_group, p_epg );
>>          return VLC_SUCCESS;
>>      }
>>      case ES_OUT_SET_GROUP_EPG_EVENT:
>> @@ -3218,7 +3259,7 @@ static int EsOutVaControlLocked( es_out_t *out, es_out_ctx_t *ctx,
>>          int i_group = va_arg( args, int );
>>          const vlc_epg_event_t *p_evt = va_arg( args, const vlc_epg_event_t * );
>>  
>> -        EsOutProgramEpgEvent( out, i_group, p_evt );
>> +        EsOutProgramEpgEvent( out, ctx, i_group, p_evt );
>>          return VLC_SUCCESS;
>>      }
>>      case ES_OUT_SET_EPG_TIME:
>> @@ -3233,7 +3274,7 @@ static int EsOutVaControlLocked( es_out_t *out, es_out_ctx_t *ctx,
>>      {
>>          int i_group = va_arg( args, int );
>>  
>> -        return EsOutProgramDel( out, i_group );
>> +        return EsOutProgramDel( out, ctx, i_group );
>>      }
>>  
>>      case ES_OUT_SET_META:
>> @@ -3559,6 +3600,9 @@ static int EsOutControl( es_out_t *out, es_out_ctx_t *ctx,
>>      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
>>      int i_ret;
>>  
>> +    if( !ctx )
>> +        ctx = p_sys->main_ctx;
>> +
>>      vlc_mutex_lock( &p_sys->lock );
>>      i_ret = EsOutVaControlLocked( out, ctx, i_query, args );
>>      vlc_mutex_unlock( &p_sys->lock );
> 
> -- 
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200219/befe6f25/attachment-0001.html>


More information about the vlc-devel mailing list