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

[Hugo Beauzée-Luyssen]

On 01/04/2017 03:24 PM, Pierre Ynard wrote:
>> +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.
>

That seems like a fair point.

>> +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.

Checking types probably is unnecessary indeed, not sure what to do if 
the tracks are NULL, though assert seems a bit harsh.

>
>> +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.

I find this more intuitive than their 
ORIENT_ROTATED_180/ORIENT_ROTATED_270 counterpart, I guess that's matter 
of tastes, but the way those values are defined seems to point at the 
TOP/BOTTOM/LEFT/RIGHT versions being the preferred ones.

>
>> +            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.
>

I agree, I wasn't sure whether to do it or not, it felt more natural to 
check the argument in order, but I'll reverse it.

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

I'm going to deny and silently fix it in the next commit.

>> +    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.

Also denying O:-)

>
>> +        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.
>

The specific goal here is to allow tracks to be exposed within the http 
interface, however it seems like a good motivation to also allow any lua 
extension to list/select tracks.
I haven't looked at the dialog API, but can't you simply attach a lua 
object to any widget you'd create from a script?

> 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.

I thought of that as it feels more natural (IMHO) in order to switch to 
a specific track, however I wanted to stay more or less in line with the 
way libvlc uses.

> 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.
>

Regards,