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

Michael Niedermayer michaelni
Mon Apr 19 00:39:53 CEST 2010


On Sun, Apr 18, 2010 at 05:20:32PM +0800, zhentan feng wrote:
> Hi
> 
> On Sun, Apr 18, 2010 at 7:54 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > > > [...]
> > > > > +
> > > > > +/** Add prefixes to MMST command packet. */
> > > > > +static void insert_command_prefixes(MMSContext *mms,
> > > > > +        uint32_t prefix1, uint32_t prefix2)
> > > > > +{
> > > > > +    ByteIOContext *context= &mms->outgoing_packet_data;
> > > > > +
> > > > > +    put_le32(context, prefix1); // first prefix
> > > > > +    put_le32(context, prefix2); // second prefix
> > > > > +}
> > > > > +
> > > > > +/** Send a prepared MMST command packet. */
> > > > > +static int send_command_packet(MMSContext *mms)
> > > > > +{
> > > > > +    ByteIOContext *context= &mms->outgoing_packet_data;
> > > > > +    int exact_length= url_ftell(context);
> > > > > +    int first_length= exact_length - 16;
> > > > > +    int len8= first_length/8;
> > > > > +    int write_result;
> > > > > +
> > > > > +    // update packet length fields.
> > > > > +    url_fseek(context, 8, SEEK_SET);
> > > > > +    put_le32(context, first_length);
> > > > > +    url_fseek(context, 16, SEEK_SET);
> > > > > +    put_le32(context, len8);
> > > > > +    url_fseek(context, 32, SEEK_SET);
> > > > > +    put_le32(context, len8-2);
> > > >
> > > > why dont you just write into the buffer with a uint8_t pointer
> > > > using intreadwrite.h / bytestream.h ?
> > > +/** Send a prepared MMST command packet. */
> > > +static int send_command_packet(MMSContext *mms)
> > > +{
> > > +    ByteIOContext *context= &mms->outgoing_packet_data;
> > > +    uint8_t *p = mms->outgoing_packet_buffer;
> > > +    int exact_length= url_ftell(context);
> > > +    int first_length= exact_length - 16;
> > > +    int len8= first_length/8;
> > > +    int write_result;
> > > +
> > > +    // update packet length fields.
> > > +    AV_WL32(p + 8, first_length);
> > > +    AV_WL32(p + 16, len8);
> > > +    AV_WL32(p + 32, len8-2);
> > > +
> > > +    // write it out.
> > > +    write_result= url_write(mms->mms_hd, context->buffer, exact_length);
> > > +    if(write_result != exact_length) {
> > > +        dprintf(NULL, "url_write returned: %d != %d\n",
> > > +                write_result, exact_length);
> > > +        return AVERROR_IO;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> >
> > this issue is still not corrected, why do you wrap a simple array
> > in a ByteIOContext?
> > I had not meant this single function but all uses of outgoing_packet_data
> > If there is some reason why this is done you should explain it otherwise
> > (and i think thats the case) it should be done through the bytestream API
> > or AV_WL*
> >
> >
> >
> I use bytestream_put_*() functions instead of useless wrapping.
> however, I have to wrap it by ByteIOContext in function mms_put_utf16() in
> order to reuse the common function in asf.c.

ok, understood

[...]
> +typedef struct {
> +    int outgoing_packet_seq;            ///< Outgoing packet sequence number.
> +    char path[256];                      ///< Path of the resource being asked for.
> +    char host[128];                      ///< Host of the resources.
> +
> +    URLContext *mms_hd;                  ///< TCP connection handle
> +    MMSStream streams[MAX_STREAMS];
> +
> +    /** Buffer for outgoing packets. */
> +    /*@{*/
> +    uint8_t *write_ptr;
> +    uint8_t outgoing_packet_buffer[512]; ///< Outgoing packet data
> +    /*@}*/
> +
> +    /** 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_read_ptr;               ///< Pointer for partial reads.
> +    int pkt_buf_len;                     ///< Buffer length.
> +    int pkt_offset;                      ///< offset in packet.
> +    /*@}*/

the doxy is quite terse. Which makes it hard to understand what these
fields represent exactly. One would have to look at how they are use to
understand what they are

also nothing ever sets pkt_offset to a non zero value thus the variable
is uneeded

[...]

> +}
> +
> +/** Read incoming MMST media, header or command packet. */
> +static MMSSCPacketType get_tcp_server_response(MMSContext *mms)
> +{
> +    int read_result;
> +    MMSSCPacketType packet_type= -1;
> +    int done;
> +
> +    do {
> +        done= 1;
> +        if((read_result= url_read_complete(mms->mms_hd, mms->incoming_buffer, 8))==8) {
> +            // handle command packet.
> +            if(AV_RL32(mms->incoming_buffer + 4)==0xb00bface) {
> +                mms->incoming_flags= mms->incoming_buffer[3];
> +                read_result= url_read_complete(mms->mms_hd, mms->incoming_buffer+8, 4);
> +                if(read_result == 4) {
> +                    int length_remaining= AV_RL32(mms->incoming_buffer+8) + 4;
> +
> +                    dprintf(NULL, "Length remaining is %d\n", length_remaining);
> +                    // read the rest of the packet.
> +                    if (length_remaining < 0
> +                        || length_remaining > sizeof(mms->incoming_buffer) - 12) {
> +                        dprintf("Incoming message len %d exceeds buffer len %d\n",
> +                            length_remaining, sizeof(mms->incoming_buffer) - 12);
> +                        break;
> +                    }
> +                    read_result = url_read_complete(mms->mms_hd, mms->incoming_buffer + 12,
> +                                                  length_remaining) ;
> +                    if (read_result == length_remaining) {
> +                        mms->incoming_buffer_length= length_remaining+12;
> +                        packet_type= AV_RL16(mms->incoming_buffer+36);
> +
> +                    } else {
> +                        dprintf(NULL, "3 read returned %d!\n", read_result);
> +                    }
> +                } else {
> +                    dprintf(NULL, "2 read returned %d!\n", read_result);
> +                }
> +            } else {
> +                int length_remaining;
> +                int packet_id_type;
> +                int tmp;
> +
> +                assert(mms->pkt_buf_len==0);
> +
> +                //** VERIFY LENGTH REMAINING HAS SPACE
> +                // note we cache the first 8 bytes,
> +                // then fill up the buffer with the others
> +                tmp                       = AV_RL16(mms->incoming_buffer + 6);
> +                length_remaining          = (tmp - 8) & 0xffff;
> +                mms->incoming_packet_seq  = AV_RL32(mms->incoming_buffer);
> +                packet_id_type            = mms->incoming_buffer[4];
> +                mms->incoming_flags       = mms->incoming_buffer[5];
> +
> +                if (length_remaining < 0
> +                        || length_remaining > sizeof(mms->incoming_buffer)) {
> +                    dprintf("Incoming data len %d exceeds buffer len %d\n",
> +                            length_remaining, sizeof(mms->incoming_buffer));
> +                    break;
> +                }
> +                mms->pkt_buf_len          = length_remaining;
> +                mms->pkt_read_ptr         = mms->incoming_buffer;
> +                read_result= url_read_complete(mms->mms_hd, mms->incoming_buffer, length_remaining);
> +                if(read_result != length_remaining) {
> +                    dprintf(NULL, "read_bytes result: %d asking for %d\n",
> +                            read_result, length_remaining);
> +                    break;
> +                } else {
> +                    // if we successfully read everything.
> +                    if(packet_id_type == mms->header_packet_id) {
> +                        packet_type = SC_PKT_ASF_HEADER;
> +                        // Store the asf header
> +                        if(!mms->header_parsed) {
> +                            void *p = av_realloc(mms->asf_header,
> +                                              mms->asf_header_size
> +                                              + mms->pkt_buf_len);
> +                            if (!p) {
> +                                av_freep(&mms->asf_header);
> +                                return AVERROR(ENOMEM);
> +                            }
> +                            mms->asf_header = p;
> +                            memcpy(mms->asf_header + mms->asf_header_size,
> +                                                 mms->pkt_read_ptr,
> +                                                 mms->pkt_buf_len);
> +                            mms->asf_header_size += mms->pkt_buf_len;
> +                        }
> +                    } else if(packet_id_type == mms->packet_id) {
> +                        packet_type = SC_PKT_ASF_MEDIA;
> +                    } else {
> +                        dprintf(NULL, "packet id type %d is old.", packet_id_type);
> +                        done= 0;
> +                    }
> +                }
> +            }
> +        } else {
> +            if(read_result<0) {
> +                dprintf(NULL, "Read error (or cancelled) returned %d!\n", read_result);
> +                packet_type = SC_PKT_CANCEL;
> +            } else {
> +                dprintf(NULL, "Read result of zero?!\n");
> +                packet_type = SC_PKT_NO_DATA;
> +            }
> +            done = 1;
> +        }
> +    } while(!done);

done can be replaced by break; and continue; which are clearer to understand


[...]
> +static int asf_header_parser(MMSContext *mms)
> +{
> +    uint8_t *p = mms->asf_header, *end = mms->asf_header + mms->asf_header_size;
> +    int flags, stream_id;
> +    mms->stream_num = 0;
> +
> +    if (mms->asf_header_size < sizeof(ff_asf_guid) * 2 + 22 ||
> +        memcmp(p, ff_asf_header, sizeof(ff_asf_guid)))
> +        return -1;
> +
> +    p += sizeof(ff_asf_guid) + 14;
> +    do {

> +        uint64_t chunksize = AV_RL64(p + sizeof(ff_asf_guid));
> +        if (!memcmp(p, ff_asf_file_header, sizeof(ff_asf_guid))) {
> +            /* read packet size */
> +            if (end - p > sizeof(ff_asf_guid) * 2 + 68) {

> +                mms->asf_packet_len = AV_RL32(p + sizeof(ff_asf_guid) * 2 + 64);

this looks unsafe, there is code that writes based on asf_packet_len


> +            }
> +        } else if (!memcmp(p, ff_asf_stream_header, sizeof(ff_asf_guid))) {
> +            flags = AV_RL16(p + sizeof(ff_asf_guid)*3 + 24);
> +            stream_id = flags & 0x7F;
> +            if (mms->stream_num < MAX_STREAMS) {
> +                mms->streams[mms->stream_num].id = stream_id;
> +                mms->stream_num++;
> +            } else
> +                dprintf("Too many streams.\n");
> +        }
> +        if (chunksize > end - p)
> +            return -1;
> +        p += chunksize;
> +    } while (end - p >= sizeof(ff_asf_guid) + 8);
> +
> +    return 0;
> +}
> +

> +/** Send MMST stream selection command based on the AVStream->discard values. */
> +static int send_stream_selection_request(MMSContext *mms)
> +{
> +    int ii;

why not i ?


> +
> +    //  send the streams we want back...
> +    start_command_packet(mms, CS_PKT_STREAM_ID_REQUEST);
> +    bytestream_put_le32(&mms->write_ptr, mms->stream_num);         // stream nums
> +    for(ii= 0; ii<mms->stream_num; ii++) {
> +        bytestream_put_le16(&mms->write_ptr, 0xffff);              // flags
> +        bytestream_put_le16(&mms->write_ptr, mms->streams[ii].id); // stream id
> +        bytestream_put_le16(&mms->write_ptr, 0);                   // selection
> +    }

buffer overflow


> +
> +    bytestream_put_le16(&mms->write_ptr, 0);
> +
> +    return send_command_packet(mms);
> +}
> +
> +static void read_data(MMSContext *mms, uint8_t *buf, const int buf_size, int* result)
> +{
> +    int read_size;
> +    read_size = FFMIN(buf_size, mms->pkt_buf_len);
> +    memcpy(buf, mms->pkt_read_ptr, read_size);
> +    mms->pkt_buf_len -= read_size;
> +    mms->pkt_read_ptr+= read_size;
> +    *result += read_size;
> +}

It would be cleaner to return "result" normally by return

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100419/f50c0e89/attachment.pgp>



More information about the ffmpeg-devel mailing list