[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