[FFmpeg-devel] [PATCH][0/4]: MMS base and MMSH implementation
Michael Niedermayer
michaelni
Fri Jan 25 04:16:55 CET 2008
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
-------------- 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/20080125/5cb80145/attachment.pgp>
More information about the ffmpeg-devel
mailing list