[FFmpeg-devel] [PATCH] MMS protocol support patch 1
Björn Axelsson
bjorn.axelsson
Tue Oct 9 14:00:25 CEST 2007
On Sat, 2007-09-22 at 02:43 +0200, Michael Niedermayer wrote:
> Hi
>
> On Wed, Sep 19, 2007 at 05:51:16PM +0200, Bj?rn Axelsson wrote:
> > Hi,
> > this is the first patch for MMS protocol support. Since the last RFC i
> > have cleaned it up overall, split it up into several files and selected
> > a minimal functional subset for your reviewing pleasure. I know it is
> > still a lot of code to review, but it should at least be better than the
> > last monster MMS patch.
> >
> > This patch only includes mmsh (MMS over HTTP) support to keep it as
> > small as possible. It also appears to be what most servers support. Some
> > of the generic mms code make more or less obvious references to the mmst
> > (MMS over TCP) protocol, which I hope is ok since that implementation
> > will be added in a later patch.
> >
> > When (if?) this is accepted I have some separate patches planned:
> > - mmst support (implemented, just waiting for the smoke to clear)
> > - mms protocol that chooses the best possible of the existing
> > subprotocols (either mmst or mmsh)
>
> > - clean API for controlling protocols (seek, play, pause).
>
> its ok if you cleanup the API after adding MMS support but i will not accept
> hacks in the code due to a missing clean API, that is if you can simply
> remove/split out the parts which would need API changes then thats fine
> otherwise the API cleanup will have to be done first
With the still open questions about the API extensions, I've split out
the "asf demuxer seek hack" from the main patch. Without it, seeking and
pausing in ffplay is broken, but at least playback works fine.
> > - fix pausing in live streams.
> >
> > The patch passes make test and ffplay works ok with the few test streams
> > I have run it against.
> >
> > Let the flaming begin ;-)
> [...]
> > +/** @file mms_private.h
> > + * Private data structures and definitions for the MMS protocols.
> > + * @author Ryan Martell
> > + * @author Bj?n Axelsson
> > + */
>
> the C files should be ASCII preferably, the AUTHORS file is UTF-8
Fixed
> [...]
> > +/** State machine states. */
> > +typedef enum {
> > + AWAITING_SC_PACKET_CLIENT_ACCEPTED = 0,
> > + AWAITING_SC_PACKET_TIMING_TEST_REPLY_TYPE,
> > + AWAITING_CS_PACKET_PROTOCOL_ACCEPTANCE,
> > + AWAITING_PASSWORD_QUERY_OR_MEDIA_FILE,
> > + AWAITING_PACKET_HEADER_REQUEST_ACCEPTED_TYPE,
> > + AWAITING_STREAM_ID_ACCEPTANCE,
> > + AWAITING_STREAM_START_PACKET,
> > + AWAITING_ASF_HEADER,
> > + ASF_HEADER_DONE,
> > + AWAITING_PAUSE_ACKNOWLEDGE,
> > + AWAITING_HTTP_PAUSE_CONTROL_ACKNOWLEDGE,
> > + STREAMING,
> > + STREAM_DONE,
> > + STATE_ERROR,
> > + STREAM_PAUSED,
> > + USER_CANCELLED
> > +} MMSState;
>
> could you describe these a little bit, also some ascii art diagram showing a
> normal connection estabishment and various other important state transitions
> might be usefull for understanding the implementation as well as MMS itself
Documentation added, including a very crude transition diagram for a
mmsh session.
> > +
> > +struct MMSProtocol;
> > +typedef struct MMSProtocol MMSProtocol;
> > +
>
> > +/** Private MMS data. */
> > +typedef struct {
> > + char local_guid[37]; ///< My randomly generated GUID.
> > + uint32_t local_ip_address; ///< Not ipv6 compatible, but neither is MMS
> > + int local_port; ///< My local port (sent but not correct).
> > + int sequence_number; ///< Outgoing packet sequence number.
> > + MMSState state; ///< Packet state machine current state.
> > + char path[256]; ///< Path of the resource being asked for.
> > + char host[128]; ///< Host of the resources.
> > + int port; ///< Port of the resource.
>
> please align the comments vertically:
Fixed.
> [...]
> > +/** Clear all buffers. Use after a seek or other discontinuity. */
> > +void clear_stream_buffers(MMSContext *mms)
> > +{
> > + mms->incoming_io_buffer.buf_ptr = mms->incoming_io_buffer.buf_end;
>
> this direct access to ByteIOContext is ugly
Removed, since it isn't needed for mmsh (and probably not even for
mmst).
> also the function is non static and has no ff_ prefix
Fixed.
> [...]
> > +/** Perform state transition. */
> > +void ff_mms_set_state(MMSContext *mms, int new_state)
> > +{
> > + /* Can't exit error state */
> > + if(mms->state == STATE_ERROR) {
> > + av_log(NULL, AV_LOG_ERROR, "MMS: trying to leave STATE_ERROR\n");
>
> it would be nice if av_logs would have a non NULL context, at least the
> non debug ones
Logging context added. I hope I did it the right way.
> [...]
> > +/** Log unexpected incoming packet */
> > +void log_packet_in_wrong_state(MMSContext *mms, MMSSCPacketType packet_type)
> > +{
>
> ff_ prefix or static
Fixed
> [...]
> > +/** Close the remote connection. */
> > +static void close_connection(MMSContext *mms)
> > +{
> > + if(mms->mms_hd) {
> > + if(mms->incoming_io_buffer.buffer) {
> > + av_free(mms->incoming_io_buffer.buffer);
> > + mms->incoming_io_buffer.buffer = NULL;
>
> av_freep()
>
> but this is ugly either way due to direct ByteIOContext access
Changed (see below)
> [...]
> > + /* open the incoming and outgoing connections; you can't open a single
> > + * one with read/write because it only has one buffer, not two.
> > + * You can't use url_fdopen if the flags of the mms_hd have a WR
> > + * component, because it will screw up (returning data that is
> > + * uninitialized) */
> > + flags = mms->mms_hd->flags;
> > + mms->mms_hd->flags = URL_RDONLY;
> > + err = url_fdopen(&mms->incoming_io_buffer, mms->mms_hd);
> > + mms->mms_hd->flags = flags;
>
> please find a clean solution, messing with the flags like that is ugly
I need to use the buffer of a r/w stream for incoming data, but it
defaults to use the buffer for writing.
aviobuf_resetbuf.diff adds a function url_resetbuf() to select the
buffer direction for an existing AV_RDWR ByteIOContext. It is somewhat
similar to url_setbufsize() in that it should be used after the
ByteIOContext is initialized, but before any IO.
If adding to the public aviobuf API is unwanted, I suggest adding it as
ff_url_resetbuf() until someone else finds use for it or until we have
bidirectionally buffered ByteIOContexts.
> [...]
> > + /* the outgoing packet buffer */
> > + init_put_byte(&mms->outgoing_packet_data, mms->outgoing_packet_buffer,
> > + sizeof(mms->outgoing_packet_buffer), 1, NULL, NULL, NULL, NULL);
>
> unused?
Yes, it appears it was only used by the mmst protocol. I've removed the
initialization from the common setup.
> [...]
> > + switch(mms->state) {
> > + case STREAM_PAUSED:
> > + /* We won't get any packets from the server if paused. Nothing else to do
> > + * than to return. FIXME: return AVERROR(EAGAIN)? */
> > + av_log(NULL, AV_LOG_WARNING, "MMS: read when STREAM_PAUSED.\n");
> > + return 0;
>
> yes, returning EAGAIN seems like to correct thing in theory
Yup. But as I may have mentioned before, it would need quite intrusive
changes in a lot of important code and I'm not comfortable enough to
poke around there (yet...).
> [...]
> > +#if (defined CONFIG_MMS_PROTOCOL) || (defined CONFIG_MMST_PROTOCOL) || (defined CONFIG_MMSH_PROTOCOL)
>
> doesnt MMST/MMSH implicate CONFIG_MMS_PROTOCOL ? if not then some ANY_MMS
> might be usefull to simplify these
I'll consider it when adding the mmst and mms protocols. For now I
reduced it to just CONFIG_MMSH_PROTOCOL.
> [...]
> > +// #define USERAGENT "User-Agent: NSPlayer/7.1.0.3055\r\n"
> > +#define USERAGENT "User-Agent: NSPlayer/4.1.0.3856\r\n"
>
> saying the truth doesnt work?
> User-Agent: FFmpeg
Unfortunately no. Explanation added to code.
> [...]
> > +/**
> > + * Build a http GET request for streaming.
> > + * For mms over http, the byte offset seem to include the header.
> > + * @param rate -5, 1, 5, for rewind, play, ffwd (seekable stream).
> > + * @param stream_offset Seek byte offset (valid on pause/play,
> > + * rewind/ffwd/seek use time). Overrides seek_offset. Use -1 to disable.
> > + * @param seek_offset Seek time in ms (valid on rewind, ffwd, seek).
> > + */
> > +static int build_http_stream_request(MMSContext *mms,
> > + char *outgoing_data, int max_length,
> > + int rate, int64_t stream_offset, int64_t seek_offset)
> > +{
> > + char *dst = outgoing_data;
> > + int i;
> > + const char *header_part =
> > + "GET %s HTTP/1.0\r\n"
> > + "Accept: */*\r\n"
> > + USERAGENT
> > + "Host: %s\r\n";
> > + dst += snprintf(dst, max_length-(dst-outgoing_data),
> > + header_part, mms->path, mms->host);
> > + if(mms->seekable) {
> > + int32_t time_offset = (int) seek_offset; /* should be okay. */
> > + const char *seekable_part =
> > + "Pragma: no-cache,rate=%d.000000,stream-time=%u,stream-offset=%u:%u,request-context=%u,max-duration=%u\r\n"
> > + "Pragma: xClientGUID={%s}\r\n"
> > + "Pragma: xPlayStrm=1\r\n";
> > + dst += snprintf(dst, max_length-(dst-outgoing_data), seekable_part,
> > + rate, time_offset, (int) (stream_offset>>32),
> > + (int)(stream_offset&0xffffffff), 2, 0, mms->local_guid);
> > + } else {
> > + const char *live_part =
> > + "Pragma: no-cache,rate=1.000000,request-context=%u\r\n"
> > + "Pragma: xPlayStrm=1\r\n"
> > + "Pragma: xClientGUID={%s}\r\n";
> > + dst += snprintf(dst, max_length-(dst-outgoing_data), live_part,
> > + 2, mms->local_guid);
> > + }
> > +
> > + dst += snprintf(dst, max_length-(dst-outgoing_data),
> > + "Pragma: client-id=%d\r\n"
> > + "Pragma: stream-switch-count=%d\r\n"
> > + "Pragma: stream-switch-entry=",
> > + mms->http_client_id, mms->av_format_ctx->nb_streams);
> > +
> > + for(i = 0; i<mms->av_format_ctx->nb_streams; i++) {
> > + AVStream *st = mms->av_format_ctx->streams[i];
> > + dst += snprintf(dst, max_length-(dst-outgoing_data), "ffff:%d:%d ",
> > + st->id, ff_mms_stream_selection_code(st));
> > + }
> > +
> > + dst += snprintf(dst, max_length-(dst-outgoing_data),
> > + "\r\nConnection: Close\r\n\r\n");
>
> maybe av_strlcatf() could be used to simplifiy code like the above?
It certainly could. My abuse of the snprintf return value was also quite
uneducated. Fixed and simplified.
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.
--
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: aviobuf_resetbuf.diff
Type: text/x-patch
Size: 2054 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071009/84340c90/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mms_protocol_base.diff
Type: text/x-patch
Size: 57886 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071009/84340c90/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mms_seek_hack.diff
Type: text/x-patch
Size: 1849 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071009/84340c90/attachment-0002.bin>
More information about the ffmpeg-devel
mailing list