[Ffmpeg-devel] [RFC/Patch] native mms code...

Ryan Martell rdm4
Fri Jan 5 18:36:15 CET 2007


Thanks for the review...

On Jan 5, 2007, at 8:07 AM, Michael Niedermayer wrote:

> Hi
>
> On Tue, Jan 02, 2007 at 04:51:07PM -0600, Ryan Martell wrote:
>> Hi--
>>
>> Attached is a native mms implementation.  I did this from the ground
>> up working off the specification, with the exception of borrowing the
>> packet output debug routine from libmms, which I will probably remove
>> before final submission.  (I was using the libmms debug output stuff
>> against my stuff to find my bugs)
>>
>> This is a rough draft only; I'm sort of interested in what people
>> think about this method of implementation.
>>
>> Things I know about:
>> 1) fprintfs must go
>> 2) there may be trailing whitespace, etc.
>>
>> Key questions:
>> 1) is the interface to asf.[ch] satisfactory?
>
> iam not entirely happy with it but i expected the initial version  
> to look
> worse

Is that a backhanded compliment? ;-)

>> 2) Using a URLProtocol instead was an option, but it doesn't support
>> streaming by timestamp.  So instead I mirrored rtp stuff.
>
> iam fine with a mms AVInputFormat but the way the mms and asf demuxers
> are connected together is somewhat ugly

I agree; I was trying to make it as clean as possible, but it's not  
easy.  Obviously I don't want to duplicate the asf code.

>> 3) How can I get the local ip and port from a url_XXX tcp
>> connection?  It doesn't seem currently available in.  Is this  
>> correct?
>
>> 4) MMS has parameters for the tcp connection bitrate; and if there
>> are multiple encodings in the file, it will choose the best ones.  I
>> am currently only streaming the first audio and first video stream.
>> How would i get the bandwidth input from the user?  I know we don't
>> want to add new AVOptions that aren't globally useful.  Also, I could
>> ask for audio only in this manor.
>
> see AVStream.discard

Will look into it.

> [...]
>> @@ -567,7 +571,9 @@
>>          if (asf->packet_size_left < FRAME_HEADER_SIZE
>>              || asf->packet_segments < 1) {
>>              //asf->packet_size_left <= asf->packet_padsize) {
>> -            int ret = asf->packet_size_left + asf->packet_padsize;
>> +            int ret;
>> +            if(!load_packet) {
>> +            ret = asf->packet_size_left + asf->packet_padsize;
>>              //printf("PacketLeftSize:%d  Pad:%d Pos:%"PRId64"\n",  
>> asf->packet_size_left, asf->packet_padsize, url_ftell(pb));
>>              if((url_ftell(&s->pb) + ret - s->data_offset) % asf- 
>> >packet_size)
>>                  ret += asf->packet_size - ((url_ftell(&s->pb) +  
>> ret - s->data_offset) % asf->packet_size);
>> @@ -578,10 +584,18 @@
>>              if (asf->data_object_size != (uint64_t)-1 &&
>>                  (asf->packet_pos - asf->data_object_offset >= asf- 
>> >data_object_size))
>>                  return AVERROR_IO; /* Do not exceed the size of  
>> the data object */
>> -            ret = asf_get_packet(s);
>> +            ret = asf_get_packet(s, asf, pb);
>>              //printf("READ ASF PACKET  %d   r:%d   c:%d\n", ret,  
>> asf->packet_size_left, pc++);
>>              if (ret < 0 || url_feof(pb))
>>                  return AVERROR_IO;
>> +            } else {
>> +                asf->packet_pos= asf->data_object_offset + asf- 
>> >packet_seq * asf->packet_size;
>> +                assert(load_packet);
>> +                ret= load_packet(opaque, pb);
>> +                if(ret<0 || url_feof(pb)) return AVERROR_IO;
>> +                ret = asf_get_packet(s, asf, pb);
>> +                if (ret < 0 || url_feof(pb)) return AVERROR_IO;
>> +            }
>
> the last 2 lines are common in both cases and can be put outside of  
> the
> if/else

Will fix.

> [...]
>> enum {
>>     DEFAULT_MMS_PORT= 1755,
>>
>>     // client to server
>>     CS_PACKET_INITIAL_TYPE= 0x01,
>>     CS_PACKET_TIMING_DATA_REQUEST_TYPE= 0x18,
>>     CS_PACKET_PROTOCOL_SELECT_TYPE= 0x02,
>>     CS_PACKET_MEDIA_FILE_REQUEST_TYPE= 0x05,
>>     CS_PACKET_USER_PASSWORD_TYPE= 0x1a,
>>     CS_PACKET_MEDIA_HEADER_REQUEST_TYPE= 0x15,
>>     CS_PACKET_STREAM_ID_REQUEST_TYPE= 0x33,
>>     CS_PACKET_START_FROM_PACKET_ID_TYPE= 0x07,
>>     CS_PACKET_STREAM_PAUSE_TYPE= 0x09, // tcp left open, but data  
>> stopped.
>>     CS_PACKET_STREAM_CLOSE_TYPE= 0x0d,
>>     CS_PACKET_KEEPALIVE_TYPE= 0x1b,
>>
>>     // server to client
>>     SC_PACKET_CLIENT_ACCEPTED= 0x01,
>>     SC_PACKET_TIMING_TEST_REPLY_TYPE= 0x15,
>>     SC_PACKET_PROTOCOL_ACCEPTED_TYPE= 0x02,
>>     SC_PACKET_PROTOCOL_FAILED_TYPE= 0x03,
>>     SC_PACKET_PASSWORD_REQUIRED_TYPE= 0x1a,
>>     SC_PACKET_MEDIA_FILE_DETAILS_TYPE= 0x06,
>>     SC_PACKET_HEADER_REQUEST_ACCEPTED_TYPE= 0x11,
>>     SC_PACKET_STREAM_ID_ACCEPTED_TYPE= 0x21,
>>     SC_PACKET_MEDIA_PACKET_FOLLOWS_TYPE= 0x05,
>>     SC_PACKET_STREAM_STOPPED_TYPE= 0x1e,
>>     SC_PACKET_KEEPALIVE_TYPE= 0x1b,
>>
>>     // my psudeo packet types
>>     SC_PACKET_ASF_HEADER_TYPE= 0x81,
>>     SC_PACKET_ASF_MEDIA_TYPE= 0x82,
>>
>>     // state machine states....
>>     AWAITING_SC_PACKET_CLIENT_ACCEPTED= 0,
>>     AWAITING_SC_PACKET_TIMING_TEST_REPLY_TYPE,
>>     AWAITING_CS_PACKET_PROTOCOL_ACCEPTANCE,
>>     AWAITING_PASSWORD_QUERY_OR_MEDIA_FILE,
>>     AWAITING_PACKET_HEADER_REQUEST_ACCEPTED_TYPE,
>>     AWAITING_STREAM_ID_ACCEPTANCE,
>>     AWAITING_STREAM_START_PACKET,
>>     AWAITING_ASF_HEADER,
>>     AWAITING_PAUSE_ACKNOWLEDGE,
>>     STREAMING,
>>     STREAM_DONE,
>>     STATE_ERROR,
>>     STREAM_PAUSED,
>>     USER_CANCELLED,
>>
>>     MAXIMUM_PACKET_LENGTH= 512,
>>     KILO= 1024
>
> not all things above have the same "type" as such they dont belong  
> into the
> same enum

Okay; I'll break them up into separate ones.

> [...]
>> static int convert_to_unicode(char *dest, int max_length, const  
>> char *src)
>> {
>>     char *dst= dest;
>>
>>     while(*src && dst-dest<max_length-2) {
>>         *dst++= *src++;
>>         *dst++= 0;
>>     }
>>     *dst++= 0;
>>     *dst++= 0;
>>
>>     return dst-dest;
>> }
>
> all strings in ffmpeg are supposed to be utf-8 so the name  
> convert_to_unicode
> is wrong, it rather should be something like convert_utf8_to_utf16  
> and should
> use GET_UTF8() (unless you know that the input will be ASCII)

Is there an example of unicode conversion in the code base?  I really  
didn't know how to do this (or even if there was some library  
function), so I just punted and did it a simple way.  I didn't want  
to get hung up on the conversion....

>>
>> static void generate_guid(char *dst)
>> {
>>     char digit[]= "0123456789ABCDEF";
>>     int ii;
>>
>> //    srandom(time(NULL));
>>     for(ii= 0; ii<36; ii++)
>>     {
>>         switch(ii)
>>         {
>>             case 8:
>>             case 13:
>>             case 18:
>>             case 23:
>>                 *dst++= '-';
>>                 break;
>>
>>             default:
>>                 *dst++= digit[random()%(sizeof(digit)/sizeof(digit 
>> [0]))];
>
> random() must not be used in libraries as it changes the user  
> applications
> state, think of:
>
> thread1:
> srandom(123);
> A=random();
>
> thread2:
> mms_demux();

The networking model in some of the games I worked on used  
deterministic random to keep all the machines in sync, so I'm  
definitely aware of the problem.  Should I roll my own random, or  
maybe add an internal random to ffmpeg?

> also sizeof(char) == 1
>
>
> [...]
>> static int get_server_response(MMSState *mms)
>> {
>>     // read the 8 byte header...
>>     int read_result;
>>     int packet_type= -1;
>>     int done;
>>
>>     do {
>>         done= 1; // assume we're going to get a valid packet.
>>         if((read_result= read_bytes(mms, mms->incoming_buffer, 8)) 
>> ==8)
>>         {
>>             // check if we are a command packet...
>>             if(LE_32(mms->incoming_buffer + 4)==0xb00bface) {
>
> this is ugly
>
> (url_fdopen(xyz, mms->mms_hd);)
> get_le32(xyz)
> get_le32(xyz)
>
> or similar is much cleaner

Will check that out; it sounds better.

>>                 mms->incoming_flags= mms->incoming_buffer[3];
>>                 if((read_result= read_bytes(mms, mms- 
>> >incoming_buffer+8, 4)) == 4) {
>>                     int length_remaining= LE_32(mms- 
>> >incoming_buffer+8) + 4;
>>
>>                     // read the rest of the packet....
>>                     read_result = read_bytes(mms, mms- 
>> >incoming_buffer + 12, length_remaining) ;
>
> incoming_buffer is 4kb, length_remaining can be arbitrary, this is a
> security hole
>
> [...]
>>             } else {
>>                 int length_remaining= (LE_16(mms->incoming_buffer  
>> + 6) - 8) & 0xffff;
>>                 uint8_t *dst= mms->incoming_buffer;
>>                 int packet_id_type;
>>
>>                 // note we cache the first 8 bytes, then fill up  
>> the buffer with the others
>>                 mms->incoming_packet_seq     = LE_32(mms- 
>> >incoming_buffer);
>>                 packet_id_type = mms->incoming_buffer[4]; // NOTE:  
>> THIS IS THE ONE I CAN CHANGE
>>                 mms->incoming_flags          = mms->incoming_buffer 
>> [5];
>>                 mms->incoming_buffer_length  = length_remaining;
>>
>>                 assert(mms->incoming_buffer_length<sizeof(mms- 
>> >incoming_buffer));
>
> assert() is not appropriate to check the validity of the input
> a library must not kill the application if the input data is  
> invalid, as
> example think of your 5 open firefox browser windows closing instantly
> after a single server returned an invaid http awnser

Will fix; more robust error handling should have been on one of the  
things on the Things I know about list.


> [...]
>> static int mms_stream_play_from_timestamp(MMSState *mms, int64_t  
>> timestamp)
>> {
>>     double ts= timestamp/1000;
>>
>>     fprintf(stderr, "mms_stream_play: %lld TS: %lf\n", timestamp,  
>> ts);
>>     if(mms->state==STREAM_PAUSED)
>>     {
>>         timestamp= *((int64_t *)&ts);
>
> completely non portable, see libavutil/intfloat_readwrite.*

Thanks for the reference; I knew when I wrote that it was unportable,  
but didn't know where to look.  I did it this way just to check and  
make sure it would work, and it does.

> [...]
>>                     if((mms->incoming_flags == 0X08) || (mms- 
>> >incoming_flags == 0X0C))
>>                     {
>>                         // we are done with the header stuff.
>>                         ByteIOContext byte_context;
>>
>>                         fprintf(stderr, "Got the full header! (%d  
>> bytes)\n", mms->asf_header_length);
>>                         // parse the header....
>>                         init_put_byte(&byte_context, mms- 
>> >asf_header, mms->asf_header_length, 0, NULL, NULL, NULL, NULL);
>>                         if(ff_asf_read_header(mms->av_format_ctx,  
>> &mms->asf_context, &byte_context)==0)
>>                         {
>>                             int stream_count= 2;
>>                             int ii;
>>
>>                             //  send the streams we want back...
>>                             put_flush_packet(&mms->packet_data);
>>                             put_le32(&mms->packet_data,  
>> stream_count);
>>                             for(ii= 0; ii<stream_count; ii++)
>>                             {
>>                                 put_le16(&mms->packet_data,  
>> 0xffff); // flags
>>                                 put_le16(&mms->packet_data, ii 
>> +1); // stream id
>>                                 /*
>>                                  00 = stream at full frame rate;
>>                                  01 = only stream key frames;
>>                                  02 = no stream, switch it off.
>>                                  */
>>                                 put_le16(&mms->packet_data,  
>> 00); // stream at full frame rate
>
> AVStream.discard could be used here maybe?

Not familiar with that; will check it out.

> [...]
>> static int mms_read_packet(AVFormatContext *s,
>>                             AVPacket *pkt)
>> {
>>     MMSState *mms = s->priv_data;
>>     int result= 0;
>>
>> //    fprintf(stderr, "mms_read_packet!\n");
>>
>>     if(mms->state==STREAMING || mms->state==STREAM_PAUSED || mms- 
>> >state==AWAITING_STREAM_START_PACKET)
>>     {
>> //        result= ff_asf_read_packet(s, pkt, &mms->asf_context,  
>> &mms->current_media_packet_context, load_packet, mms);
>> //        result= ff_asf_read_packet(s, pkt, &mms->asf_context,  
>> &mms->av_format_ctx->pb, load_packet, mms);
>>         result= ff_asf_read_packet_with_load_proc(s, pkt,  
>> load_packet, mms);
>
> isnt it possible that the mms AVInputFormat provides the asf  
> AVInputFormat
> with a valid asf stream instead of this callback to get the next  
> packet?

I was trying to do that initially, but i ran into lots and lots of  
url_ftell's in the asf code, that were causing it to error out.  by  
putting the packet load as a separate call, i could make sure the  
ftell's would be valid, and the code would continue.  The asf code is  
really pretty dependent on it being file based.

But, I'm not the expert on working with this code base; if anyone can  
give me any suggestions on how to further separate these, I'd  
appreciate them.  I'm not exactly sure what you mean by providing the  
asf AVInputFormat with a valid asf stream; all that the asf stuff  
does is read from the ByteIOContext.

> basically i think the mms and asf demuxers are too entangled ..

Also, I was trying to put the ASFContext as the first parameter of  
the MMSContext, and then passing it on to the ASF code without having  
to change anything.  That worked for several of the calls, but not  
the header parsing (because the byte context wasn't being read right-  
i don't remember why), and of course I had to have the packet  
function for loading the next packet.

> [...]
> -- 
> Michael     GnuPG fingerprint:  
> 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No human being will ever know the Truth, for even if they happen to  
> say it
> by chance, they would not even known they had done so. -- Xenophanes
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel





More information about the ffmpeg-devel mailing list