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

Michael Niedermayer michaelni
Fri Jan 5 15:07:40 CET 2007


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


> 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


> 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


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



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


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


> 
> 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(); 

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


>                 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


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


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


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

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

[...]
-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070105/d3cc8c3a/attachment.pgp>



More information about the ffmpeg-devel mailing list