[vlc-devel] [PATCH] Fix track selection according to user preference.

[Rafaël Carré]

Hello,

Logic is ok but I have a few remarks:

Le 13/07/2013 17:12, Denis Charmet a écrit :
> Fix #6375 and may benefit to #8936
> ---
>  src/input/es_out.c |  109 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 68 insertions(+), 41 deletions(-)
> 
> diff --git a/src/input/es_out.c b/src/input/es_out.c
> index c566287..d2f9803 100644
> --- a/src/input/es_out.c
> +++ b/src/input/es_out.c
> @@ -1756,28 +1756,37 @@ static void EsOutSelect( es_out_t *out, es_out_id_t *es, bool b_force )
>  
>          if( i_cat == AUDIO_ES )
>          {
> -            int idx1 = LanguageArrayIndex( p_sys->ppsz_audio_language,
> -                                     es->psz_language_code );
> -
> -            if( p_sys->p_es_audio &&
> -                p_sys->p_es_audio->fmt.i_priority >= es->fmt.i_priority )
> +            if( p_sys->ppsz_audio_language )
>              {
> -                int idx2 = LanguageArrayIndex( p_sys->ppsz_audio_language,
> +                int idx1 = LanguageArrayIndex( p_sys->ppsz_audio_language,
> +                                     es->psz_language_code );
> +                if( !p_sys->p_es_audio )
> +                {
> +                    /* Only select the language if it's in the list */
> +                    if( idx1 >= 0 )
> +                        i_wanted = es->i_channel;
> +                }
> +                else

else if (idx1 >= 0) {
 int idx2 = ....

BTW idx1 and idx2 really suck, please do something about it while you're
working on this function..

I will put splitting of this function on my TODO list but feel free to
beat me on that one.

> +                {
> +                    int idx2 = LanguageArrayIndex( p_sys->ppsz_audio_language,
>                                           p_sys->p_es_audio->psz_language_code );
> -
> -                if( idx1 < 0 || ( idx2 >= 0 && idx2 <= idx1 ) )
> -                    return;
> -                i_wanted = es->i_channel;
> +                    if( idx1 >= 0 )
> +                    {
> +                        if( idx1 < idx2 )
> +                            i_wanted = es->i_channel;
> +                        else if( idx1 == idx2 &&
> +                                 p_sys->p_es_audio->fmt.i_priority < es->fmt.i_priority )
> +                            i_wanted = es->i_channel;

You will hate me but I'd use || because both conditions have the single
same effect.

I think I split long || lines in several only when they break the flow
of the program (break / return / goto) but not for other statements.

> +                    }
> +                }
>              }
>              else
>              {
> -                /* Select audio if (no audio selected yet)
> -                 * - no audio-language
> -                 * - no audio code for the ES
> -                 * - audio code in the requested list */
> -                if( idx1 >= 0 ||
> -                    !strcmp( es->psz_language_code, "??" ) ||
> -                    !p_sys->ppsz_audio_language )
> +                /* no list select the first one */

The english is a bit weak, what about:

"No user-specified list of languages, select the first ES" ?

We are not limited to 140 characters! (well actually our limit is 80...)

> +                if( !p_sys->p_es_audio )
> +                    i_wanted = es->i_channel;
> +                /* Select the most prioritary */
> +                else if( p_sys->p_es_audio->fmt.i_priority < es->fmt.i_priority )
>                      i_wanted = es->i_channel;
>              }
>  
> @@ -1794,34 +1803,52 @@ static void EsOutSelect( es_out_t *out, es_out_id_t *es, bool b_force )
>          }
>          else if( i_cat == SPU_ES )
>          {
> -            int idx1 = LanguageArrayIndex( p_sys->ppsz_sub_language,
> -                                     es->psz_language_code );
> -
> -            if( p_sys->p_es_sub &&
> -                p_sys->p_es_sub->fmt.i_priority >= es->fmt.i_priority )
> +            if( p_sys->ppsz_sub_language )
>              {
> -                int idx2 = LanguageArrayIndex( p_sys->ppsz_sub_language,
> +                int idx1 = LanguageArrayIndex( p_sys->ppsz_sub_language,
> +                                     es->psz_language_code );
> +                if( !p_sys->p_es_sub )
> +                {
> +                    /* Select the language if it's in the list */
> +                    if( idx1 >= 0 )
> +                        i_wanted = es->i_channel;
> +                    /*FIXME: Should default subtitle not in the list be 
> +                     * displayed if not forbidden by none? */
> +                    else if( p_sys->i_default_sub_id >= 0 )
> +                    {
> +                        /* check if the subtitle isn't forbidden by none */
> +                        if( LanguageArrayIndex( p_sys->ppsz_sub_language, "none" ) < 0 )
> +                        {
> +                            if( es->i_id == p_sys->i_default_sub_id )
> +                                i_wanted = es->i_channel;
> +                        }
> +                    }
> +                }
> +                else
> +                {
> +                    int idx2 = LanguageArrayIndex( p_sys->ppsz_sub_language,
>                                           p_sys->p_es_sub->psz_language_code );
> -
> -                msg_Dbg( p_sys->p_input, "idx1=%d(%s) idx2=%d(%s)",
> -                        idx1, es->psz_language_code, idx2,
> -                        p_sys->p_es_sub->psz_language_code );
> -
> -                if( idx1 < 0 || ( idx2 >= 0 && idx2 <= idx1 ) )
> -                    return;
> -                /* We found a SPU that matches our language request */
> -                i_wanted  = es->i_channel;
> -            }
> -            else if( idx1 >= 0 )
> -            {
> -                msg_Dbg( p_sys->p_input, "idx1=%d(%s)",
> -                        idx1, es->psz_language_code );
> -
> -                i_wanted  = es->i_channel;
> +                    if( idx1 >= 0 )
> +                    {
> +                        if( idx1 < idx2 )
> +                            i_wanted = es->i_channel;
> +                        else if( idx1 == idx2 &&
> +                                 p_sys->p_es_sub->fmt.i_priority < es->fmt.i_priority )
> +                            i_wanted = es->i_channel;
> +                    }
> +                }
>              }
> -            else if( p_sys->i_default_sub_id >= 0 )
> +            else
>              {
> -                if( es->i_id == p_sys->i_default_sub_id )
> +                /* no list select the default one */
> +                if( !p_sys->p_es_sub )
> +                {
> +                    if( p_sys->i_default_sub_id >= 0 &&
> +                        es->i_id == p_sys->i_default_sub_id )
> +                        i_wanted = es->i_channel;
> +                }
> +                /* Select the most prioritary */

"Select ES based on priority" ?

> +                else if( p_sys->p_es_sub->fmt.i_priority < es->fmt.i_priority )
>                      i_wanted = es->i_channel;
>              }
>  
>