[PATCH][0/4]: MMS base and MMSH implementation
This is basically the same as my old MMS patch, only somewhat updated and split into smaller patches (one per file). Patch 1 and 2 are only new files and can be applied in any order. Patch 3 depends on 1 and 2, but still only touches the new files. The fourth patch in this series contains the makefile changes and protocol registration code, and should not be applied before the other three are applied. Only when this patch is applied can the new code be compiled and used. The complete series passes "make test". -- 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
On Thu, 2007-11-29 at 11:00 +0100, Bj?rn Axelsson wrote:
This is basically the same as my old MMS patch, only somewhat updated and split into smaller patches (one per file).
Patch 1 and 2 are only new files and can be applied in any order. Patch 3 depends on 1 and 2, but still only touches the new files. The fourth patch in this series contains the makefile changes and protocol registration code, and should not be applied before the other three are applied. Only when this patch is applied can the new code be compiled and used.
The complete series passes "make test".
Ping? This has hasn't been reviewed in two weeks. Please let me know if I can do anything more to help. -- 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
On Thu, Dec 13, 2007 at 04:29:26PM +0100, Bj?rn Axelsson wrote:
On Thu, 2007-11-29 at 11:00 +0100, Bj?rn Axelsson wrote:
This is basically the same as my old MMS patch, only somewhat updated and split into smaller patches (one per file).
Patch 1 and 2 are only new files and can be applied in any order. Patch 3 depends on 1 and 2, but still only touches the new files. The fourth patch in this series contains the makefile changes and protocol registration code, and should not be applied before the other three are applied. Only when this patch is applied can the new code be compiled and used.
The complete series passes "make test".
Ping? This has hasn't been reviewed in two weeks. Please let me know if I can do anything more to help.
spliting the patch per files, (it seems thats what you did) does not help me review it actually it makes it harder, what would help is split it in some self contained way 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 ... in the above 1/1+2/1+2+3/.. must of corse be applyable without breaking ffmpeg and 1 must not contain anything not needed for 1 itself, ... anyway ill review the patches you posted eventually, iam just reviewing smaller/simpler patches first [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Thu, 2007-12-13 at 23:03 +0100, Michael Niedermayer wrote:
On Thu, Dec 13, 2007 at 04:29:26PM +0100, Bj?rn Axelsson wrote:
On Thu, 2007-11-29 at 11:00 +0100, Bj?rn Axelsson wrote:
This is basically the same as my old MMS patch, only somewhat updated and split into smaller patches (one per file).
Patch 1 and 2 are only new files and can be applied in any order. Patch 3 depends on 1 and 2, but still only touches the new files. The fourth patch in this series contains the makefile changes and protocol registration code, and should not be applied before the other three are applied. Only when this patch is applied can the new code be compiled and used.
The complete series passes "make test".
Ping? This has hasn't been reviewed in two weeks. Please let me know if I can do anything more to help.
spliting the patch per files, (it seems thats what you did) does not help me review it actually it makes it harder, what would help is split it in some self contained way
Splitting into seperate files seemed like a useful compromise to me, since it would allow me to fix minor issues in a single file without you having to review everything again. I have more (though smaller) patches in the pipeline waiting for this to pass. It is very frustrating for me not being able to get these patches passed, but I understand that you have limited time and motivation.
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 ...
The above would indeed be very hard to do without a lot of extra work on my end and a big risk of introducing bugs and/or dead code in the intermediate steps. Anyhow. Attached is a combined version of the first three patches. They are updated for current SVN including the recent changes in the "unused" av_read_pause() API. The fourth patch was the makefile stuff, which still applies and passed review I believe. I would also like to remind you about the av_url_read_fseek_api.diff patch that was never applied: http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-December/039248.html -- 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
On Mon, Jan 07, 2008 at 02:04:43PM +0100, Bj?rn Axelsson wrote:
On Thu, 2007-12-13 at 23:03 +0100, Michael Niedermayer wrote:
On Thu, Dec 13, 2007 at 04:29:26PM +0100, Bj?rn Axelsson wrote:
On Thu, 2007-11-29 at 11:00 +0100, Bj?rn Axelsson wrote:
This is basically the same as my old MMS patch, only somewhat updated and split into smaller patches (one per file).
Patch 1 and 2 are only new files and can be applied in any order. Patch 3 depends on 1 and 2, but still only touches the new files. The fourth patch in this series contains the makefile changes and protocol registration code, and should not be applied before the other three are applied. Only when this patch is applied can the new code be compiled and used.
The complete series passes "make test".
Ping? This has hasn't been reviewed in two weeks. Please let me know if I can do anything more to help.
spliting the patch per files, (it seems thats what you did) does not help me review it actually it makes it harder, what would help is split it in some self contained way
Splitting into seperate files seemed like a useful compromise to me, since it would allow me to fix minor issues in a single file without you having to review everything again.
The problem is these files are highly dependant on each other, its not really possible to say ok to one without also saying ok to all others. As example just take mms_private.h i cant really say anything about anything in there without fully reviewing the code which uses the specific part. Take one state of the state machine, ask the question if this state is needed. You cant awnser that without the code using that state. Or ask the more general question if the state machine is needed at all. I wont accept the messy state machine without being certain that it is impossible to reimplement it cleaner. To which of the files could i say ok without saying ok to the state machine? none? And the state machine is just one part ... [...]
I would also like to remind you about the av_url_read_fseek_api.diff patch that was never applied: http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-December/039248.html
Sending several patches in a single mail is a perfect recipe to have some of them lost, others have been hit by this as well. Anyway the av_url_read_fseek_api patch does not look very important, it just changes a doxy comment ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
On Mon, Jan 07, 2008 at 02:04:43PM +0100, Bj?rn Axelsson wrote:
On Thu, 2007-12-13 at 23:03 +0100, Michael Niedermayer wrote:
On Thu, Dec 13, 2007 at 04:29:26PM +0100, Bj?rn Axelsson wrote:
On Thu, 2007-11-29 at 11:00 +0100, Bj?rn Axelsson wrote:
This is basically the same as my old MMS patch, only somewhat updated and split into smaller patches (one per file).
Patch 1 and 2 are only new files and can be applied in any order. Patch 3 depends on 1 and 2, but still only touches the new files. The fourth patch in this series contains the makefile changes and protocol registration code, and should not be applied before the other three are applied. Only when this patch is applied can the new code be compiled and used.
The complete series passes "make test".
Ping? This has hasn't been reviewed in two weeks. Please let me know if I can do anything more to help.
spliting the patch per files, (it seems thats what you did) does not help me review it actually it makes it harder, what would help is split it in some self contained way
Splitting into seperate files seemed like a useful compromise to me, since it would allow me to fix minor issues in a single file without you having to review everything again.
I have more (though smaller) patches in the pipeline waiting for this to pass. It is very frustrating for me not being able to get these patches passed, but I understand that you have limited time and motivation.
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 ...
The above would indeed be very hard to do without a lot of extra work on my end and a big risk of introducing bugs and/or dead code in the intermediate steps.
well i think about 80% of the code needs to be completely redesigned and rewritten and what you just said above confirms that the code is unmaintainable that is it cant be changed without introducing hundreads of bugs none the less i tried to review it a little further, this is far from a complete review. And i suspect it will take many years if you dont radically cleanup the code (like removing the state machine) before a really complete review will be done by anyone. And most likely the review will say little more than "unneeded" to 80% of the code [...]
+ SC_PACKET_STREAM_STOPPED_TYPE = 0x1e, // mmst, mmsh + SC_PACKET_STREAM_CHANGING_TYPE = 0x20,
should this be a doxygen comment? [...]
+/** State machine states. */ +typedef enum { + /** The protocol handler is initializing. This state should never be + * seen outside of mms_open(). */ + INITIALIZING = 0, + /** The startup packet has been sent and we are awaiting acceptance from + * the server (TCP only). */ + AWAITING_SC_PACKET_CLIENT_ACCEPTED, + /** We have sent timing packets to the server and are awaiting the reply + * (TCP only). */ + AWAITING_SC_PACKET_TIMING_TEST_REPLY_TYPE, + /** We have sent a protocol request packet and are awaiting acceptance + * from the server (TCP only). */ + AWAITING_CS_PACKET_PROTOCOL_ACCEPTANCE, + /** We have requested media stream details and are awaiting either the + * actual details or a password query (TCP only). */ + AWAITING_PASSWORD_QUERY_OR_MEDIA_FILE, + /** We have requested a media file header and are waiting for acceptance + * (TCP only). */ + AWAITING_PACKET_HEADER_REQUEST_ACCEPTED_TYPE, + /** We have sent a stream id request and selected elementary streams. + * Waiting for acceptance (TCP and HTTP). */ + AWAITING_STREAM_ID_ACCEPTANCE, + /** We have selected the time point or packet sequence to start streaming + * from and are waiting for acknowledge (TCP only). */ + AWAITING_STREAM_START_PACKET, + /** We now expect the ASF media header (TCP and HTTP) to follow in the + * stream (TCP and HTTP). */ + AWAITING_ASF_HEADER, + /** The ASF header has been fully received (TCP and HTTP). */ + ASF_HEADER_DONE, + /** We have requested a pause and are waiting for the ack + * (TCP and HTTP). */ + AWAITING_PAUSE_ACKNOWLEDGE, + /** We have requested a pause and are waiting for the stream to end. + * (Prestate to AWAITING_PAUSE_ACKNOWLEDGE) (HTTP only). */ + AWAITING_HTTP_PAUSE_CONTROL_ACKNOWLEDGE, + /** We are currently expecting ASF media packets (TCP and HTTP). */ + STREAMING, + /** EOF or unhandled switch between two streams (TCP and HTTP). */ + STREAM_DONE, + /** An unresolved error occured. There is no exit from the error state, + * and all protocol calls should return errors (TCP and HTTP). */ + STATE_ERROR, + /** The stream is currently paused (TCP and HTTP). */ + STREAM_PAUSED, +} MMSState;
all states which can be removed must be removed all while(packet_type != X) packet_type = ff_mms_packet_state_machine(mms); must be changed to while(packet_type != X) packet_type = read_the_next_packet(mms); and with read_the_next_packet() of course not touching any state machine related variables all while(state != X) packet_type = ff_mms_packet_state_machine(mms); must be changed to while(whatever condition signifies change to state X) packet_type = read_the_next_packet(mms); and so on [...]
+ for(i = 0; i<36; i++) { + switch(i) { + case 8: + case 13: + case 18: + case 23: + *dst++ = '-'; + break; + + default: + *dst++ = digit[av_random(&random_state)%16]; + break; + } + } + *dst = '\0';
for(i = 0; i<36; i++) dst[i] = digit[av_random(&random_state)&15]; dst[8]=dst[13]=dst[18]=dst[23]= '-' dst[36]= 0; [...]
+ switch (*chunk_type) { + case CHUNK_TYPE_ASF_HEADER: + case CHUNK_TYPE_DATA: + mms->incoming_packet_seq = get_le32(mms->incoming_io_buffer); + get_byte(mms->incoming_io_buffer); + get_byte(mms->incoming_io_buffer); + get_le16(mms->incoming_io_buffer); + *chunk_length -= 8; + break; + + case CHUNK_TYPE_END: + case CHUNK_TYPE_RESET: + get_le32(mms->incoming_io_buffer); + *chunk_length -= 4; + break; + + case CHUNK_TYPE_HTTP_HEADER_1: + /* Not a chunk header, but a HTTP header. */ + default: + break; + }
this can be simplified alot, its not really doing anything besides skiping over data
+ + return url_feof(mms->incoming_io_buffer); +} +
+/** Get a MMSH data packet from the server. */ +static MMSSCPacketType get_http_server_response_data(MMSContext *mms, + const char *content_type) +{ + MMSSCPacketType packet_type = SC_PACKET_TYPE_ERROR; + + int res, chunk_type, chunk_length, eof; + + while(!(eof = get_http_chunk_header(mms, &chunk_type, &chunk_length))) { + if(chunk_type==CHUNK_TYPE_ASF_HEADER || chunk_type==CHUNK_TYPE_DATA) { + if(chunk_length > sizeof(mms->media_packet_incoming_buffer)) { + av_log(mms, AV_LOG_ERROR, + "Data chunk larger than buffer (%d > %d)\n", + chunk_length, + sizeof(mms->media_packet_incoming_buffer)); + break; + } + + res = get_buffer(mms->incoming_io_buffer, + mms->media_packet_incoming_buffer, chunk_length); + if(res != chunk_length) { +// av_log(mms, AV_LOG_ERROR, +// "Couldn't read full chunk (%d != %d)\n", +// res, chunk_length); + eof = 1; + break; + } + mms->media_packet_read_ptr = mms->media_packet_incoming_buffer; + mms->media_packet_buffer_length = res; + + if(chunk_type == CHUNK_TYPE_DATA) { + if(mms->state != STREAMING && + mms->state != AWAITING_HTTP_PAUSE_CONTROL_ACKNOWLEDGE) + ff_mms_set_state(mms, STREAMING); + packet_type = SC_PACKET_ASF_MEDIA_TYPE; + break; + } else { + ff_mms_store_header(mms, 0); + packet_type = SC_PACKET_ASF_HEADER_TYPE; + /* force the bit saying we got the entire header */ + mms->incoming_flags = 0x08; + + /* Break out after header on the first request. */ + if(mms->state == AWAITING_ASF_HEADER && content_type && ( + strcmp(content_type, "application/vnd.ms.wms-hdr.asfv1") == 0 || + strcmp(content_type, "application/octet-stream") == 0)) { + break; + } + } + } else if(chunk_type == CHUNK_TYPE_HTTP_HEADER_1 && + chunk_length == CHUNK_TYPE_HTTP_HEADER_2) { + if(mms->state == AWAITING_HTTP_PAUSE_CONTROL_ACKNOWLEDGE) { + /* start of the header: break out and change state. */ + packet_type = SC_PACKET_HTTP_CONTROL_ACKNOWLEDGE; + } + break; + } else if(chunk_type == CHUNK_TYPE_END) { + av_log(mms, AV_LOG_VERBOSE, "Server indicated end-of-stream\n"); + packet_type = SC_PACKET_TYPE_NO_DATA; + break; + } else { + av_log(mms, AV_LOG_WARNING, + "Unknown chunk type: 0x%04x, length: %d\n", + chunk_type, chunk_length); + break; + } + } + + if(eof && packet_type == SC_PACKET_TYPE_ERROR) { + packet_type = SC_PACKET_TYPE_NO_DATA; + } + + return packet_type; +}
this combines reading chunks and the state machine, this must be split the code reading chunks MUST NOT use any variable or state related to the state machine neither direct nor indirect the only part which is allowed to touch variables of the state machine is the state machine itself (the reason being its unmaintainable otherwise)
+ +/** Parse MMSH server response with a HTTP response header. */ +static MMSSCPacketType get_http_server_response_header(MMSContext *mms) +{ + MMSSCPacketType packet_type = SC_PACKET_TYPE_ERROR; + char line[512]; + int content_length = 0; + char content_type[128] = ""; + int line_number = 0; + + /* Parse HTTP response header */ + while(get_http_line(mms->incoming_io_buffer, line, sizeof(line))) { + const char *match; + + if(mms->state == AWAITING_PAUSE_ACKNOWLEDGE && line_number == 0) { + /* the chunking system read the HTTP, so we reinsert it */ + memmove(line+4, line, FFMIN(strlen(line)+1, sizeof(line)-4)); + line[0]= 'H'; line[1]= 'T'; line[2]= 'T'; line[3]= 'P'; + line[sizeof(line)-1] = '\0'; // Terminate if line got too long + }
same as above, all uses of the state machine must be removed this is a totally unmaintainable and intermingled mess [...]
+ } else if(http_code>=300 && http_code<400) { + av_log(mms, AV_LOG_ERROR, + "3xx redirection not implemented: %d %s\n", + http_code, http_status); + return SC_PACKET_TYPE_ERROR; + } else { + av_log(mms, AV_LOG_ERROR, "%d %s\n", http_code, http_status); + return SC_PACKET_TYPE_ERROR; + }
the else if is totally senseless [...]
+/** Parse MMSH server response. The response is normally a media or header + * chunk when streaming, and/or a HTTP response header after a command or + * after a (re)connection. + */ +static MMSSCPacketType get_http_server_response(MMSContext *mms) +{ + MMSSCPacketType packet_type = SC_PACKET_TYPE_ERROR; + + switch(mms->state) { + case AWAITING_ASF_HEADER: + case AWAITING_STREAM_ID_ACCEPTANCE: + case AWAITING_PAUSE_ACKNOWLEDGE: + packet_type = get_http_server_response_header(mms); + break; + + case STREAMING: + case AWAITING_HTTP_PAUSE_CONTROL_ACKNOWLEDGE: + packet_type = get_http_server_response_data(mms, NULL); + break; + + case STREAM_DONE: + packet_type = SC_PACKET_TYPE_NO_DATA; + + default: + av_log(mms, AV_LOG_ERROR, + "Asking for packet in state %d\n", mms->state); + break; + } + + return packet_type; +}
as with the 2 above all uses of the state machine variables have to disappear
+ +/** Send a single HTTP request on an established socket. */ +static int send_request(MMSContext *mms, char *request) +{ + int length, res; + av_log(mms, AV_LOG_DEBUG, "Sending HTTP request:\n%s", request); + + length = strlen(request); + res = url_write(mms->mms_hd, request, length); + if(res != length) { + av_log(mms, AV_LOG_ERROR, "write failed, %d != %d\n", res, length); + ff_mms_set_state(mms, STATE_ERROR); + return res < 0 ? res : AVERROR(EIO); + } + return 0; +}
again, the ff_mms_set_state() has to disapear, the caller if it is the state machine can change the state based on the return value [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
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 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. Greetings, Reimar D?ffinger
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. -- 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
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.
Great to see you're still working on this!
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
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: [...]
Thanks for your speedy response. I'd like to ask for a few clarifications before I start on the next version of the patch.
+/** Private MMS data. */ +typedef struct {
+ const AVClass *av_class; ///< Context for logging.
this does belong in URLWhatever not in your private context
That would require an API change and a major version bump. Is that acceptable and wanted?
+ MMSProtocol *protocol; ///< Function pointers for the subprotocol. + void *priv_data; ///< Private data for subprotocol.
[...]
Iam also somewhat opposed to MMSProtocol *protocol. But that is rater minor.
With only one subprotocol, it doesn't make much sense. But with mmsh, mmst and possibly even mmsu I believe mms->protocol->foo(mms) is a cleaner solution than sprinkling the shared code with a bunch of if(mms->protocol == MMS_HTTP) mms_http_foo(mms) else if(mms->protocol == MMS_TCP) mms_tcp_foo(mms) Although I'm open for other suggestions.
+ + /** @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?
That is surely up to the calling application. IIRC mplayer does this for some reason.
+ /*@{*/ + 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.
The protocol needs to parse the streamed ASF header to get info about the available elementary streams. Is it ok (but still ugly) to call the ASF demuxer for this? The alternatives I can think of is to either reimplement a ASF header parser (very bad idea) or to directly call the private functions of the ASF demuxer. Your separate points about bad documentation and unneded context variables are taken and will be taken care of.
[...] 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 [].
The protocol needs the AVDiscard data from the application before it requests the multiplexed ASF stream from the server. I am attempting to do this transparently by getting a pointer to the application's demuxer context and the AVDiscard values in there and hope that the application sets the discard values between a read_header() and the first read() or play(). Getting only the AVDiscard[] pointer is slightly less ugly, but there's still a huge chance of fail. Better ideas are welcome. I can remove this functionality for now (as the default is to request all elementary streams from the server), but some kind of solution will be needed.
[...]
+ +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.
Which header would that be? "AVInputFormat asf_demuxer" is never declared in any header, and is only used once in allformats.c (with an extern declaration). -- 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
On Fri, Mar 07, 2008 at 01:48:08PM +0100, Bj?rn Axelsson wrote:
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: [...]
Thanks for your speedy response. I'd like to ask for a few clarifications before I start on the next version of the patch.
+/** Private MMS data. */ +typedef struct {
+ const AVClass *av_class; ///< Context for logging.
this does belong in URLWhatever not in your private context
That would require an API change and a major version bump. Is that acceptable and wanted?
no, :( but a AVClass under #ifdef VERSION >= next major could be added
+ MMSProtocol *protocol; ///< Function pointers for the subprotocol. + void *priv_data; ///< Private data for subprotocol.
[...]
Iam also somewhat opposed to MMSProtocol *protocol. But that is rater minor.
With only one subprotocol, it doesn't make much sense. But with mmsh, mmst and possibly even mmsu I believe
mms->protocol->foo(mms)
is a cleaner solution than sprinkling the shared code with a bunch of
if(mms->protocol == MMS_HTTP) mms_http_foo(mms) else if(mms->protocol == MMS_TCP) mms_tcp_foo(mms)
well, you still have if(mms->protocol == MMS_HTTP) mms->protocol->foo= mms_http_foo; else if(mms->protocol == MMS_TCP) mms->protocol->foo= mms_tcp_foo; The reason why i dont like these pointers is that it adds another layer of indirection a reader has to go through (where is that pointer set and to what before he can look up the code of the function)
Although I'm open for other suggestions.
+ + /** @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?
That is surely up to the calling application. IIRC mplayer does this for some reason.
If mms itself doesnt support it then i dont see why we should emulate this. just fail or return zero bytes (unless this really doesnt work ...)
+ /*@{*/ + 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.
The protocol needs to parse the streamed ASF header to get info about the available elementary streams. Is it ok (but still ugly) to call the ASF demuxer for this? The alternatives I can think of is to either reimplement a ASF header parser (very bad idea) or to directly call the private functions of the ASF demuxer.
Its ok to open a asf demuxer, parse the header, extract what you need and close it again. Its not ok to keep the AVFormatContext around to confuse me :) [...]
[...]
+ +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.
Which header would that be? "AVInputFormat asf_demuxer" is never declared in any header, and is only used once in allformats.c (with an extern declaration).
Once upon a time there was allformats.h ... well it seems somone killed it :( [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle
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
On Mon, Mar 10, 2008 at 03:47:06PM +0100, Bj?rn Axelsson wrote:
On Thu, 2008-03-06 at 22:44 +0100, Michael Niedermayer wrote: [...]
+ /*@{*/ + 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.
very good, this is a big improvment [...]
[...]
+/** 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.
I ve no good feeling about this, it looks like our code would only work with servers which workaround NSPlayer/4.1.0.3856 bugs. [...]
+/** startup/play/pause state as controlled by the application */ +typedef enum { + MMS_CTRL_STARTUP, ///<Streaming has not yet started. + MMS_CTRL_PLAY, ///<Streaming is currently active. + MMS_CTRL_PAUSE, ///<Streaming is currently paused. +} MMSControlState; + +/** unconnected/ok/end-of-file/error state of connection */ +typedef enum { + MMS_STREAM_UNCONNECTED, ///<Connection is not yet established. + MMS_STREAM_OK, ///<Everything is ok for streaming. + MMS_STREAM_EOF, ///<Stream has reached the end, otherwise ok. + MMS_STREAM_ERROR, ///<An unrecoverable error has occured. +} MMSStreamState;
Iam very alergic to your state machines. Error and eof are 2 completely seperate things they can both be true or just one or none. [...]
+/** @name Utility functions used by subprotocols. */ +/*@{*/ +/** Open or reopen (for http) the remote connection. */ +int ff_mms_open_connection(MMSContext *mms); +/** Convert from an AVStream's AVDiscard value to MMS stream switch code. */ +int ff_mms_get_switch_code(enum AVDiscard discard); +/** Get the next packet from the server. If the returned packet type is + * MMS_SC_PACKET_TYPE_ASF_MEDIA or MMS_SC_PACKET_TYPE_ASF_HEADER, the payload + * is available in mms->asf_buffer. */ +MMSSCPacketType ff_get_next_packet(MMSContext *mms); +/*@}*/
Please put an empty line between the functions this is just one big unreadable meatball. [...]
+/** Read at most one media packet (or a whole header). */ +static int read_packet(MMSContext *mms, uint8_t *buf, int buf_size)
document the return value
+{ + int size_to_copy; + +// av_log(mms, AV_LOG_DEBUG, +// "*** read packet %p needs %d bytes at %lld...\n", +// buf, buf_size, url_ftell(&mms->av_format_ctx->pb)); + for(;;) { + if(mms->stream_status == MMS_STREAM_EOF) + return 0; + else if(mms->stream_status == MMS_STREAM_ERROR) + return AVERROR(EIO);
+ else if(mms->asf_header_read_pos < mms->asf_header_length) { + /* Read from ASF header buffer */ + size_to_copy = FFMIN(buf_size, + mms->asf_header_length - mms->asf_header_read_pos); + memcpy(buf, mms->asf_header + mms->asf_header_read_pos, + size_to_copy); + mms->asf_header_read_pos += size_to_copy; + return size_to_copy; + } else if(mms->asf_buffer_length) { + /* Read from media packet buffer */ + size_to_copy = FFMIN(buf_size, mms->asf_buffer_length); + memcpy(buf, mms->asf_buffer_read_ptr, size_to_copy); + mms->asf_buffer_length -= size_to_copy; + mms->asf_buffer_read_ptr += size_to_copy; + return size_to_copy;
Why is the header not read into the same buffer as other packets? Above is code duplication and it also duplicates what fifo.c does. And above all the code is less readable than a fifo_read() call. Also a fifo has the advantage of being dynamically allocated and not limited to the randomly guessed max packet size. [...]
+/** Possible content-types in responses from the streaming server */ +typedef enum { + MMSH_ERROR = -1, ///< Input, parsing or HTTP error. + MMSH_NO_CONTENT, ///< Content-less response. + MMSH_X_MMS_FRAMED, /**< "application/x-mms-framed" - + ASF stream (with header) follows */ + MMSH_OCTET_STREAM, /**< "application/octet-stream" - + seen from older servers. Both header and + media stream follows. */ + MMSH_VND_MS_WMS_HDR_ASFV1, /**< "application/vnd.ms.wms-hdr.asfv1" - + an ASF header follows */ + MMSH_UNKNOWN, ///< Content-type specified but unknown +} MMSHContentType;
ERROR is not a content type, also what is the sense of this remapping? Why dont you just pass the string if its needed?
+ +/** Read a single HTTP response line. */
... and return it in buffer also document the return value [...]
+MMSProtocol ff_mmsh_mmsprotocol = { + .name = "mmsh", + .init = mmsh_init,
+ .send_startup_message = send_startup_request, + .play_from = request_streaming_from, + .get_server_response = get_http_packet,
Name them like .X = mmsh_X please. And when you are already at it dont hesitate to change them to better names. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus
participants (4)
-
andreas＠olebyn.nu -
bjorn.axelsson＠intinor.se -
michaelni＠gmx.at -
Reimar.Doeffinger＠stud.uni-karlsruhe.de