[FFmpeg-devel] [Patch] MMS protocol bug fixed

[Ronald S. Bultje]

Hi,

On Fri, Jul 16, 2010 at 11:41 AM, zhentan feng <spyfeng at gmail.com> wrote:
> 0: add a guid for asf header parser.
[..]
> +const ff_asf_guid ff_asf_stream_bitrate_properties = {
> +        0xce, 0x75, 0xf8, 0x7b, 0x8d, 0x46, 0xd1, 0x11, 0x8d, 0x82, 0x00, 0x60, 0x97, 0xc9, 0xa2, 0xb2
> +};

Is this still used? (See below.)

> 1: add time test data message for making it strictly with spec.

OK.

> 2: check the returned error code from the server side.
[..]
> +                    hr = AV_RL32(mms->in_buffer + 40);
> +                    if (hr)
> +                    {

Same line please ("if (..) {").

> 3: recv asf header sent by multiple packet
[..]
> @@ -335,6 +333,8 @@
>                                                   mms->remaining_in_len);
>                              mms->asf_header_size += mms->remaining_in_len;
>                          }
> +                        if (mms->incoming_flags == 0x04)
> +                            continue;
>                      } else if(packet_id_type == mms->packet_id) {
>                          packet_type = SC_PKT_ASF_MEDIA;
>                      } else {

Add a comment please what that means ("header split over multiple packets").

> @@ -472,6 +472,22 @@
>                  dprintf(NULL, "Too many streams.\n");
>                  return -1;
>              }
> +        } else if (!memcmp(p, ff_asf_head1_guid, sizeof(ff_asf_guid))) {
> +            chunksize = 46;

I don't quite see the point. Why do you force the chunksize for head1
GUIDs? Does that fix a bug? If so, which?

> +        } else if (!memcmp(p, ff_asf_stream_bitrate_properties, sizeof(ff_asf_guid))) {
> +            int record_cnt = AV_RL16(p + sizeof(ff_asf_guid) + 8);
> +            if (record_cnt*6 + 16 + 8 + 2 > chunksize) {
> +                dprintf(NULL, "Too many stream record count.\n");
> +                return -1;
> +            }
> +            if (mms->stream_num < MAX_STREAMS &&
> +                  46 + mms->stream_num * 6 < sizeof(mms->out_buffer)) {
> +                mms->stream_num = record_cnt;
> +                is_stream_num_known = 1;
> +            } else {
> +                dprintf(NULL, "Too many streams(bitrate properties)\n");
> +                return -1;
> +            }
>          }
>          p += chunksize;
>      }

This doesn't so anything other than setting mms->stream_num, which
only complicates the code (so this commit introduces a bug which you
fix in a next patch). What is the goal of this code? If only to set
stream_num, can't we just skip this, thus simplifying the next
patches?

This also doesn't compile because is_stream_num_known is undeclared.

For both chunks, how is this part of the "receive ASF header sent in
multiple packets" patch?

> 4: make the message 8 bytes aligned.

OK.

> 5: fix a possible bug when cann't read the asf header one time.

Should be in patch #3, most likely. OK otherwise.

> 6: fix a bug for send stream message select.

I'm hoping we don't need this if we don't apply the relevant chunk in #3...

Ronald