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

[Rémi Denis-Courmont]

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?

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

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

I don't know what you mean.

> > 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;
}

-- 
Rémi Denis-Courmont
http://www.remlab.net/