[vlc-devel] [PATCH] GSoC: lua services discovery, first try

Fabio Ritrovato exsephiroth87 at gmail.com
Thu Jul 23 18:19:18 CEST 2009


2009/7/23 Rémi Denis-Courmont <remi at remlab.net>:
> Le jeudi 23 juillet 2009 18:35:20 Fabio Ritrovato, vous avez écrit :
>> > Locking in services_discovery_AddParameterValue is weird and
>> > inconsistent. The va_list variant should take the lock instead. In fact,
>> > I couldn't figure out what the lock was protecting... ? Also why do you
>> > need new functions and new events to manage SD items?
>>
>> Why is that?
>
> Why is what? common sense? principle of least surprise?
>
>> Take a look at input_item_AddInfo() since it's basically
>> the same code...
>
> That's not an excuse to do the same mistake.
>
>> What's protected is the parameter list, one thing i probably forgot is
>> to actually use the lock in the interface...
>
> If the item itself is not protected, why does the parameter list need to be?
> Or to put it another way, if the parameter list needs to be protected why does
> the rest not need to be too?

I think because the services_discovery_t is not access in a way that
needs a lock...
I'm accessing the parameter list from 2 different threads, the SD
thread to set it, and the interface thread to read it, so I thought a
lock was needed...
If I'm wrong, I'll remove it, no problem...

>> Because you can't manage SD items right now; or, to be right, you can
>> only manage the actual playlist items, but you can't do anything with
>> them...
>> With the new functions, you can dinamically add items to the SD, for
>> example in the youtube script, you can search for a string and the
>> videos are added to the playlist, or in the mtp SD you can transfer
>> song to the device, that's what the new functions and events are
>> for...
>
> The whole point of SD is to add (and remove) items. I don't see that.

The point of the SD is to add items, but from the code, the user can't
interact with them...
For example, the shoutcast SD, it just add a list of radios and that's it...
If a youtube SD was to be made as of now, it would simply list some
videos, decided by whom coded the module (for example, list the video
in the hompage), and stop...
With those functions, you can perform searches, add the videos you
want, show the video from a category...

>> > In 0003, the closing function will crash if vlc_clone() failed (which can
>> > happen).
>>
>> Is checking if p_sys->p_thread != NULL enough?

if( p_sd->p_sys->thread != NULL )
{
    vlc_cancel (p_sd->p_sys->thread);
    vlc_join (p_sd->p_sys->thread, NULL);
}

>> > Also, registering to your own event manager seems rather silly. Unless I
>> > am missing something this is meant for libvlc applications to talk to
>> > plugins, not for plugins to talk to themselves.
>>
>> Because I need to wait for vlc_sd_Start() to finish, if not the Run()
>> function will start too fast and crash while trying to use stuff not
>> already allocated...
>
> That simply *cannot* be. vlc_sd_Start() does not allocate _anything_ after
> module_need():
>
> bool vlc_sd_Start ( services_discovery_t * p_sd, const char *module )
> {
>    assert(!p_sd->p_module);
>
>    p_sd->p_module = module_need( p_sd, "services_discovery", module, true );
>
>    if( p_sd->p_module == NULL )
>    {
>        msg_Err( p_sd, "no suitable services discovery module" );
>        return false;
>    }
>
>    vlc_event_t event = {
>        .type = vlc_ServicesDiscoveryStarted
>    };
>    vlc_event_send( &p_sd->event_manager, &event );
>    return true;
> }

module_need is exactly the one that's allocating stuff and it's not
fast enough...
Without the event, it crash on module_get_name, because p_module it's
still not allocated...



More information about the vlc-devel mailing list