[vlc-devel] [PATCH] Preliminary dual subtitles support.

Thomas Guillem thomas at gllm.fr
Wed Apr 24 11:51:33 CEST 2019



On Wed, Apr 24, 2019, at 10:17, Thomas Guillem wrote:
> Hello,
> 
> Thanks a lot for working on this really wanted feature.
> 
> On Tue, Apr 23, 2019, at 12:43, Roland Bewick wrote:
> > Secondary subtitle track will be displayed at the top of the video.
> > Screenshot: http://prntscr.com/nfok5k
> > 
> > Usage:
> > - Select two subtitle tracks.
> > - If two subtitle tracks are already selected and a third is selected,
> >     the previous secondary subtitle track will be unselected.
> > - To change the primary sub track,
> >     make sure to deselect the secondary one first.
> 
> This feature should be disabled by default. I think most people will 
> expect to unselect the previous subtitle when selecting a new one. You 
> should add a a new vlc option (cf. src/libvlc-module.c) to enable it.

OK, maybe not. The UI/player could take care of unselecting the previous SPU if we want only one SPU at a time.

> 
> > 
> > Limitations:
> > - Only SRT and USF subtitles are supported.
> >     I can update the other codecs if my chosen solution is accepted.
> 
> Solution looks OK to me, but I'm no spu export. Francois ?
> 
> > - Primary and secondary subtitles share track synchronization.
> >     This must be fixed.
> 
> Why not having one clock per spu ?
> 
> > - Hotkeys haven't been done yet.
> > A single hotkey could be added to switch state between
> >  manipulating the primary or secondary subtitle track.
> > - Legacy / VLC < 4.0 QT GUI isn't supported.
> >     I can add support for this so that people can still use their skins.
> > - Primary and secondary subtitles share the same style (font size, etc).
> > - Remote control is not supported.
> > 
> > Untested on MAC interface.
> 
> It should work on every OS.
> 
> > ---
> >  modules/codec/subsdec.c             |  4 +-
> >  modules/codec/subsusf.c             |  5 +-
> >  src/input/es_out.c                  | 84 +++++++++++++++++++++++++++--
> >  src/video_output/video_output.c     | 12 +++--
> >  src/video_output/vout_subpictures.c |  2 +-
> >  5 files changed, 93 insertions(+), 14 deletions(-)
> > 
> > diff --git a/modules/codec/subsdec.c b/modules/codec/subsdec.c
> > index 0016332906..a733dffce2 100644
> > --- a/modules/codec/subsdec.c
> > +++ b/modules/codec/subsdec.c
> > @@ -464,9 +464,9 @@ static subpicture_t *ParseText( decoder_t *p_dec, 
> > block_t *p_block )
> >      int i_inline_align = -1;
> >      p_spu_sys->region.p_segments = ParseSubtitles( &i_inline_align, 
> > psz_subtitle );
> >      free( psz_subtitle );
> > -    if( p_sys->i_align >= 0 ) /* bottom ; left, right or centered */
> > +    if( p_sys->i_align >= 0 ) /* user-set alignment */
> >      {
> > -        p_spu_sys->region.align = SUBPICTURE_ALIGN_BOTTOM | 
> > p_sys->i_align;
> > +        p_spu_sys->region.align = p_sys->i_align;
> >          p_spu_sys->region.inner_align = p_sys->i_align;
> >      }
> >      else if( i_inline_align >= 0 )
> > diff --git a/modules/codec/subsusf.c b/modules/codec/subsusf.c
> > index c02c52b52f..faef159576 100644
> > --- a/modules/codec/subsusf.c
> > +++ b/modules/codec/subsusf.c
> > @@ -436,12 +436,11 @@ static subpicture_region_t *CreateTextRegion( 
> > decoder_t *p_dec,
> >              }
> >          }
> >  
> > -        /* Set default or user align/magin.
> > +        /* Set default or user align/margin.
> >           * Style overriden if no user value. */
> >          p_text_region->i_x = i_sys_align > 0 ? 20 : 0;
> >          p_text_region->i_y = 10;
> > -        p_text_region->i_align = SUBPICTURE_ALIGN_BOTTOM |
> > -                                 ((i_sys_align > 0) ? i_sys_align : 0);
> > +        p_text_region->i_align = i_sys_align >= 0 ? i_sys_align : 
> > SUBPICTURE_ALIGN_BOTTOM;
> >  
> >          if( p_ssa_style )
> >          {
> > diff --git a/src/input/es_out.c b/src/input/es_out.c
> > index 3352602849..835975fa1f 100644
> > --- a/src/input/es_out.c
> > +++ b/src/input/es_out.c
> > @@ -142,6 +142,7 @@ typedef struct
> >  {
> >      int         i_count;    /* es count */
> >      es_out_id_t *p_main_es; /* current main es */
> > +    es_out_id_t *p_secondary_es; /* current secondary es (SPU_ES only) */
> 
> Do you really need this variable ? We can get multiple VIDEO ES 
> selected without having a reference to the "secondary" one.
> 
> >      enum es_out_policy_e e_policy;
> >  
> >      /* Parameters used for es selection */
> > @@ -300,6 +301,7 @@ static void EsOutPropsInit( es_out_es_props_t 
> > *p_props,
> >      p_props->i_channel = (psz_trackvar) ? var_GetInteger( p_input, 
> > psz_trackvar ): -1;
> >      p_props->i_demux_id = -1;
> >      p_props->p_main_es = NULL;
> > +    p_props->p_secondary_es = NULL;
> >  
> >      if( !input_priv(p_input)->b_preparsing && psz_langvar )
> >      {
> > @@ -343,7 +345,7 @@ es_out_t *input_EsOutNew( input_thread_t *p_input, 
> > float rate )
> >                      "video-track-id", "video-track", NULL, NULL );
> >      EsOutPropsInit( &p_sys->audio, true, p_input, 
> > ES_OUT_ES_POLICY_EXCLUSIVE,
> >                      "audio-track-id", "audio-track", "audio-language", 
> > "audio" );
> > -    EsOutPropsInit( &p_sys->sub,  false, p_input, 
> > ES_OUT_ES_POLICY_EXCLUSIVE,
> > +    EsOutPropsInit( &p_sys->sub,  false, p_input, 
> > ES_OUT_ES_POLICY_SIMULTANEOUS,
> >                      "sub-track-id", "sub-track", "sub-language", "sub" 
> 
> So, the ES_OUT_ES_POLICY_SIMULTANEOUS should depend on a VLC option.
> 
> > );
> >  
> >      p_sys->i_group_id = var_GetInteger( p_input, "program" );
> > @@ -1083,6 +1085,7 @@ static void EsOutProgramSelect( es_out_t *out, 
> > es_out_pgrm_t *p_pgrm )
> >          p_sys->audio.p_main_es = NULL;
> >          p_sys->video.p_main_es = NULL;
> >          p_sys->sub.p_main_es = NULL;
> > +        p_sys->sub.p_secondary_es = NULL;
> >      }
> >  
> >      msg_Dbg( p_input, "selecting program id=%d", p_pgrm->i_id );
> > @@ -1805,6 +1808,46 @@ static bool EsIsSelected( es_out_id_t *es )
> >          return es->p_dec != NULL;
> >      }
> >  }
> > +
> > +/**
> > +    Set the alignment used by the chosen subtitle codec.
> > +    This solution works for subsdec and subsusf formats. (SRT + MKV 
> > Embedded).
> > +
> > +    TODO: It would be nice to have a unified way of setting the 
> > subtitle alignment.
> > +    The other codecs could be updated to follow subsusf's example.
> > +*/
> > +static void EsOutSetDecoderSubtitleAlignment( es_out_t *out, 
> > es_out_id_t *p_es )
> > +{
> > +    es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
> > +    es_out_es_props_t *p_esprops = GetPropsByCat( p_sys, 
> > p_es->fmt.i_cat );
> > +    input_thread_t *p_input = p_sys->p_input;
> > +
> > +    if( p_es->fmt.i_cat == SPU_ES )
> > +    {
> > +        int i_subtitle_alignment = SUBPICTURE_ALIGN_BOTTOM;
> > +        if( p_esprops->p_main_es != NULL && p_es != 
> > p_esprops->p_main_es )
> > +        {
> > +            /* Show secondary sub track on the top of the video */
> > +            i_subtitle_alignment = SUBPICTURE_ALIGN_TOP;
> > +        }
> > +        /* Temporarily create this variable as only the configuration 
> > exists */
> > +        var_Create( p_input, "subsdec-align", VLC_VAR_INTEGER);
> > +        var_SetInteger( p_input, "subsdec-align",  
> > i_subtitle_alignment);
> > +    }
> > +}
> > +
> > +/* we don't need these alignment variables anymore (already read by 
> > the codec) */
> > +static void EsOutReleaseDecoderAlignmentVariables( es_out_t *out, 
> > es_out_id_t *p_es )
> > +{
> > +    es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
> > +    input_thread_t *p_input = p_sys->p_input;
> > +    
> > +    if( p_es->fmt.i_cat == SPU_ES )
> > +    {
> > +        var_Destroy( p_input, "subsdec-align" );
> > +    }
> > +}
> > +
> >  static void EsOutCreateDecoder( es_out_t *out, es_out_id_t *p_es )
> >  {
> >      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
> > @@ -1821,8 +1864,13 @@ static void EsOutCreateDecoder( es_out_t *out, 
> > es_out_id_t *p_es )
> >      if( !p_es->p_clock )
> >          return;
> >  
> > +    EsOutSetDecoderSubtitleAlignment( out, p_es );
> > +
> >      dec = input_DecoderNew( p_input, &p_es->fmt, p_es->p_clock,
> >                              input_priv(p_input)->p_sout );
> > +
> > +    EsOutReleaseDecoderAlignmentVariables( out, p_es );
> > +
> >      if( dec != NULL )
> >      {
> >          input_DecoderChangeRate( dec, p_sys->rate );
> > @@ -1998,6 +2046,7 @@ static void EsDeleteCCChannels( es_out_t *out, 
> > es_out_id_t *parent )
> >  static void EsOutUnselectEs( es_out_t *out, es_out_id_t *es, bool 
> > b_update )
> >  {
> >      es_out_sys_t *p_sys = container_of(out, es_out_sys_t, out);
> > +    es_out_es_props_t *p_esprops = GetPropsByCat( p_sys, es->fmt.i_cat 
> > );
> >      input_thread_t *p_input = p_sys->p_input;
> >  
> >      if( !EsIsSelected( es ) )
> > @@ -2006,6 +2055,15 @@ static void EsOutUnselectEs( es_out_t *out, 
> > es_out_id_t *es, bool b_update )
> >          return;
> >      }
> >  
> > +    if( es == p_esprops->p_main_es && es->fmt.i_cat == SPU_ES )
> > +    {
> > +        p_esprops->p_main_es = NULL;
> > +    }
> > +    else if( es == p_esprops->p_secondary_es )
> > +    {
> > +        p_esprops->p_secondary_es = NULL;
> > +    }
> > +
> >      if( es->p_master )
> >      {
> >          if( es->p_master->p_dec )
> > @@ -2035,7 +2093,7 @@ static void EsOutUnselectEs( es_out_t *out, 
> > es_out_id_t *es, bool b_update )
> >   *
> >   * \param out The es_out structure
> >   * \param es es_out_id structure
> > - * \param b_force ...
> > + * \param b_force force the selection of the ES even if it is lower 
> > priority
> >   * \return nothing
> >   */
> >  static void EsOutSelect( es_out_t *out, es_out_id_t *es, bool b_force )
> > @@ -2049,8 +2107,11 @@ static void EsOutSelect( es_out_t *out, 
> > es_out_id_t *es, bool b_force )
> >          return;
> >      }
> >  
> > +    /* Only auto-unselect SPU ES when looking for a higher priority ES 
> > */
> > +    bool b_auto_unselect_spu = es->fmt.i_cat == SPU_ES && !b_force;
> > +
> >      bool b_auto_unselect = p_esprops && p_sys->i_mode == 
> > ES_OUT_MODE_AUTO &&
> > -                           p_esprops->e_policy == 
> > ES_OUT_ES_POLICY_EXCLUSIVE &&
> > +                           ( p_esprops->e_policy == 
> > ES_OUT_ES_POLICY_EXCLUSIVE || b_auto_unselect_spu ) &&
> >                             p_esprops->p_main_es && 
> > p_esprops->p_main_es != es;
> >  
> >      if( p_sys->i_mode == ES_OUT_MODE_ALL || b_force )
> > @@ -2059,6 +2120,11 @@ static void EsOutSelect( es_out_t *out, 
> > es_out_id_t *es, bool b_force )
> >          {
> >              if( b_auto_unselect )
> >                  EsOutUnselectEs( out, p_esprops->p_main_es, true );
> > +            else if( b_force && p_esprops->p_secondary_es && 
> > p_esprops->p_secondary_es != es )
> > +            {
> > +                /* Auto unselect secondary subtrack */
> > +                EsOutUnselectEs( out, p_esprops->p_secondary_es, true 
> > );
> > +            }
> >  
> >              EsOutSelectEs( out, es );
> >          }
> > @@ -2172,7 +2238,17 @@ static void EsOutSelect( es_out_t *out, 
> > es_out_id_t *es, bool b_force )
> >  
> >      /* FIXME TODO handle priority here */
> >      if( p_esprops && p_sys->i_mode == ES_OUT_MODE_AUTO && EsIsSelected( es ) )
> > -        p_esprops->p_main_es = es;
> > +    {
> > +        if( p_esprops->p_main_es == NULL || es->fmt.i_cat != SPU_ES )
> > +        {
> > +            p_esprops->p_main_es = es;
> > +        }
> > +        else
> > +        {
> > +            /* Only set a secondary SPU if we already have a primary one */
> > +            p_esprops->p_secondary_es = es;
> > +        }
> > +    }
> >  }
> >  
> >  static void EsOutCreateCCChannels( es_out_t *out, vlc_fourcc_t codec, 
> > uint64_t i_bitmap,
> > diff --git a/src/video_output/video_output.c 
> > b/src/video_output/video_output.c
> > index 9604522825..df5ff3d327 100644
> > --- a/src/video_output/video_output.c
> > +++ b/src/video_output/video_output.c
> > @@ -271,10 +271,14 @@ int vout_RegisterSubpictureChannel( vout_thread_t 
> > *vout )
> >  
> >  void vout_SetSubpictureClock( vout_thread_t *vout, vlc_clock_t *clock )
> >  {
> > -    vlc_mutex_lock(&vout->p->spu_lock);
> > -    if (vout->p->spu)
> > -        spu_clock_Set(vout->p->spu, clock);
> > -    vlc_mutex_unlock(&vout->p->spu_lock);
> > +    /* Currently all subtitle tracks share the same clock, so it must 
> > be valid. */
> > +    if ( clock != NULL )
> > +    {
> > +        vlc_mutex_lock(&vout->p->spu_lock);
> > +        if (vout->p->spu)
> > +            spu_clock_Set(vout->p->spu, clock);
> > +        vlc_mutex_unlock(&vout->p->spu_lock);
> 
> Why ? Is the secondary SPU also created via EsOutCreateDecoder(), in 
> that case, it should have its own clock, no ?
> 
> > +    }
> >  }
> >  
> >  void vout_FlushSubpictureChannel( vout_thread_t *vout, int channel )
> > diff --git a/src/video_output/vout_subpictures.c 
> > b/src/video_output/vout_subpictures.c
> > index 6b106534d3..8b530d3c2c 100644
> > --- a/src/video_output/vout_subpictures.c
> > +++ b/src/video_output/vout_subpictures.c
> > @@ -476,7 +476,7 @@ static void SpuRegionPlace(int *x, int *y,
> >          *y = region->i_y;
> >      } else {
> >          if (region->i_align & SUBPICTURE_ALIGN_TOP)
> > -            *y = region->i_y;
> > +            *y = region->i_y + ( subpic->b_subtitle ? 
> > region->fmt.i_visible_height : 0 );
> >          else if (region->i_align & SUBPICTURE_ALIGN_BOTTOM)
> >              *y = subpic->i_original_picture_height - 
> > region->fmt.i_visible_height - region->i_y;
> >          else
> > -- 
> > 2.17.1
> 
> Regards,
> 
> > 
> > _______________________________________________
> > 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


More information about the vlc-devel mailing list