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

[zhentan feng]

Hi

On Fri, Aug 13, 2010 at 6:00 AM, Ronald S. Bultje <rsbultje at gmail.com>wrote:

> 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.
>
>

fixed.

> > 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.
>
>

fixed.

> > +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?
>
>

yes, removed.


> > +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.
>
>

removed.

> > +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.
>
>

fixed.

> > +        } 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.
>
>

modified.

> > +    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.
>
>
fixed.
please see the new patch.thanks!

zhentan
-- 
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mmsh_817.patch
Type: application/octet-stream
Size: 13602 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100817/be327698/attachment.obj>