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

[Adrien Maglo]

Le 11/09/2014 20:28, 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_out(NULL)
>> +    {
>> +        atomic_init(&ab_error, false);
>> +    }
>> +
>> +    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;
>
> I think we've already been through this.

Yes, it is no longer needed but I forgot to totally remove it.

>> +    bool b_pingSent;
>> +    atomic_bool ab_error;
>
> Dubious. What if the error is flagged after the other thread checks the flag?

This flag is just here so that Send() can return VLC_EGENERIC when there 
is a Chromecast control problem.
If the error is flagged after the check in Send(), then the buffer is 
sent to the rest of the sout chain and the chain will halt with the next 
Send() call. I guess this should not be a problem since in that case the 
sout chain is still correctly functioning.

>> +    add_string(SOUT_CFG_PREFIX "receiver-ip", "", RECEIVER_IP_TEXT,
>> RECEIVER_IP_LONGTEXT, false)
>> +    add_string(SOUT_CFG_PREFIX "sender-ip",
>> "", SENDER_IP_TEXT, SENDER_IP_LONGTEXT, false)
>
> I can see the point in setting the receiver, but why is the sender not
> automatically selected by the INET stack?

I will fix that.

>> +
>> add_integer(SOUT_CFG_PREFIX "sender-port", SENDER_SOUT_PORT,
>> SENDERPORT_TEXT, NULL, false)
>
> Ditto.

Well here, it's the http sout port. Maybe the name of the variable 
should be changed...

>> +static int connectChromecast(sout_stream_t *p_stream, char
>> *psz_ipChromecast)
>> +{
>> +    sout_stream_sys_t *p_sys = p_stream->p_sys;
>> +    int fd = net_ConnectTCP(p_stream, psz_ipChromecast,
>> CHROMECAST_CONTROL_PORT);
>> +    if (fd < 0)
>> +        return -1;
>> +
>> +    p_sys->p_creds = vlc_tls_ClientCreate(VLC_OBJECT(p_stream));
>> +    if (p_sys->p_creds == NULL)
>> +        return -1;
>> +
>> +    p_sys->p_tls = vlc_tls_ClientSessionCreate(p_sys->p_creds, fd,
>> psz_ipChromecast,
>> +                                               "tcps", NULL, NULL);
>
> What is tcps?

Should it be changed to something like "chromecast"?

>> +static int recvPacket(sout_stream_t *p_stream, bool &b_msgReceived,
>> +                      uint32_t &i_payloadSize)
>> +{
>> +    // Use here only C linkage and POD types as this function is a
>> cancelation point.
>
> The comment does not match the code, or vice versa.
>
>> +    sout_stream_sys_t *p_sys = p_stream->p_sys;
>> +    struct pollfd ufd[1];
>> +    ufd[0].fd = p_sys->i_sock_fd;
>> +    ufd[0].events = POLLIN;
>> +
>> +    /* The Chromecast normally sends a PING command every 5 seconds or so.
>> +     * If we do not receive one after 6 seconds, we send a PING.
>> +     * If after this PING, we do not receive a PONG, then we consider the
>> +     * connection as dead. */
>> +    if (poll(ufd, 1, 6000) == 0)
>> +    {
>> +        if (p_sys->b_pingSent)
>> +        {
>> +            msg_Err(p_stream, "No PONG answer received from the
>> Chromecast");
>> +            return 0; // Connection died
>> +        }
>> +        msg_Warn(p_stream, "No PING received from the Chromecast, sending a
>> PING");
>> +        msgPing(p_stream);
>> +        p_sys->b_pingSent = true;
>> +        return -1;
>> +    }
>> +
>> +    p_sys->b_pingSent = false;
>> +    char *p_data = p_sys->p_packet;
>> +    int i_ret;
>> +
>> +    /* Packet structure:
>> +     *
>> +------------------------------------+------------------------------+ +
>> * | Payload size (uint32_t big endian) |         Payload data         | +
>>    * +------------------------------------+------------------------------+
>> */ +    if (p_sys->i_received < PACKET_HEADER_LEN)
>> +    {
>> +        // We receive the header.
>> +        i_ret = tls_Recv(p_sys->p_tls, p_data, PACKET_HEADER_LEN -
>> p_sys->i_received);
>> +        if (i_ret <= 0)
>> +            return i_ret;
>> +        p_sys->i_received += i_ret;
>> +    }
>> +    else
>> +    {
>> +        // We receive the payload.
>> +
>> +        // Get the size of the payload
>> +        memcpy(&i_payloadSize, p_data, PACKET_HEADER_LEN);
>> +        i_payloadSize = hton32(i_payloadSize);
>> +        const uint32_t i_maxPayloadSize = PACKET_MAX_LEN -
>> PACKET_HEADER_LEN; +
>> +        if (i_payloadSize > i_maxPayloadSize)
>> +        {
>> +            // Error case: the packet sent by the Chromecast is too long:
>> we drop it.
>> +            msg_Err(p_stream, "Packet too long: droping its
>> data");
>> +
>> +            uint32_t i_size = i_payloadSize - (p_sys->i_received -
>> PACKET_HEADER_LEN);
>> +            if (i_size > i_maxPayloadSize)
>> +                i_size = i_maxPayloadSize;
>> +
>> +            i_ret = tls_Recv(p_sys->p_tls, p_data + PACKET_HEADER_LEN,
>> i_size);
>> +            if (i_ret <= 0)
>> +                return i_ret;
>> +            p_sys->i_received += i_ret;
>> +
>> +            if (p_sys->i_received < i_payloadSize + PACKET_HEADER_LEN)
>> +                return i_ret;
>> +
>> +            p_sys->i_received = 0;
>> +            return -1;
>> +        }
>> +
>> +        // Normal case
>> +        i_ret = tls_Recv(p_sys->p_tls, p_data + p_sys->i_received,
>> +                         i_payloadSize - (p_sys->i_received -
>> PACKET_HEADER_LEN));
>> +        if (i_ret <= 0)
>> +            return i_ret;
>> +        p_sys->i_received += i_ret;
>> +
>> +        if (p_sys->i_received < i_payloadSize + PACKET_HEADER_LEN)
>> +            return i_ret;
>> +
>> +        assert(p_sys->i_received == i_payloadSize + PACKET_HEADER_LEN);
>> +        p_sys->i_received = 0;
>> +        b_msgReceived = true;
>> +        return i_ret;
>> +    }
>> +
>> +    return i_ret;
>> +}

The cancellation-safeness problem is only caused by the msgPing() call 
or I should not access as well to sout_stream_sys_t members?

Thanks,


-- 
MagSoft