[FFmpeg-devel] [PATCH] MMS protocol support patch 1
Björn Axelsson
bjorn.axelsson
Fri Nov 2 11:43:44 CET 2007
On Sun, 2007-10-21 at 02:14 +0200, Michael Niedermayer wrote
> 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:
> > > > > On Tue, Oct 09, 2007 at 02:00:25PM +0200, Bj?rn Axelsson wrote:
[...]
> > >
> > > (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
It doesn't for me as gcc still tries to link with
ff_mms_set_stream_selection(), which is not included if
CONFIG_MMSH_PROTOCOL is not defined. Maybe because I like to compile
without optimizations when debugging, maybe because gcc is stupid. In
any case I wanted to be on the safe side and #ifdef-protected the call.
> > 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
Ok. Since the changes to asf.c and ffplay.c arent't crucial to the mms
protocol, I have broken them out in separate patches and will post them
in separate threads.
> [...]
> > @@ -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
It prevents bloating the code ;-). Anyhow, broken out to separate patch
and thread.
> [...]
>
> > + 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
Ok. I took out all fields not shared between the tcp and http protocols
and used a priv_data field for that instead. Slightly pointless for the
http protocol (only one unique field so far), but required for the tcp
protocol.
> [...]
> > 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
Yes.
> [...]
>
> > +/** 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
Done. However, except for the HTTP string it is unclear whether they are
meant to be meaningful in ASCII or if it is by accident.
> [...]
> > + } 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;
> }
Done.
> > + }
> > + *dst='\0';
>
> any reason why not *dst=0; ?
Personal preference to emphasize that it is a string termination and not
the number 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
Certainly so. The function is removed and the code that called it
slightly simplified. Thanks for pointing that out.
> [...]
> > + 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);
Changed.
> > +
> > + 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
nitpicking accepted. ext_length was left over from some debugging code I
removed earlier.
> [...]
> > + 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
Both 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
Done, but in separate patch and mail.
--
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: 53158 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071102/f38f4306/attachment.bin>
More information about the ffmpeg-devel
mailing list