[FFmpeg-devel] [PATCH][0/4]: MMS base and MMSH implementation
Michael Niedermayer
michaelni
Thu Mar 6 22:44:58 CET 2008
On Thu, Mar 06, 2008 at 03:53:09PM +0100, Bj?rn Axelsson wrote:
> On Sun, 2008-02-03 at 12:48 +0100, Reimar D?ffinger wrote:
> > Hello,
> > On Thu, Dec 13, 2007 at 11:03:46PM +0100, Michael Niedermayer wrote:
> > > a totally hypothetical and probably not applicable variant would be
> > > 1. a patch for just establishing the connection
> > > 2. a patch for reading packets
> > > 3. a patch which adds seeking
> > > 4. a patch which adds pause support
> > > 5. a patch which adds support to send back connection statistics to the servr
>
> I have had some time between other projects to look into this again.
>
> The attached mmsh patch has no state machine (which didn't make much
> sense for mmsh anyway), and is reduced to only include points 1 and 2
> from above.
> Also, the HTTP parser and packet reader has been through a few more
> iterations and cleanups.
> Hopefully this brings it into the realm of reviewability...
>
> A refreshed build patch is also included for anyone who wants to test
> it.
>
> > I wanted to see if I could do such splitting (actually just removing 3
> > and 4 to see if the state machine can be get rid of), but I do not have
> > a stream that works with it.
> > I tried the first best thing that google could find:
> > mmsh://wm.interoute.com/{98f50251-8adb-4429-af0f-60ee34e6cd57}/{d2f6a347-0e2f-4ac1-a375-1d9d6e295c0f}/qotsa_wieLive.wmv
> > It plays with MPlayer but not with ffplay.
> > Messages:
> > > [mmsh @ 0x79ff00]read before play. Playing automatically.
> > > [mmsh @ 0x79ff00]Data chunk larger than buffer (8948 > 8192)
> > > [mmsh @ 0x79ff00]Got packet 0x81 in the wrong state: AWAITING_STREAM_ID_ACCEPTANCE
> >
> > I might be missing a part of the patch though, not sure, I applied this
> > one and the build system stuff.
>
> No, it was just that the packet size of the stream was larger than the
> hardcoded buffer size. I have increased the buffer size considerably in
> this version, but ultimately it should be dynamically allocated. The
> attached patch plays the stream above.
[...]
> +/** Server to client packet types. */
> +typedef enum {
> + /** @name Pseudo packets. Used for internal signalling. */
> + /*@{*/
> + SC_PACKET_TYPE_ERROR = -1, /// Error detected
> + SC_PACKET_TYPE_NO_DATA = -2, /// No data available (EOF)
> + SC_PACKET_TYPE_ACKNOWLEDGE = -3, /// Dataless control package
> + /*@}*/
are these comments correctly associated with the fields? shouldnt it be ///< ?
[...]
> +/** Private MMS data. */
> +typedef struct {
> + const AVClass *av_class; ///< Context for logging.
this does belong in URLWhatever not in your private context
> + char local_guid[37]; ///< My randomly generated GUID.
> + char path[256]; ///< Path of the resource being asked for.
> + char host[128]; ///< Host of the resources.
> + int port; ///< Port of the resource.
> +
> + URLContext *mms_hd; ///< TCP connection handle.
> + ByteIOContext *incoming_io_buffer; ///< Incoming data on the socket.
> +
> + /** @name Incoming Media Buffer
> + * Buffer for incoming media and header packets. */
> + /*@{*/
> + uint8_t media_packet_incoming_buffer[64*1024];///< Header or media packet.
asf or mms ? can this contain one packet, several?
please document it!
The comment as it is is useless one still has to read all related code.
> + uint8_t *media_packet_read_ptr; ///< Pointer for partial reads
Its nice to know what a variable is used for better would be to know what
the variable actually contains. I tried to guess but after reading the code
it seems i guessed wrong.
> + int media_packet_buffer_length; ///< Buffer length.
> + int incoming_packet_seq; ///< Packet sequence number.
what is this good for? it is never read ...
> + /*@}*/
> +
> + MMSProtocol *protocol; ///< Function pointers for the subprotocol.
> + void *priv_data; ///< Private data for subprotocol.
No, no void * subprotocol stuff please! We dont need this abstraction for TCP vs.
HTTP.
Iam also somewhat opposed to MMSProtocol *protocol. But that is rater minor.
> +
> + /** @name ASF Header
> + * Internal handling of the ASF header. The ASF header is stored by the
> + * protocol to facilitate seeking into it. The protocol also parses the
> + * header to get the packet size and elementary stream information.
> + */
why would we want to seek into the header?
> + /*@{*/
> + uint8_t *asf_header; ///< ASF header buffer.
> + int asf_header_size; ///< Size of stored ASF header.
> + int asf_header_read_pos; ///< Header read offset. See read_packet().
> + int header_parsed; ///< Flag: header has been fully parsed.
> + AVFormatContext private_av_format_ctx; ///< Private header data (generic).
> + ByteIOContext private_av_format_pb; ///< Private header data (IO buf).
> + ASFContext asf_context; ///< Private header data (ASF).
private of what? we have an asf demuxer (which you shouldnt touch) a
subprotocol (mmsh), this mms stuff, an underlaying protocol (tcp/http)..
is tha asf stuff here the one actually demuxing things or some private
seperate thing unrelated to the demuxer used to help avoid code duplication.
All this should be clearly documented.
And after looking at the code, half of the above look like local variables
which shouldnt be in the context.
> + AVFormatContext *av_format_ctx; /**< Optional external format context
> + (for stream selection). */
hmm, "Private header data (generic)." and "Optional external format context"
I really wonder if anyone short of the author could make any sense of these.
yes i remember
it faintly and i can figure it out by reading the code but this kind of
documentation is not usefull. It doesnt tell me anything at all.
You could just write foo and bar and i would know the same.
And after reading the code, no you cannot access the demuxer context and
discard streams based on its AVStream.discard. This is very ugly.
Also theres no need nor sense or anything else in this.
The data if it really has to be passed from demuxer to protocol can be
in an AVDiscard [].
> + /*@}*/
> +
> + MMSControlState control_state; ///< Play/Pause state from application.
If it stores pause or not call it is_paused and make it a int. If it stores
something else then the comment is not correct.
> + int eof; ///< Flag: EOF or error state.
EOF and error are 2 quite seperate things, also the field name eof is not
appropriate to indicate the later.
[...]
> +/** @name Shared utility functions. */
> +/*@{*/
> +int ff_mms_open_connection(MMSContext *mms);
> +int ff_mms_stream_selection_code(AVStream *st);
> +int ff_mms_store_header(MMSContext *mms, int offset);
> +MMSSCPacketType ff_get_next_packet(MMSContext *mms);
They all need to be documented
[...]
> +
> +extern AVInputFormat asf_demuxer;
This is ugly besides that it should be #include not such copy and
paste hacks of what you need from the header.
[...]
> + if(mms->asf_header_size + mms->media_packet_buffer_length > 1024*1024) {
> + av_log(mms, AV_LOG_ERROR,
> + "Unreasonably large ASF header (at least %d bytes)\n",
> + mms->media_packet_buffer_length + mms->asf_header_size);
> + mms->eof = 1;
a large packet does not indicate eof
treating it as such is not acceptable
> + return -1;
> + }
> +
> + mms->asf_header = av_realloc(mms->asf_header, mms->asf_header_size +
> + mms->media_packet_buffer_length);
> + memcpy(mms->asf_header + mms->asf_header_size, mms->media_packet_read_ptr,
> + mms->media_packet_buffer_length);
> + mms->asf_header_size += mms->media_packet_buffer_length;
This code is exploitable if realloc() fails.
also its a mess
see url_open_dyn_buf() and fifo.c, not sure which is more appropriate
[...]
> +/** Convert from st->discard AVDISCARD values to MMS stream selection code. */
> +int ff_mms_stream_selection_code(AVStream *st)
ff_mms_stream_selection_code is a noun thus this sounds like a variable,
it is not thus the name is not very good.
[...]
> + av_log(mms, AV_LOG_ERROR, "Unexpected packet %d\n", packet_type);
> + mms->eof = 1;
as already said eof != error
[...]
> +/** We must lie and say that we are NSPlayer, or the server will give us a
> + * redirector file (video/x-ms-wvx) instead of the actual stream. Also, it
> + * seems that the server will use different protocol variants depending on
> + * the reported client version. */
> +#define USERAGENT "NSPlayer/4.1.0.3856"
> +// #define USERAGENT "NSPlayer/7.1.0.3055"
What is different?
[...]
> +/** Try to get a MMSH data packet from the server.
> + * A command response is normally a HTTP response header possibly followed by
> + * header and/or media data. We will read at most one new packet of media
> + * data before returning.
> + */
> +static MMSSCPacketType get_http_packet(MMSContext *mms)
> +{
> + MMSHContentType content_type = MMSH_UNKNOWN;
> + int res, chunk_type, chunk_length;
> +
> + for(;;) {
> + chunk_type = get_le16(mms->incoming_io_buffer);
> + chunk_length = get_le16(mms->incoming_io_buffer);
> +
> + if(url_feof(mms->incoming_io_buffer))
> + return SC_PACKET_TYPE_NO_DATA;
> + else if(chunk_type==CHUNK_TYPE_ASF_HEADER ||
> + chunk_type==CHUNK_TYPE_DATA) {
> + mms->incoming_packet_seq = get_le32(mms->incoming_io_buffer);
> + url_fskip(mms->incoming_io_buffer, 4);
> + chunk_length -= 8;
> +
> + if(chunk_length > sizeof(mms->media_packet_incoming_buffer)) {
> + /* TODO: should dynamically resize the buffer */
> + av_log(mms, AV_LOG_ERROR,
> + "Data chunk larger than buffer (%d > %d)\n",
> + chunk_length,
> + sizeof(mms->media_packet_incoming_buffer));
> + return SC_PACKET_TYPE_ERROR;
Please make type (MMSSCPacketType) and identifers
returned (SC_PACKET_TYPE_ERROR) have names which have a common prefix.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20080306/b1c00287/attachment.pgp>
More information about the ffmpeg-devel
mailing list