[FFmpeg-devel] [PATCH] MMS protocol support patch 1

Michael Niedermayer michaelni
Sat Oct 13 02:37:04 CEST 2007


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 :)


[...]

> +MMSProtocol mmsh_mmsprotocol = {

ff prefix missing


[...]

> +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?


[...]

> +    char *src = path;
> +    while(*src) {
> +        seed += *src;
> +        src++;
> +    }

for(src= path; *src; src++)
    seed += *src;


[...]

> +            if(packet_type == SC_PACKET_ASF_MEDIA_TYPE ||
> +                    packet_type == SC_PACKET_ASF_HEADER_TYPE)

this can be vertically aligned



> +                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?


[...]

> +    MMSContext *mms;
> +
> +    mms = av_malloc(sizeof(MMSContext));
> +    if (!mms)
> +        return AVERROR(ENOMEM);
> +    memset(mms, 0, sizeof(MMSContext));

MMSContext *mms= av_mallocz()


> +    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 ...)


> +    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()


[...]
> +/** 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


[...]

> +    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


[...]

> +/** 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?


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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20071013/8dc2b71a/attachment.pgp>



More information about the ffmpeg-devel mailing list