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

[Ronald S. Bultje]

Hi,

On Thu, Aug 12, 2010 at 1:26 PM, zhentan feng <spyfeng at gmail.com> wrote:
> On Thu, Aug 12, 2010 at 7:14 AM, Ronald S. Bultje <rsbultje at gmail.com>wrote:
>> On Mon, Aug 9, 2010 at 12:05 PM, zhentan feng <spyfeng at gmail.com> wrote:
>> > #9 adds mmsh.c
>>
>>
>> > +#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.
>
> the value has special meaning. I add comment for this.

You can still make them an enum:

enum {
    CHUNK_TYPE_DATA = 0x4424,
[etc]
};

That way the doxy output is nicer. You can also add some doxy to say
what a chunk_type is or so and it'll group them.

> I have fixed the code according to each reviewing item.
> please see the new patch for mmsh.c
[..]
> +// see Ref 2.2.3 for packet type define
> +#define CHUNK_TYPE_DATA           0x4424
> +#define CHUNK_TYPE_ASF_HEADER     0x4824
> +#define CHUNK_TYPE_END            0x4524
> +#define CHUNK_TYPE_STREAM_CHANGE  0x4324

Please make this an enum, as mentioned above.

> +// mmsh request messages.
> +static const char* mmsh_first_request =
> +    "Accept: */*\r\n"
> +    USERAGENT
> +    "Host: %s:%d\r\n"
> +    "Pragma: no-cache,rate=1.000000,stream-time=0,stream-offset=0:0,request-context=%u,max-duration=0\r\n"
> +    CLIENTGUID
> +    "Connection: Close\r\n\r\n";
> +
> +static const char* mmsh_live_request =
> +    "Accept: */*\r\n"
> +    USERAGENT
> +    "Host: %s:%d\r\n"
> +    "Pragma: no-cache,rate=1.000000,request-context=%u\r\n"
> +    "Pragma: xPlayStrm=1\r\n"
> +    CLIENTGUID
> +    "Pragma: stream-switch-count=%d\r\n"
> +    "Pragma: stream-switch-entry=%s\r\n"
> +    "Connection: Close\r\n\r\n";

As I mentioned before, you can inline those in the code where they're
used, since each is used only once.

> +static int get_chunk_header(MMSHContext *mmsh, int *len)
[..]
> +    switch (chunk_type) {
> +    case CHUNK_TYPE_END:
> +    case CHUNK_TYPE_STREAM_CHANGE:
> +        ext_header_len = 4;
> +        break;
> +    case CHUNK_TYPE_ASF_HEADER:
> +    case CHUNK_TYPE_DATA:
> +        ext_header_len = 8;
> +        break;
> +    default:
> +        av_log(NULL, AV_LOG_ERROR, "strange chunk type %d\n", chunk_type);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (ext_header_len > EXT_HEADER_LENGTH) {
> +        av_log(NULL, AV_LOG_ERROR, "ext_header_len = %d exceed the buffer size %d\n",
> +                    ext_header_len, EXT_HEADER_LENGTH);
> +        return AVERROR_INVALIDDATA;
> +    }

That can never happen, can it?

> +static int read_data_packet(MMSHContext *mmsh, const int len)
[..]
> +    int res, pad_size = 0;
[..]
> +    } else {
> +        pad_size = mms->asf_packet_len - len;
> +        memset(mms->in_buffer + len, 0, pad_size);
> +    }

Only used once and in local context, so you can remove pad_size as a variable.

> +static int get_http_header_data(MMSHContext *mmsh)
[..]
> +            if (res != len) {
> +                av_log(NULL, AV_LOG_ERROR, "recv asf header data len %d != %d", res, len);

Missing \n.

> +        } else {
> +            if (len) {
> +                if (len > sizeof(mms->in_buffer)) {
> +                    av_log(NULL, AV_LOG_ERROR, "other packet len = %d exceed the in_buffer size %d\n",
> +                                len, sizeof(mms->in_buffer));
> +                    return AVERROR_IO;
> +                }
> +                res = url_read_complete(mms->mms_hd, mms->in_buffer, len);
> +                if (res != len) {
> +                    av_log(NULL, AV_LOG_ERROR, "read other chunk type data failed!\n");
> +                    return AVERROR(EIO);
> +                } else {
> +                    dprintf(NULL, "skip chunk type %d \n", chunk_type);
> +                    continue;
> +                }
> +            }
> +        }

Did you try using url_fskip() here, as suggested earlier?

> +static int mmsh_open(URLContext *h, const char *uri, int flags)
[..]
> +    char stream_selection[10 * MAX_STREAMS];

People are trying to get rid of MAX_STREAMS, so you can't use it
anymore. I'm affraid you'll have to locally allocate it here and then
av_free() it after use.

Also, 10 isn't enough, look at the format, it's "ffff:%d:0 ", so
that's 11 characters max (2billion plus a sign symbol if it's
negative, which we don't check for), so this should be 19*streams+1
for terminating zero.

> +    snprintf (headers, sizeof(headers), mmsh_first_request,

Weird space here ^^.

> +    for (i = 0; i < mms->stream_num; i++) {
> +        err = snprintf(stream_selection + offset, sizeof(stream_selection) - offset,
> +                          "ffff:%d:0 ", mms->streams[i].id);
> +        if (err < 0)
> +            goto fail;
> +        offset += err;
> +    }

Please use av_strlcat(), and the last terminating space would ideally
be removed.

Ronald