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

[zhentan feng]

Hi

On Wed, Mar 24, 2010 at 1:28 AM, Ronald S. Bultje <rsbultje at gmail.com>wrote:

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

modified the code according to all reviews.
see the patch version 2.

zhentan
-- 
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mmst_2.patch
Type: application/octet-stream
Size: 25959 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100325/bd2b1fca/attachment.obj>