[vlc-devel] Support of events in Mozilla plugin - Beta

[jpd at videolan.org]

With apologies for the delay.

On Tue, Nov 03, 2009 at 02:27:29PM +0100, Yannick Br??hon wrote:
> >>>> First, I noticed you did use a bitmap, which is good, but I had
> >>>> something slightly different in mind. I have that code around so I'll
> >>>> drop that in soonish. Same with the list of events, or you could take
> >>>> a look at the lookup function in core.
> >>> We are not quite sure what you want here, can you please expand on this
> >>> topic? Thanks.

Sure. Something like:

// Note that the accessor functions are unsafe, but this is handled in
// the next layer up. 64bit uints can be substituted to taste (shift=6).
template<size_t M> class bitmap {
private:
    typedef uint32_t bitu_t; enum { shift=5 };
    enum { bmax=M, bpu=1<<shift, mask=bpu-1, units=(bmax+bpu-1)/bpu };
    bitu_t bits[units];
public:
    bool get(size_t idx) const { return bits[idx>>shift]&(1<<(idx&mask)); }
    void set(size_t idx)       { bits[idx>>shift]|=  1<<(idx&mask);  }
    void reset(size_t idx)     { bits[idx>>shift]&=~(1<<(idx&mask)); }
    void toggle(size_t idx)    { bits[idx>>shift]^=  1<<(idx&mask);  }
    size_t maxbit() const      { return bmax; }
    void clear()               { memset(bits,0,sizeof(bits)); }
    bitmap() { clear(); }
    ~bitmap() { }
};

class eventtypes_bitmap_t: private bitmap<libvlc_num_event_types> {
private:
    typedef libvlc_event_type_t event_t;
    event_t find_event(const char *s)
    {
        event_t i;
        for(i=0;i<maxbit();++i)
            if(!strcmp(s,libvlc_event_type_name(i)))
                break;
        return i;
    }
public:
    bool add_event(const char *s) {
        event_t event=find_event(s);
        bool b=event<maxbit();
        if(b) set(event);
        return b;
    }
    bool del_event(const char *s) {
        event_t event=find_event(s);
        bool b=event<maxbit();
        if(b) reset(event);
        return b;
    }
    bool have_event(libvlc_event_type_t event)
    {
        return event<maxbit()?get(event):false;
    }
};

Use an instance of eventtypes_bitmap_t to handle storing and filtering
events, one for each receiver. This implementation allows for all events
to be reported back. If that is too many, well, that can be fixed.

You can also keep a plugin-global instance of this to lazily register
callbacks with libvlc.

I think you'll find that with the above you can simplify if not outright
remove a lot of code. Note that the above code hasn't been tested;
please make sure you test it first.


> >>>> Second, I'd really rather you didn't traverse string arguments
> >>>> where you could be parsing an array-of-strings passed from
> >>>> javascript instead.
> >>> We have mimicked the mechanism used today to setup a new VLC
> >>> plugin to get the options. We now support both a string of options
> >>> or an array of strings (exactly the same as the options for a new
> >>> plugin).

I would like you to drop support for multiple events in a single string.

Instead, support any number of arguments of type string, or one argument
of type array of string. Feel free to return an error and do nothing as
soon as you find any argument that does not match those profiles.

This because I'd like a new API to come into life clean, and there
really is no need to spend any effort on picking apart strings for
tokens. Just let the type above look at the entire string, and only
handle walking through multiple string and array arguments.


> >>> We have deliberately not called the
> >>> LibvlcPlaylistNPObject::parseOptions method for the following two
> >>> reasons:
> >>> - it is apparently bound to evolve quite a bit according to the number
> >>> of FIXMEs and we are not sure our code will be compatible with such
> >>> evolutions and/or that our code won't prevent such evolutions

If the code is integrated then that ceases to be a problem: It will have
to be maintained integrally with the rest. But I wasn't asking you to
call parseOptions.


> >>> Moreover, we noticed on *our* development platform that
> >>> compilation fails because the HAS_NPFUNCTIONS_H variable is not
> >>> #defined for us. We had to manually define it, otherwise compiler
> >>> uses npupp.h instead of npfunctions.h, which doesn't define the
> >>> API functions we need for our patch.

Might be something for autoconf to handle, but I can't really comment
on that at this point.


> +// Verifies the version of the NPAPI.
> +// The eventListeners use a NPAPI function available
> +// since Gecko 1.9.
> +bool canUseEventListener()

Could use a static. In fact it should be a static member of the plugin
wrapper object.


>  const NPUTF8 * const LibvlcRootNPObject::methodNames[] =
>  {
>      "versionInfo",
> +    "addEventListener",
> +    "removeEventListeners"
>  };

After rethinking this for a while, and considering how the rest of the
plugin api is structured, this should change to an events property with
addListener and removeListener methods. Naming suggestions welcome.


> +#if (((NP_VERSION_MAJOR << 8) + NP_VERSION_MINOR) >= 20)
> +    return (*gNetscapeFuncs.pluginthreadasynccall)(plugin, func, userData);
> +#endif
> +}
[...]
>          gNetscapeFuncs.reloadplugins = nsTable->reloadplugins;
> +        gNetscapeFuncs.pluginthreadasynccall =
> +            nsTable->pluginthreadasynccall;

Looks like missing #if guards?