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

[zhentan feng]

Hi

On Fri, Apr 9, 2010 at 2:36 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 ?
>

After checking that I found that :
if use bytestream.h functions in this situation, we don't need the
variable mms->outgoing_packet_data in MMSContext.
So we can write some value into mms->outgoing_packet_buffer directly.
this is more efficiency.

besides, any other the differences between the two methods?


>
>
> [...]
>
> > +static void handle_packet_stream_changing_type(MMSContext *mms)
> > +{
> > +    ByteIOContext pkt;
> > +    dprintf(NULL, "Stream changing!\n");
> > +
> > +    // 40 is the packet header size, without the prefixea.s
> > +    init_put_byte(&pkt, mms->incoming_buffer+40,
> > +            mms->incoming_buffer_length-40, 0, NULL, NULL, NULL, NULL);
> > +    get_le32(&pkt);                                 // prefix 1
> > +    mms->header_packet_id= (get_le32(&pkt) & 0xff); // prefix 2
> > +    dprintf(NULL, "Changed header prefix to 0x%x",
> mms->header_packet_id);
> > +}
>
> thats an interresting way to read 1 byte of an array
>
>
ok, I'll read the byte directly.

>
> [...]
> > +/** 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 > sizeof(mms->incoming_buffer)
> - 12) {
> > +                        dprintf("Incoming message len %d exceeds buffer
> len %d\n",
> > +                            length_remaining,
> sizeof(mms->incoming_buffer) - 12);
> > +                        break;
> > +                    }
>
> doesnt catch negative values
>
>
sizeof(mms->incoming_buffer) - 12 is not negative value definitely.
do you mean length _remaining value? if it is and what will happen?

>
> > +                    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];
> > +                mms->pkt_buf_len          = length_remaining;
> > +                mms->pkt_read_ptr         = mms->incoming_buffer;
> > +
> > +                if (length_remaining > sizeof(mms->incoming_buffer)) {
> > +                    dprintf("Incoming data len %d exceeds buffer len
> %d\n",
> > +                            length_remaining,
> sizeof(mms->incoming_buffer));
> > +                    break;
> > +                }
>
> the length_remaining variables is used before checking it
>
>
> no, the length_reaming is a local variable in else{}, it is different with
the former 'length_remaining'.
maybe I should rename it in case of confusion.

zhentan
-- 
Best wishes~