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

[Michael Niedermayer]

On Mon, Apr 12, 2010 at 12:33:28AM +0800, zhentan feng wrote:
> Hi
> 
> On Fri, Apr 9, 2010 at 2:36 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Thu, Apr 08, 2010 at 09:08:34PM +0800, zhentan feng wrote:
> > > Hi
> > >
> > > On Thu, Apr 8, 2010 at 3:04 AM, Michael Niedermayer <michaelni at gmx.at
> > >wrote:
> > >
> > > > On Thu, Apr 08, 2010 at 01:52:26AM +0800, zhentan feng wrote:
> > > > [...]
> > > > [...]
> > > > > ===================================================================
> > > > > --- mmst.c    (revision 5735)
> > > > > +++ mmst.c    (working copy)
> > > > > @@ -321,6 +321,8 @@
> > > > >                              mms->asf_header =
> > > > av_realloc(mms->asf_header,
> > > > >                                                mms->asf_header_size
> > > > >                                                + mms->pkt_buf_len);
> > > > > +                            if (!mms->asf_header)
> > > > > +                                return -1;
> > > >
> > > > thats probably a memleak and should return ENOMEM
> > > >
> > > >
> > > fixed.
> > >
> > > > [...]
> > > > > @@ -563,10 +563,14 @@
> > > > >      int err = AVERROR(EIO);
> > > > >      int ret;
> > > > >
> > > > > +    h->is_streamed = 1;
> > > > > +    h->priv_data = av_mallocz(sizeof(MMSContext));
> > > > > +    mms = (MMSContext *) h->priv_data;
> > > >
> > > > that cast should be unneeded
> > > >
> > > > fixed.
> > >
> > > I attached the new version patch below.
> > > zhentan
> > > --
> > > Best wishes~
> >
> > [...]
> > > +
> > > +/** 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*


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


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

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20100418/71fff41d/attachment.pgp>