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

[zhentan feng]

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.

> >
> > >
> > >
> > > [...]
> > >
> > > > +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
> > +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
> > +    get_le24(&pkt);                                 // prefix 2
> > +    mms->header_packet_id= get_byte(&pkt);
> > +    dprintf(NULL, "Changed header prefix to 0x%x",
> mms->header_packet_id);
> > +}
>
> same here, you still wrap an array in a ByteIOContext
> and then read a single byte then throw the ByteIOContext away
>
>
fixed.

>
> [...]
> > +typedef struct {
> > +    int sequence_number;                 ///< Outgoing packet sequence
> number.
>
> i would add out to the name, maybe
> outgoing_packet_seq, which nicely matches incoming_packet_seq
>
>
> fixed.

The new version patch attached below.
thanks
zhentan
-- 
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mmst_9.patch
Type: application/octet-stream
Size: 26581 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100418/47f68fec/attachment.obj>