[FFmpeg-devel] [patch]add mmsh protocol and extract common code for mmst.c

[Ronald S. Bultje]

Hi,

On Mon, Aug 9, 2010 at 12:05 PM, zhentan feng <spyfeng at gmail.com> wrote:
> #9 adds mmsh.c
[..]
> +#define DEBUG

No. :-).

> +#define CHUNK_TYPE_DATA           0x4424
> +#define CHUNK_TYPE_ASF_HEADER     0x4824
> +#define CHUNK_TYPE_END            0x4524
> +#define CHUNK_TYPE_STREAM_CHANGE  0x4324

Do these mean anything? (If not, that's OK, just wondering...)

You could consider making CHUNK_TYPE_* an enum.

> +// mmsh request messages.
> +static const char* mmsh_first_request =
[..]
> +static const char* mmsh_seekable_request =
[..]
> +static const char* mmsh_live_request =

char[] would save 4-8 bytes for the actual pointer.

More importantly, each of these are used only once, so I'd prefer if
you'd just inline them where they are used.

> +typedef struct
> +{

No new line.

> +    char location[1024];

Only used in mmsh_open() and mmsh_open_cnx(), there's no reason to
store this in the struct. You could probably merge these two functions
as per mmst.c.

> +    int seekable;

What is this? It's never set.

+    int stream_num;

Seems a duplicate of MMSContext->stream_num.

> +    int request_seq;
> +    int chunk_seq;

Missing doxy.

> +static int get_chunk_header(MMSHContext *mmsh, int *len)
[..]
> +    res = url_read(mms->mms_hd, chunk_header, CHUNK_HEADER_LENGTH);
> +    if (res != CHUNK_HEADER_LENGTH) {
> +        dprintf(NULL, "read data packet  header failed!\n");
> +        return AVERROR(EIO);
> +    }

I think now would be a great time to introduce good error messages.
Please look at how I did it in mmst.c. The problem here is that a user
does ./ffplay mmsh://..., and it gives an "I/O error". There's no way
to debug what happened or do something useful with it. Please use
av_log() to make it a little easier to work with.

> +    if (chunk_type == CHUNK_TYPE_END ||chunk_type == CHUNK_TYPE_STREAM_CHANGE) {
> +        ext_header_len = 4;
> +    } else if (chunk_type == CHUNK_TYPE_ASF_HEADER || chunk_type == CHUNK_TYPE_DATA) {
> +        ext_header_len = 8;
> +    }
> +    if (ext_header_len) {
> +        res = url_read(mms->mms_hd, ext_header, ext_header_len);
> +        if (res != ext_header_len) {
> +            dprintf(NULL, "read ext header failed!\n");
> +            return AVERROR(EIO);
> +        }
> +    } else {
> +        dprintf(NULL, "strange chunk type %d\n", chunk_type);
> +        return -1;
> +    }

The following would sound more logical to me:

int ext_header; // no = 0
switch (chunk_type) {
case CHUNK_TYPE_END:
case CHUNK_TYPE_STREAM_CHANGE:
    ext_header_len = 4;
    break;
case ..:
    ext_header_len = 8;
    break;
default:
    av_log(!!!);
    return AVERROR(..);
}
res = url_read(..);
if (res != .. ) {
    av_log(..);
    return AVERROR(EIO);
}

> +    if (chunk_type == CHUNK_TYPE_END || chunk_type == CHUNK_TYPE_DATA)
> +        mmsh->chunk_seq = AV_RL32(ext_header);

Here, the if() is more logical, so don't change this one. ;-).

> +static int read_data_packet(MMSHContext *mmsh, const int len)
> +{
[..]
> +    res = url_read_complete(mms->mms_hd, mms->in_buffer, len);

Why do you use url_read() elsewhere and _complete() here? One of the
two is wrong, I think.

Are you checking in_buffer's length to prevent overflows?

> +static int get_http_header_data(MMSHContext *mmsh)
[..]
> +            res = url_read_complete(mms->mms_hd, mms->asf_header, len);

Same as above, overflow check?

> +                res = url_read_complete(mms->mms_hd, mms->in_buffer, len);

Same. It also seems like this is data skipping, could url_fskip() be
useful here?

> +        port = 80; // defaut mmsh protocol port

default.

> +    // send paly request

play.

> +static int handle_chunk_type(MMSHContext *mmsh)
> +{
> +    MMSContext *mms = &mmsh->mms;
> +    int res, len = 0;
> +    int chunk_type;
> +    chunk_type = get_chunk_header(mmsh, &len);
> +
> +    if(chunk_type == CHUNK_TYPE_END) {
> +        if (mmsh->chunk_seq == 0) {
> +            dprintf(NULL, "The stream is end.\n");
> +            return -1;
> +        }
> +        // reconnect
> +        mmsh->request_seq = 1;
> +        if ((res = mmsh_open_cnx(mmsh)) !=0) {
> +            dprintf(NULL, "Reconnect failed!\n");
> +            return res;
> +        }
> +    } else if (chunk_type == CHUNK_TYPE_STREAM_CHANGE) {
> +        mms->header_parsed = 0;
> +        if ((res = get_http_header_data(mmsh)) !=0) {
> +            dprintf(NULL,"stream changed! get new header failed!\n");
> +            return res;
> +        }
> +    } else if (chunk_type == CHUNK_TYPE_DATA) {
> +        return read_data_packet(mmsh, len);
> +    } else {
> +        dprintf(NULL, "recv other type packet %d\n", chunk_type);
> +        return -1;
> +    }

switch().

> +static int mmsh_read(URLContext *h, uint8_t *buf, int size)
[..]
> +        } else if (mms->remaining_in_len){
> +            res = ff_mms_read_data(mms, buf, size);
> +        } else {
> +             // read data packet from network
> +            res = handle_chunk_type(mmsh);
> +            if (res == 0) {
> +                res = ff_mms_read_data(mms, buf, size);
> +            } else {
> +                dprintf(NULL, "other situation!\n");
> +            }
> +        }

else {
    if (!mms->remaining_in_len && (res = handle_chunk_type(..)))
        return res;
    res = ff_mms_read_data(..);
}

Looks good in general, great work so far, let's get this in before SoC end!

Ronald