[vlc-devel] [PATCH 5/5] chromecast: add an experimental sout module that connects to a ChromeCast device and streams using the HTTP access output

[Adrien Maglo]

Hello Rémi,


Thank you for your review.

Le 20/08/2014 11:48, Rémi Denis-Courmont a écrit :
>> +struct sout_stream_sys_t
>> +{
>> +    sout_stream_sys_t()
>> +        : i_received(0), i_status(CHROMECAST_DISCONNECTED),
>> +          b_threadStarted(false), b_pingSent(false),
>> +          p_mux(NULL), p_access(NULL)
>> +    {
>> +        atomic_init(&ab_error, false);
>> +    }
>> +
>> +    vlc_url_t url;
>> +    string serverIP;
>> +
>> +    int i_sock_fd;
>> +    vlc_tls_creds_t *p_creds;
>> +    vlc_tls_t *p_tls;
>> +
>> +    vlc_thread_t chromecastThread;
>> +
>> +    unsigned i_received;
>> +    char p_packet[PACKET_MAX_LEN];
>> +    unsigned i_requestId;
>> +    string appTransportId;
>> +
>> +    queue<CastMessage> messagesToSend;
>> +
>> +    int i_status;
>> +    bool b_threadStarted;
>> +    bool b_pingSent;
>> +    atomic_bool ab_error;
>> +    vlc_mutex_t loadCommandMutex;
>> +    vlc_cond_t loadCommandCond;
>> +
>> +    char *psz_mux;
>> +    sout_mux_t* p_mux;
>> +    sout_access_out_t *p_access;
>
> There is the standard output if you want to hook a muxer to an access out.

Sure, I copied some code from the standard sout. I thought that creating 
a new sout would be a good idea to allow usages such as the one given in 
example in my first mail.
If you have a better idea about the type of module that should be used 
for this integration, please let me know.

> And while I have not checked in depth, that seems like a surprising lot
> of parameters that need to be retained.

I'll check if I can simplify that.

> Indeed at least b_threadStarted is always true across entry points, so
> useless.

Actually, b_threadStarted is initialized to false by the 
sout_stream_sys_t constructor. It is used by the Close() function to 
know whether it should kill the thread or not. Close() is indeed used 
after an error in Open() since gotos in C++ cannot be used like in C for 
error handling.

>> +/*****************************************************************************
>>
>> + * Sout callbacks
>> +
>>
>> *****************************************************************************/
>>
>> +struct sout_stream_id_sys_t
>> +{
>> +};

Well, copy-pasted from stream_out/standard.c

>> +    char *psz_ipChromecast = var_GetNonEmptyString(p_stream,
>> SOUT_CFG_PREFIX "receiver-ip");
>> +    if (psz_ipChromecast == NULL)
>> +    {
>> +        msg_Err(p_stream, "No Chromecast receiver IP provided");
>> +        Close(p_this);
>> +        return VLC_EGENERIC;
>> +    }
>> +
>> +    vlc_url_t url;
>
> Uh? Why?

It seems indeed useless to use a vlc_url_t here.

>> +    char psz_access[] = "http";
>> +    char psz_url[] = ":8009/stream";
>
> Useless string copies.

Ok

>> +    /* Ugly part:
>> +     * We want to be sure that the Chromecast receives the first
>> data packet sent by
>> +     * the HTTP server. */
>> +
>> +    // Lock the sout thread until we have sent the media loading
>> command to the Chromecast.
>> +    vlc_mutex_lock(&p_sys->loadCommandMutex);
>> +    const mtime_t deadline = mdate() + 6000 * 1000;
>> +    int i_ret = vlc_cond_timedwait(&p_sys->loadCommandCond,
>> &p_sys->loadCommandMutex, deadline);
>> +    if (i_ret == ETIMEDOUT)
>> +    {
>> +        msg_Err(p_stream, "Timeout reached before sending the media
>> loading command");
>> +        vlc_mutex_unlock(&p_sys->loadCommandMutex);
>> +        Close(p_this);
>> +        return VLC_EGENERIC;
>> +    }
>> +    vlc_mutex_unlock(&p_sys->loadCommandMutex);
>> +
>> +    // Sleep more to let to the Chromecast initiate the connection
>> to the http server.
>> +    sleep(2);
>
> Not thread-safe.

Ok

> And extremely dubious anyway.

This looks dubious to me as well. I explained more the purpose of this 
part in my answer to Ilkka.

>> +/*****************************************************************************
>>
>> + * Close: destroy interface
>> +
>>
>> *****************************************************************************/
>>
>> +static void Close(vlc_object_t *p_this)
>> +{
>> +    sout_stream_t *p_stream = (sout_stream_t *)p_this;
>> +    sout_stream_sys_t *p_sys = p_stream->p_sys;
>> +
>> +    if (p_sys->b_threadStarted)
>> +    {
>> +        vlc_cancel(p_sys->chromecastThread);
>
> glibc supports it but generally cancel is not portably defined for C++
> functions. I believe you need to stick to C linkage and POD at all
> potential cancel points.

Ok

>> +        vlc_join(p_sys->chromecastThread, NULL);
>> +
>> +        vlc_cond_destroy(&p_sys->loadCommandCond);
>> +        vlc_mutex_destroy(&p_sys->loadCommandMutex);
>
> Leaked on false branch.

Ok

>> +vlc_tls_t *chromecast_ClientSessionCreate(vlc_tls_creds_t *crd, int fd,
>> +                                          const char *host, const
>> char *service)
>> +{
>> +    vlc_tls_t *session = vlc_tls_SessionCreate(crd, fd, host);
>> +    if (session == NULL)
>> +        return NULL;
>> +
>> +    mtime_t deadline = mdate();
>> +    deadline += var_InheritInteger(crd, "ipv4-timeout") * 1000;
>> +
>> +    struct pollfd ufd[1];
>> +    ufd[0].fd = fd;
>> +
>> +    int val;
>> +    while ((val = vlc_tls_SessionHandshake(session, host, service)) > 0)
>> +    {
>> +        mtime_t now = mdate ();
>> +        if( now > deadline )
>> +            now = deadline;
>> +
>> +        ufd[0] .events = (val == 1) ? POLLIN : POLLOUT;
>> +
>> +        if (poll(ufd, 1, (deadline - now) / 1000) == 0)
>> +        {
>> +            msg_Err( session, "TLS client session handshake timeout" );
>> +            val = -1;
>> +            break;
>> +        }
>> +    }
>> +
>> +    msg_Warn(session, "TLS client session is NOT trusted (%i)", val);
>> +
>> +    return session;
>> +}
>
> This looks like cut&paste of tls.c...

Yes, I'll check with the initial author of that part for the reasons.


-- 
MagSoft