[FFmpeg-devel] [PATCH] MMS protocol support patch 1
Michael Niedermayer
michaelni
Sat Sep 22 02:43:38 CEST 2007
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
> - 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
[...]
> +/** 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
> +
> +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:
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).
...
[...]
> +/** 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
also the function is non static and has no ff_ prefix
[...]
> +/** 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
[...]
> +/** Log unexpected incoming packet */
> +void log_packet_in_wrong_state(MMSContext *mms, MMSSCPacketType packet_type)
> +{
ff_ prefix or static
[...]
> +/** 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
[...]
> + /* 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
[...]
> + /* 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?
[...]
> + 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
[...]
> +#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
[...]
> +// #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
[...]
> +/**
> + * 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?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20070922/377a6b83/attachment.pgp>
More information about the ffmpeg-devel
mailing list