[FFmpeg-devel] [PATCH] MMS protocol support patch 1
Michael Niedermayer
michaelni
Sun Oct 21 02:14:01 CEST 2007
Hi
On Mon, Oct 15, 2007 at 05:58:44PM +0200, Bj?rn Axelsson wrote:
> 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.
the above should compile if CONFIG_MMSH_PROTOCOL is not defined
>
> In the general case, is it ok to separate patches that either don't
> compile separately or don't work separately?
they must compile and must not break what is working currently
[...]
>
> > [...]
> >
> > > +/** 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.
ok, i missed the url_interrupt_cb / tcp_read()
>
> 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?
ok
[...]
> @@ -105,6 +107,14 @@
> }
> #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");
> +}
> +#endif
i dont think this needs the #ifdef
[...]
> + uint32_t local_ip_address; ///< Not ipv6 compatible, but neither is MMS
> + int local_port; ///< My local port (sent but not correct).
these are only set but never read
> + int sequence_number; ///< Outgoing packet sequence number.
unused
[...]
> + unsigned int packet_id; ///< Current identifier for media packets.
> + unsigned int header_packet_id; ///< Current header packet identifier.
these are written but never read
[...]
> Index: ffplay.c
> ===================================================================
> --- ffplay.c.orig 2007-10-09 10:38:02.000000000 +0200
> +++ ffplay.c 2007-10-09 10:41:16.000000000 +0200
> @@ -1976,8 +1976,10 @@
> else
> av_read_play(ic);
> }
> -#ifdef CONFIG_RTSP_DEMUXER
> - if (is->paused && !strcmp(ic->iformat->name, "rtsp")) {
> +#if (defined CONFIG_RTSP_DEMUXER) || (defined CONFIG_MMSH_PROTOCOL)
> + if (is->paused &&
> + (!strcmp(ic->iformat->name, "rtsp") ||
> + !strcmp(url_fileno(&ic->pb)->prot->name, "mmsh"))) {
> /* wait 10 ms to avoid trying to get another packet */
> /* XXX: horrible */
> SDL_Delay(10);
->seperate patch
[...]
> +/** MMSH data and control is packetized as "chunks" in the HTTP body.
> + * A 4-byte header indicate the chunk type and length. */
> +#define CHUNK_HEADER_LENGTH 4
> +#define CHUNK_TYPE_ASF_HEADER 0x4824 ///<ASF header data follows.
> +#define CHUNK_TYPE_DATA 0x4424 ///<ASF media data follows.
> +#define CHUNK_TYPE_END 0x4524 ///<End of stream reached.
> +#define CHUNK_TYPE_RESET 0x4324
> +#define CHUNK_TYPE_HTTP_HEADER_START 0x5448 ///<"TH" - a HTTP header.
> +#define CHUNK_TYPE_HTTP_HEADER_SECOND 0x5054 ///<"PT" - a HTTP header.
please use 'H' + ('T'<<8) and similar instead of the less meaningfull 0xABCD
these all are meaningfull in ASCII
[...]
> + } else if(ch == '\r') {
> + /* eat it. */
> + } else if(dst-buffer < max_size-1) {
> + *dst++ = ch;
> + }
} else if(ch != '\r' && dst-buffer < max_size-1) {
*dst++ = ch;
}
> + }
> + *dst='\0';
any reason why not *dst=0; ?
> +
> + return buffer-dst;
> +}
> +
> +/** Try to match match_str as a prefix of src.
> + * @param src String to search in.
> + * @param match_str String to search for.
> + * @return NULL if no match, pointer to the end of the prefix in src if match.
> + */
> +static const char *matches(const char *src, const char *match_str)
> +{
> + int match_length = strlen(match_str);
> +
> + if(strncmp(src, match_str, match_length) == 0)
> + return src + match_length;
> + else
> + return NULL;
> +}
maybe av_strstart() could be used
[...]
> + const char *match = NULL;
> +
> + if((match = strstr(src, match_str)) != 0)
> + match += strlen(match_str);
const char *match = strstr(src, match_str);
if(match)
match+=strlen(match_str);
> +
> + return match;
> +}
> +
> +/** Parse the header of a MMSH chunk. */
> +static int get_http_chunk_header(MMSContext *mms,
> + int *chunk_type, int *chunk_length)
> +{
> + int ext_length;
> +
> + *chunk_type = get_le16(&mms->incoming_io_buffer);
> + *chunk_length = get_le16(&mms->incoming_io_buffer);
> +
> + switch (*chunk_type) {
> + case CHUNK_TYPE_ASF_HEADER:
> + case CHUNK_TYPE_DATA:
> + mms->incoming_packet_seq = get_le32(&mms->incoming_io_buffer);
> + get_byte(&mms->incoming_io_buffer);
> + get_byte(&mms->incoming_io_buffer);
> + get_le16(&mms->incoming_io_buffer);
> + ext_length = 8;
> + break;
> +
> + case CHUNK_TYPE_END:
> + case CHUNK_TYPE_RESET:
> + get_le32(&mms->incoming_io_buffer);
> + ext_length = 4;
> + break;
> +
> + case CHUNK_TYPE_HTTP_HEADER_START:
> + default:
> + /* Not a chunk header, but a HTTP header. */
> + ext_length = 0;
> + break;
> + }
> +
> + *chunk_length -= ext_length;
the ext_length variable seems redundant, chunk_length could be decremented
directly, though that is nitpicking and iam perfectly fine with the
ext_length if you prefer
[...]
> + if(mms->state != STREAMING &&
> + mms->state != AWAITING_HTTP_PAUSE_CONTROL_ACKNOWLEDGE)
this could be vertically aligned
[...]
> + const char *seekable = matches_anywhere(features, "seekable");
> + const char *broadcast = matches_anywhere(features,"broadcast");
these could be vertically aligned
[...]
> + .read_play = asf_read_play,
> + .read_pause = asf_read_pause,
these 2 should be under CONFIG_MMSH_PROTOCOL to prevent bloating the code with
unused functions
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071021/4ab3b72a/attachment.pgp>
More information about the ffmpeg-devel
mailing list