[Ffmpeg-devel] [RFC/Patch] native mms code...
Ryan Martell
rdm4
Tue Mar 6 18:36:49 CET 2007
On Mar 5, 2007, at 8:48 PM, Michael Niedermayer wrote:
> Hi
>
> On Tue, Feb 27, 2007 at 02:28:42PM -0600, Ryan Martell wrote:
>> On Jan 5, 2007, at 3:56 PM, Ryan Martell wrote:
>>> On Jan 5, 2007, at 2:23 PM, Michael Niedermayer wrote:
>>>> On Fri, Jan 05, 2007 at 11:36:15AM -0600, Ryan Martell wrote:
>>>> [...]
> [...]
>
>>
>>>>> [...]
>>>>>> 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 ...
I think that's possible, after some refactoring; I've now added mms
through http tunneling to the code.
To be clear:
the mms_read_header would be called, and would return streams for
everything, and would fill in the bitrates for those streams based on
the header parameters.
then, the application would set AVDiscard on anything that it didn't
want, based on stream types, bitrates, etc.
Finally, on the first call to mms_read_packet, I would negotiate the
streams desired with the server, based on the AVDiscard flag.
Is that what you're thinking?
The only problem with that is that if you ran it in default mode, on
an mms server that had multiple audio streams, I'm not sure what
ffplay.c would do.
>>>
>>> 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
Um... not sure how best to do that; and now it's even larger, as it
has the http code in it as well. ;-)
> [...]
>> enum { // client to server packets....
>
> comment is not doxygen compatible
[..]
Will doxygenize everything and also align as your suggestion. Also I
am now adamant on removing trailing whitespace prior to a final patch
(this was a work in progress update, because I have been getting a
fair number of requests for this code off the newsgroups)
> [...]
>> 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
I know, but I would like that to change. Maybe I can set the seed
based on a hash of the url or something.
> [...]
>> 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*()
I had a reason for not doing that, but I don't recall what it is
now. I'd rather do it the way you suggest.
> [...]
>> 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
[...]
> 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"
Will #ifdef DEBUG all of this prior to final checkin.
Part of this is that I'm working from incomplete specifications, and
it will work fine on a live stream, but then break due to a missing
state transition on a on demand stream, so I have all that stuff in
there for now. I have changed it in the latest code to call a
function that prints out an error message if there is a packet in a
state that wasn't expected, and that is #ifdef DEBUG'd out on final.
>> }
>>
>> 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 ...
Agreed; see note above about AVDiscard.
> [...]
>> 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?
live streams. if you don't do this, the stats on ffplay will show
really odd times. By doing this, it shows the amount of time you've
been watching the stream....
> [...]
>> 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
I'll check this out; I think I did it because mms_stream_pause was
being called from somewhere that I had a MMSState but not an
AVFormatContext. Now the AVFormatContext is also in the MMSState, so
it shouldn't be a problem to change.
> [...]
>> /*
>> ---------------------------------------------------------------------
>> ---------------------- */
>> /* 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
It was working for me on the old server; I have since reworked it and
added the Reference stuff in there & made sure it all worked well.
This code ends up getting called from utils.c, in the same manner as
the rtsp_redirector code, which I don't really like.
Thanks for the review; I'll try and implement all these changes in
the next couple of days and post another version.
-Ryan
More information about the ffmpeg-devel
mailing list