[FFmpeg-devel] [patch]MMS protocol over TCP

[Ronald S. Bultje]

Hi,

some more minor nitpicks.

On Tue, Mar 23, 2010 at 12:56 PM, zhentan feng <spyfeng at gmail.com> wrote:
Index: libavformat/mmst.c
===================================================================
[..]

In the MMSContext struct:

> +    uint32_t local_ip_address;

This isn't really used. How about we add some useful information here,
or if the server really doesn't care, just make it a #define?

> +    char location[4096];
[..]
> +    // only for MMS over TCP, so set proto = NULL
> +    ff_url_split(NULL, 0, NULL, 0,
> +            mms->host, sizeof(mms->host), &mms->port, mms->path,
> +            sizeof(mms->path), mms->location);

This is only used once, so a variable shouldn't be needed, just make
it an argument to mms_open_cnx(). Saves another 4kB in the struct.

> +    /** Buffer for incoming control packets. */
> +    /*@{*/
> +    uint8_t incoming_buffer[8192];       ///< Incoming buffer location.
> +    int incoming_buffer_length;          ///< Incoming buffer length.
> +    /*@}*/
> +
> +    /** Buffer for incoming media/header packets. */
> +    /*@{*/
> +    uint8_t pkt_buf[8192];               ///< header or media packet.
> +    uint8_t *pkt_read_ptr;               ///< Pointer for partial reads.
> +    int pkt_buf_len;                     ///< Buffer length.
> +    int pkt_offset;                      ///< offset in packet.
> +    /*@}*/

That's two 8kB data buffers, can they be merged? I.e. is it
likely/valid that the server sends two types of data interleaved?

> +static int read_bytes(MMSContext *mms, uint8_t *buffer, int length_to_read)
> +{
> +    int len= 0;
> +
> +    while(len<length_to_read) {
> +        int read_result= url_read(mms->mms_hd, buffer+len, length_to_read-len);
> +        if(read_result < 0)
> +            return read_result;
> +        if(read_result) {
> +            len+= read_result;
> +        } else
> +            return read_result;
> +    }
> +
> +    return len;
> +}

Probably duplicate of url_read_complete().

The patch looks quite good, I hope others like it too.

Ronald