[vlc-devel] [PATCH] lua: input: Expose tracks through lua bindings

[Pierre Ynard]

> +typedef struct input_track
> +{
> +    int i_id;
> +    int es_type;
> +} input_track;

Aren't ES IDs unique regardless of type? I'm not sure what's the point
of the es_type field.

> +static int vlclua_input_compare_track_id( lua_State* L )
> +{
> +    input_track* left  = luaL_checkudata( L, 1, "track" );
> +    input_track* right = luaL_checkudata( L, 2, "track" );
> +    lua_pushboolean( L, left != NULL && right != NULL &&
> +            left->i_id == right->i_id && left->es_type == right->es_type );
> +    return 1;
> +}

I haven't read the doc on what it's supposed to return, but shouldn't it
return true if both are NULL, and isn't checking es_type unnecessary?
You can put asserts if it's not supposed to happen.

> +static const char* orientation_to_str(video_orientation_t orient)
> +{
> +    switch (orient)
> +    {
> +    case ORIENT_TOP_LEFT:
> +        return "topleft";
> +    case ORIENT_TOP_RIGHT:
> +        return "topright";
> +    case ORIENT_BOTTOM_LEFT:
> +        return "bottomleft";

These don't seem intuitive at all for a user.

> +            PUSH_STR( p_es->subs.psz_encoding ? p_es->subs.psz_encoding : "", "encoding" );

lua_pushstring() supports NULL input. I don't know what a NULL
psz_encoding means, but maybe nil or unset will do just fine instead of
the empty string.

> +static int vlclua_input_item_current_track( lua_State* L )
> +{
> +    input_thread_t* p_input_thread = vlclua_get_input_internal( L );
> +    if ( p_input_thread == NULL )
> +        return luaL_error( L, "vlc.input:current_track(): No playback in progress" );

Error handling would be simpler if this was moved after the other
checks.

> +        return luaL_error( L, "vlc.input:set_track(): invalid track type provided" );

Copy-paste error.

> +    for( int i = 0; i < val_list.p_list->i_count; i++ )
> +    {
> +        msg_Err( p_input_thread, "Comparing tracks %d and %" PRId64, p_track->i_id, val_list.p_list->p_values[i].i_int);

Stray debug.

> +        if( p_track->i_id == val_list.p_list->p_values[i].i_int )
> +        {
> +            if( strcmp( psz_track_type, "video-es" ) == 0 && var_SetBool( p_input_thread, psz_track_type, true ) < 0 )
> +                break;
> +            if( var_SetInteger( p_input_thread, psz_es_name, p_track->i_id ) < 0 )
> +                break;
> +            break;

I get most confused there.

You haven't explained much the purpose of the API; is it to view and
choose tracks? From where? At least the CLI already allows that. A lua
object ID isn't going to be especially easy to pass back from the UI to
the API.

These API functions look kinda weird to me. Perhaps it would be better
if the track itself was a lua object and the function to set the current
track was a method of it. But I still don't see how a lua script would
use it. That's depressing. There are two API functions to get tracks,
but neither of them is sufficient in itself to do the job so you have to
call both anyway.

-- 
Pierre Ynard
"Une âme dans un corps, c'est comme un dessin sur une feuille de papier."