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-60ee34e6cd5
7}//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 %dn", 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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
|
