[FFmpeg-devel] [PATCH][0/4]: MMS base and MMSH implementation
Björn Axelsson
bjorn.axelsson
Mon Mar 10 15:47:06 CET 2008
On Thu, 2008-03-06 at 22:44 +0100, Michael Niedermayer wrote:
> 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 ///< ?
Should be. Fixed.
> [...]
> > +/** Private MMS data. */
> > +typedef struct {
>
> > + const AVClass *av_class; ///< Context for logging.
>
> this does belong in URLWhatever not in your private context
A separate patch for AVClass-in-URLContext at the next major version
bump has been submitted.
I've left the avclass stuff in MMSContext for now with a TODO comment to
use the URLContext later. If you insist I'll remove it and use a NULL
context for mms logging, as all other protocols do.
> > + 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.
I've tried to clean up the field names and comments. Hopefully this
version is less confusing.
> > + 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.
Replaced.
> > + int media_packet_buffer_length; ///< Buffer length.
>
> > + int incoming_packet_seq; ///< Packet sequence number.
>
> what is this good for? it is never read ...
It was used for seeking. Removed in this version.
> > + /*@}*/
> > +
>
> > + 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.
Both removed for now.
> > +
> > + /** @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?
Discussed in separate mail.
I've kept this functionality for now because it lets client applications
access mms streams just like asf files. I also don't think it adds much
complexity, since I still need to store the header and return it from
read() at least until read_header() is done.
> > + /*@{*/
> > + 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.
Point taken, and the stored contexts have been replaced with the
extracted data.
> > + 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 [].
Removed for now. Maybe the only clean way is a new av_discard(URLContext
*h, AVDiscard[]) call?
> > + /*@}*/
> > +
>
> > + 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.
Comment updated. Note that the pause state isn't actually used yet, but
will be as soon as play/pause is added.
> > + 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.
Changed it to a more expressive enum.
> [...]
> > +/** @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
Docs moved from .c to .h
> [...]
> > +
> > +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.
Moved into mms_protocol.c, but still the same as there's no header to
include from.
> > + 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
url_open_dyn_buf() seems appropriate, but as MMSH doesn't split the
header over multiple packets I got rid of it completely for now.
> [...]
> > +/** 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.
Changed, though I'm still not happy about the name.
Also, the function can be replaced with a table if you think that is
cleaner. But it isn't time-critical code and the function is slighly
more robust against bad inputs.
> [...]
> > +/** 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?
I honestly don't know. That alternative USERAGENT comes from the very
first versions of the code, and I have no idea what would be different.
I simply removed it for now.
> [...]
> > +/** 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.
I think you'll find the new patch much more consistent in naming.
--
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_base_and_http.diff
Type: text/x-patch
Size: 30407 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080310/7dc02da8/attachment.bin>
More information about the ffmpeg-devel
mailing list