[FFmpeg-devel] [PATCH] MMS protocol support patch 1
Björn Axelsson
bjorn.axelsson
Mon Oct 15 17:58:44 CEST 2007
On Sat, 2007-10-13 at 02:37 +0200, Michael Niedermayer wrote:
> On Thu, Oct 11, 2007 at 02:59:00PM +0200, Bj?rn Axelsson wrote:
> > On Wed, 2007-10-10 at 04:09 +0200, Michael Niedermayer wrote:
> > > Hi
> > >
> > > On Tue, Oct 09, 2007 at 02:00:25PM +0200, Bj?rn Axelsson wrote:
> > > [...]
> > > > I have also fixed the issues mentioned by Diego earlier in this thread
> > > > (some trailing whitespace and many long lines).
> > > >
> > > > Attached patches:
> > > > 1. aviobuf_resetbuf.diff: add functionality to set the direction of a
> > > > ByteIOContext buffer.
> > > > 2. mms_protocol_base.diff: basic mmsh protocol support
> > > > 3. mms_seek_hack.diff: Add MMS seek and pause support to the ASF
> > > > demuxer.
> > > >
> > > > Thanks to Neil Brown for suggesting using quilt for patch management,
> > > > and to Michael for taking his time for the review.
> > > [...]
> > >
> > > (1. aviobuf_resetbuf.diff)
> > >
> > > patch ok
> >
> > Yay!
>
> (many) small patches reach svn quicker in general than big ones ...
> -> spliting is always a good idea ... (just see the PAFF patch)
>
>
> [...]
> > Index: libavformat/asf.c
> > ===================================================================
> > --- libavformat/asf.c.orig 2007-10-09 10:38:02.000000000 +0200
> > +++ libavformat/asf.c 2007-10-11 14:43:32.403185962 +0200
> > @@ -24,6 +24,8 @@
> > #include "asf.h"
> > #include "common.h"
> >
> > +extern void ff_mms_set_stream_selection(URLContext *h, AVFormatContext *format);
> > +
> > #undef NDEBUG
> > #include <assert.h>
> >
> > @@ -105,6 +107,16 @@
> > }
> > #endif
> >
> > +#ifdef CONFIG_MMSH_PROTOCOL
> > +static int is_mms(ByteIOContext *pb)
> > +{
> > + return url_fileno(pb) && url_fileno(pb)->prot &&
> > + !strcmp(url_fileno(pb)->prot->name, "mmsh");
> > +}
> > +#else
> > +#define is_mms(x) 0
> > +#endif
> > +
> > static void get_str16_nolen(ByteIOContext *pb, int len, char *buf, int buf_size)
> > {
> > char* q = buf;
> > @@ -532,6 +544,10 @@
> > }
> > }
> >
> > + /* Give info about ourselves to the mms protocol */
> > + if(is_mms(pb))
> > + ff_mms_set_stream_selection(url_fileno(pb), s);
> > +
> > return 0;
> >
> > fail:
>
> the above is ok (if it where a seperate patch in a seperate email :)
Point taken. I'll keep it in mind for future patches. However, the part
above doesn't compile without the rest. That is an error this patch
should fix.
In the general case, is it ok to separate patches that either don't
compile separately or don't work separately?
> [...]
>
> > +MMSProtocol mmsh_mmsprotocol = {
>
> ff prefix missing
Added.
> [...]
>
> > +static int mms_close(URLContext *h);
> > +int ff_mms_play(URLContext *h);
> > +int ff_mms_seek(URLContext *h, int stream_index, int64_t timestamp, int flags);
>
> why not order the functions so that these arent needed?
Reordered now. All forward declarations removed.
> [...]
>
> > + char *src = path;
> > + while(*src) {
> > + seed += *src;
> > + src++;
> > + }
>
> for(src= path; *src; src++)
> seed += *src;
Modified
> [...]
>
> > + if(packet_type == SC_PACKET_ASF_MEDIA_TYPE ||
> > + packet_type == SC_PACKET_ASF_HEADER_TYPE)
>
> this can be vertically aligned
Aligned (and some more similar places).
> > + continue;
> > +
> > + if(mms->state == STREAM_PAUSED) {
> > + // TODO: result = AVERROR(EAGAIN);
> > + break;
> > + } else if(mms->state == STREAM_DONE || mms->state == STATE_ERROR) {
> > + break;
> > + } else if(mms->state == AWAITING_ASF_HEADER) {
>
> > + /* we have reset the header; spin though the loop. */
> > + while(mms->state != STREAMING && mms->state != STATE_ERROR) {
> > + ff_mms_packet_state_machine(mms);
> > + }
>
> can this end in an infinite loop?
Yes and no (see below). I disabled the loop anyway, because it doesn't
happen normally and I don't have a test case for it.
> [...]
>
> > + MMSContext *mms;
> > +
> > + mms = av_malloc(sizeof(MMSContext));
> > + if (!mms)
> > + return AVERROR(ENOMEM);
> > + memset(mms, 0, sizeof(MMSContext));
>
> MMSContext *mms= av_mallocz()
Thanks. I didn't know about that one.
> > + h->priv_data = mms;
>
> > + h->is_streamed = 1; /* Note: seeking is possible, but not preferred */
>
> maybe we should extend is_streamed to more than just 0/1 so the possible but
> slow could be clearly indicated (this of course has nothing to do with this
> patch ...)
I agree.
> > + mms->av_class = &mmscontext_class;
> > +
> > + url_split(protocol, sizeof(protocol), authorization, sizeof(authorization),
> > + mms->host, sizeof(mms->host), &mms->port,
> > + mms->path, sizeof(mms->path), uri);
> > +
>
> > + if(strcmp(protocol, "mmsh") == 0 || strcmp(protocol, "mms") == 0) {
> > + mms->protocol = &mmsh_mmsprotocol;
> > + } else {
> > + goto fail;
> > + }
>
> how can this else be reached? if it cant then this should be a assert()
True. It was a leftover from separated mmst code. Changed to an assert
for now.
> [...]
> > +/** Close the MMSH/MMST connection */
> > +static int mms_close(URLContext *h)
> > +{
> > + MMSContext *mms = (MMSContext *)h->priv_data;
> > +
> > + if(mms->mms_hd) {
> > + /* send the close packet if we should. */
> > + if(mms->state != STATE_ERROR && mms->protocol->send_shutdown_message)
> > + mms->protocol->send_shutdown_message(mms);
> > +
> > + close_connection(mms);
> > + }
> > +
>
> > + if(mms->asf_header)
> > + av_free(mms->asf_header);
>
> the if() is unneeded and av_freep() seems a better choice as well
Thanks, changed to av_freep(). However, the av_freep():ed pointer
shouldn't ever be reached since the struct it lives in is freed just
below.
> [...]
>
> > + switch(whence) {
> > + case SEEK_END:
> > + case AVSEEK_SIZE:
> > + /* file_size is probably not valid after stream selection,
> > + * so we just disable this */
> > + return -1;
> > + case SEEK_CUR:
> > + cur_pos = mms->incoming_packet_seq * mms->asf_context.packet_size +
> > + mms->asf_header_read_pos +
> > + (mms->media_packet_read_ptr - mms->media_packet_incoming_buffer);
> > +
> > + /* special case for no-op. returns current position in bytes */
> > + if(offset == 0)
> > + return cur_pos;
> > +
> > + offset += cur_pos;
> > +
> > + break;
> > + case SEEK_SET:
> > + break;
> > +
> > + default:
> > + return -1;
> > + }
>
> all the return -1 cases could be placed together
Done
> [...]
>
> > +/** Like AVInputFormat::read_pause().
> > + * @see AVInputFormat::read_pause()
> > + */
> > +int ff_mms_pause(URLContext *h)
> > +{
> > + MMSContext *mms = h->priv_data;
> > +
> > + if(mms->state == STREAMING) { // || mms->state == STREAM_DONE) {
> > + mms->protocol->pause(mms);
> > +
> > + /* wait for acknowledge (throwing away any incoming media) */
> > + while(mms->state != STREAM_PAUSED && mms->state != STATE_ERROR)
> > + ff_mms_packet_state_machine(mms);
>
> cant this end in an infinite loop?
Yes, if the server isn't cooperating and the application isn't using a
url_interrupt_cb that times out.
Every call to ff_mms_packet_state_machine() should read data from the
socket, meaning that tcp_read() gets called. tcp_read() polls
url_interrupt_cb() before and during data reception, and an
application-signaled interrupt will result in EINTR and mms->state =
STATE_ERROR. (Unfortunatly, the EINTR from the tcp protocol is only
propagated as a generic EIO by the mms protocol.)
I can add an explicit url_interrupt_cb poll to the state machine, but I
don't think that is necessary as long as tcp_read() does it.
The state machine also detects unexpected state changes and goes into
STATE_ERROR when it gets confused, so many cases of unexpected server
behaviour are handled here (and in other similar loops that checks for
STATE_ERROR.)
Is it ok?
(Also removed all TODOs that mentioned timeouts, since that should be
handled by the application and the tcp protocol.)
Attached new versions of the base patch and the seek hack. Both updated
for the current svn.
--
Bj?rn Axelsson Phone: +46-(0)90-18 98 97
Intinor AB Fax: +46-(0)920-757 10
www.intinor.se
Interactive Television & Digital Media Distribution
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mms_protocol_base.diff
Type: text/x-patch
Size: 55703 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071015/30ca2358/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mms_seek_hack.diff
Type: text/x-patch
Size: 1960 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071015/30ca2358/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list