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

Michael Niedermayer michaelni
Tue Mar 6 03:48:20 CET 2007


Hi

On Tue, Feb 27, 2007 at 02:28:42PM -0600, Ryan Martell wrote:
> Hi---
> 
> On Jan 5, 2007, at 3:56 PM, Ryan Martell wrote:
> >On Jan 5, 2007, at 2:23 PM, Michael Niedermayer wrote:
> >
> >>Hi
> >>
> >>On Fri, Jan 05, 2007 at 11:36:15AM -0600, Ryan Martell wrote:
> >>[...]
> >>>>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....
> >>
> >>see ascii_to_wc() in mov.c
> >
> >Perfect.  Thanks.
> 
> I couldn't use this one, because it's big endian, and I needed little  
> endian.  I have a put_le_utf16 instead.
> 

ok


[...]
> 
> >>[...]
> >>
> >>>>[...]
> >>>>>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.
> >>
> >>hmm, so what about providing the asf demuxer with a ByteIOContext?
> >>a simple fixed size buffer + a read_packet function passed to
> >>init_put_byte() might work ...
> >
> >I think I tried that, but it didn't work for some reason.  But I  
> >think I have learned more since then & can try it again.
> 
> This new solution is much cleaner, it doesn't require any changes to  
> asf.c except making asf_reset_header() public (ff_asf_reset_header()).

great


[...]

> 
> >>>[...]
> >>>>                    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.
> >
> >Again, I don't think so (unless I'm missing something).  This is  
> >telling the server what to send; it has no bearing on the client  
> >(except, of course, that the client can only play what the server  
> >sends)

my idea IIRC (its some time ago when i suggested AVStream.discard ...)
was that the user sets AVStream.discard for all but 1 video and
audio stream and the mms code then requests these from the server
so the server sends just them ...


> >
> >Will work on implementing the other changes over the weekend and  
> >getting a another version up for RFC.
> 
> Here'a another version.  There are lots of internal changes, mainly  
> designed to get it to work as cleanly as possible with asf.c.  This  
> version isn't tested as much as the previous version (I've only  
> tested it on live video streams, not on demand stuff), but it does  
> work for that.
> 
> Let me know what people think....

its big, which makes review hard, if you can split this into
a series of incremental patches it would simplify and shorten review cycles
alot


[...] 
> enum { // client to server packets....

comment is not doxygen compatible


>     CS_PACKET_INITIAL_TYPE= 0x01,
>     CS_PACKET_PROTOCOL_SELECT_TYPE= 0x02,
>     CS_PACKET_MEDIA_FILE_REQUEST_TYPE= 0x05,
>     CS_PACKET_START_FROM_PACKET_ID_TYPE= 0x07,
>     CS_PACKET_STREAM_PAUSE_TYPE= 0x09, // tcp left open, but data stopped.

comment is not doxygen compatible


>     CS_PACKET_STREAM_CLOSE_TYPE= 0x0d,
>     CS_PACKET_MEDIA_HEADER_REQUEST_TYPE= 0x15,
>     CS_PACKET_TIMING_DATA_REQUEST_TYPE= 0x18,
>     CS_PACKET_USER_PASSWORD_TYPE= 0x1a,
>     CS_PACKET_KEEPALIVE_TYPE= 0x1b,
>     CS_PACKET_STREAM_ID_REQUEST_TYPE= 0x33

this would be more readable if things where vertically aligned like:

CS_PACKET_STREAM_CLOSE_TYPE         = 0x0d,
CS_PACKET_MEDIA_HEADER_REQUEST_TYPE = 0x15,
CS_PACKET_TIMING_DATA_REQUEST_TYPE  = 0x18,
CS_PACKET_USER_PASSWORD_TYPE        = 0x1a,
CS_PACKET_KEEPALIVE_TYPE            = 0x1b,
CS_PACKET_STREAM_ID_REQUEST_TYPE    = 0x33

thats of course not really important though, just a suggestion


[...]
> typedef struct { 
>     ASFContext asf_context; ///< asf_context (since this is sorta an AVInputFormat using an AVInputFormat.  Critical that it's the first parameter, as the ASF parser thinks it's it's structure.
> 
>     char local_guid[37]; ///< my randomly generated GUID
>     uint32_t local_ip_address; ///< not ipv6 compatible, but neither is the protocol (sent, but not correct)
>     int local_port; ///< my local port (sent but not correct)
>     int sequence_number; ///< packet sequence number

again vertical alignment makes these look nicer too

char     local_guid[37];    ///< my randomly generated GUID
uint32_t local_ip_address;  ///< not ipv6 compatible, but neither is the protocol (sent, but not correct)
int      local_port;        ///< my local port (sent but not correct) 
int      sequence_number;   ///< packet sequence number 


[...]
> static int mms_probe(AVProbeData *p)
> {
>     if (strstart(p->filename, "mms:", NULL) ||
>         strstart(p->filename, "mmsu:", NULL) ||
>         strstart(p->filename, "mmst:", NULL) || 

trailing whitespace is forbidden in svn


[...]
> static void generate_guid(char *dst)
> {
>     AVRandomState random_state;
>     char digit[]= "0123456789ABCDEF";
>     int ii;
>     
>     av_init_random(1234, &random_state);
>     for(ii= 0; ii<36; ii++) {
>         switch(ii) {
>             case 8:
>             case 13:
>             case 18:
>             case 23:
>                 *dst++= '-';
>                 break;
>                 
>             default:
>                 *dst++= digit[av_random(&random_state)%sizeof(digit)];
>                 break;
>         }
>     }
>     *dst= '\0';

what good does generating the GUID with av_random but a fixed seed do?
you could as well hardcode the GUID


[...]
> static int get_server_response(MMSState *mms)
> {
>     // read the 8 byte header...
>     int read_result;
>     int packet_type= -1;
>     int done;
>     
>     // use url_fdopen & url_fclose...
>     do {
>         done= 1; // assume we're going to get a valid packet.
>         if((read_result= get_buffer(&mms->incoming_io_buffer, mms->incoming_buffer, 8))==8) {
>             // check if we are a command packet...
>             if(AV_RL32(mms->incoming_buffer + 4)==0xb00bface) {
>                 mms->incoming_flags= mms->incoming_buffer[3];
>                 if((read_result= get_buffer(&mms->incoming_io_buffer, mms->incoming_buffer+8, 4)) == 4) {
>                     int length_remaining= AV_RL32(mms->incoming_buffer+8) + 4;

int length_remaining= get_le32(mms->incoming_io_buffer);
the same sugestion applies to all the other get_buffer() + AV_RL*()


[...]
>                 if(mms->media_packet_buffer_length>=sizeof(mms->media_packet_incoming_buffer))
>                {
>                     fprintf(stderr, "Incoming Buffer Length exceeds buffer: %d>%lud\n", mms->media_packet_buffer_length, sizeof(mms->media_packet_incoming_buffer));

(f)printf() is forbidden outside debugging code, please us av_log()
and instead of
#ifdef DEBUG
*printf()
#endif

theres dprintf which will be compiled to nothing if DEBUG isnt #defined



>                }
>                 assert(mms->media_packet_buffer_length<sizeof(mms->media_packet_incoming_buffer));

the indention of {} seems wrong


[...]

>             done= 1;

redundant


>         }
>     } while(!done);
>     
>     return packet_type;
> }
> 
> static int mms_stream_play_from_timestamp(MMSState *mms, int64_t timestamp)
> {
>     double ts= timestamp/1000;

should be /1000.0 to prevent rounding


[...]
>     } else {
> #ifdef DEBUG
>         fprintf(stderr, "Tried a read_play when the state was not stream paused (%d)\n", mms->state);
> #endif
>     }
>     
>     return 0;

conditions which are impossible unless our/your code is buggy, (i dont know
if this applies to the else above) should be checked via assert()

conditions which are possible with some (in)valid input should always be
dealt with, and not be just under #ifdef DEBUG, a return something and printing
of an error message may be appropriate then ...

i see some code under #ifdef DEBUG which shouldnt be, for example the
print "password required" later, of course a more user friendly error message
should be used for such errors like "password protected MMS is not supported"


> }
> 
> static int mms_video_enabled= 1;
> static int mms_audio_enabled= 1;
> static int mms_max_bitrate= 1024*1024;

this is UGLY, what if an application wants to download several mms streams
from different servers with different parameters each ...


[...]
> static int mms_private_close(MMSState *mms)
> {
> #ifdef DEBUG
>     fprintf(stderr, "mms_read_close\n");
> #endif
>     if(mms->mms_hd)
>     {
>         // send the close packet if we should...
>         if(mms->state!=STATE_ERROR)
>         {
>             // send the closedown message...
>             start_command_packet(mms, CS_PACKET_STREAM_CLOSE_TYPE);
>             insert_command_prefixes(mms, 1, 1);
>             send_command_packet(mms);
>         }
>         
>         // need an url_fdclose()
>         if(mms->incoming_io_buffer.buffer)
>         {
>             av_free(mms->incoming_io_buffer.buffer);
>         }

av_free(x) does not need to be protected by if(x)


[...]
> 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= asf_demuxer.read_packet(s, pkt);
>         if(pkt->pts != AV_NOPTS_VALUE && mms->base_timestamp==AV_NOPTS_VALUE)
>         {
>             mms->base_timestamp= pkt->pts;
> //            fprintf(stderr, "Setting base timestamp to %lld!\n", mms->base_timestamp);
>         }
>         
>         // required for live to make ffplay show something sensible; should be done in asf, but looks more complicated there.
>         if(mms->base_timestamp != AV_NOPTS_VALUE)
>         {
> //            fprintf(stderr, "Adjusting timestamp by %lld!\n", mms->base_timestamp);
>             if(pkt->pts != AV_NOPTS_VALUE)
>             {
> //                fprintf(stderr, "Adjusting PTS timestamp from %lld to %lld!\n", pkt->pts, pkt->pts-mms->base_timestamp);
>                 pkt->pts -= mms->base_timestamp;
>             }
>             if(pkt->dts != AV_NOPTS_VALUE)
>             {
> //                 fprintf(stderr, "Adjusting DTS timestamp from %lld to %lld!\n", pkt->dts, pkt->dts-mms->base_timestamp);
>                 pkt->dts -= mms->base_timestamp;
>             }

what is the base_timestamp subtraction good for?


[...]
> static int mms_read_play(AVFormatContext *s)
> {
>     MMSState *mms = s->priv_data;
> #ifdef DEBUG
>     fprintf(stderr, "MMS Read Play\n");
> #endif
>     return mms_stream_play(mms);
> }
> 
> /* pause the stream */
> static int mms_read_pause(AVFormatContext *s)
> {
>     MMSState *mms = s->priv_data;
> #ifdef DEBUG
>     fprintf(stderr, "MMS Read Pause\n");
> #endif
>     return mms_stream_pause(mms);
> }

i dont like such wraper functions much they tend to just make code harder
to understand, so i would suggest to rather make mms_stream_play/pause use
AVFormatContext *s as argument or something else which would avoid
such 2 layer system


[...]
> /* ------------------------------------------------------------------------------------------- */
> /* This code is for parsing asx files that then have mms references in them.  This is done in  */
> /* the same manner as a rtp http redirect.                                                     */
> /* ------------------------------------------------------------------------------------------- */
> static int asx_probe(AVProbeData *p)

comment is not doxygen compatible


[...]
> int asx_open(AVFormatContext **ic_ptr, const char *filename)
> {

needs ff_ prefix or static


>     char buffer[4096];
>     char *dst= buffer;
>     char *src= buffer;
>     char *end;
>     AVFormatContext *ic = NULL;
>     int c;
>     ByteIOContext pb1, *pb = &pb1;
>     const char *asx_start_sequence= "<ASX VERSION=\""; // prolly should be case insensitive..
>     const char *reference_start_sequence= "[Reference]"; // prolly should be case insensitive..
>     
>     if (url_fopen(pb, filename, URL_RDONLY) < 0) {
>         return AVERROR_IO;
>     }
>     
>     do {
>         c = url_fgetc(pb);
>         if(c==URL_EOF) break;
>         *dst++= c;
>     } while((dst-buffer)<sizeof(buffer)-1);

get_buffer()


>     *dst= 0;
>     url_fclose(pb);
> 
>     
>     if(!strcmp(src, asx_start_sequence))
>     {
>         char *match_string= " HREF=\"";
>         
>         while(*src && ((src-buffer)+strlen(match_string))<(dst-buffer))
>         {

does asx_open() work at all?
its code looks pretty broken

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

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20070306/0b7f096e/attachment.pgp>



More information about the ffmpeg-devel mailing list