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

[Ronald S. Bultje]

Hi Zhentan,

On Tue, Aug 3, 2010 at 11:49 AM, zhentan feng <spyfeng at gmail.com> wrote:
> #1 patch is extracted common code from mmst.c into mms.c and mms.h to be
> used by mmsh.c

- MMSContext should be embedded in MMSTContext, it shouldn't be a
pointer as it is now. If you need a pointer, use &mmst->mms.
- don't call the variable (in functions) mmst_ctx. Just mmst is fine.
We know it's a context from the type. If you call it mms, you get the
added benefit of not having to change each function call and function
header, so I'd go for that.
- There is no reason why you would move the definition in mmst.c,
right now you're moving it, causing every line within MMS.Context to
appear in the diff. There is no reason for this.
- ff_mms_*() should be a separate patch.
- send_media_packet_request() and clear_stream_buffers() are moved,
that should be separated

> -    int len= mms->write_out_ptr - mms->out_buffer;
> +    MMSContext *mms  = mmst_ctx->ff_ctx;
> +    int len          = mms->write_out_ptr - mms->out_buffer;

Cosmetic.

> -    int size = mms->write_out_ptr - mms->out_buffer;
>      int len;
> +    MMSContext *mms = mmst_ctx->ff_ctx;
> +    int size        = mms->write_out_ptr - mms->out_buffer;

Cosmetic.

> -    int first_length= exact_length - 16;
> -    int len8= first_length/8;
> +    int first_length = exact_length - 16;
> +    int len8         = first_length/8;

Cosmetic.

> -
> +    MMSContext *mms = mmst_ctx->ff_ctx;

Cosmetic (the first line).

> -            tmp                       = AV_RL16(mms->in_buffer + 6);
> -            length_remaining          = (tmp - 8) & 0xffff;
> -            mms->incoming_packet_seq  = AV_RL32(mms->in_buffer);
> -            packet_id_type            = mms->in_buffer[4];
> -            mms->incoming_flags       = mms->in_buffer[5];
> +            tmp                            = AV_RL16(mms->in_buffer + 6);
> +            length_remaining               = (tmp - 8) & 0xffff;
> +            mmst_ctx->incoming_packet_seq  = AV_RL32(mms->in_buffer);
> +            packet_id_type                 = mms->in_buffer[4];
> +            mmst_ctx->incoming_flags       = mms->in_buffer[5];

Some of these are cosmetic.

>      if (!mms->asf_packet_len || !mms->stream_num)
>          goto fail;
>
> +    /* Since we read the header at open(), this shouldn't be possible */
> +    assert(mmst_ctx->ff_ctx->header_parsed);
> +
> +    dprintf(NULL, "mms_read() before play().\n");
> +    clear_stream_buffers(mmst_ctx);
> +    err = mms_safe_send_recv(mmst_ctx, send_stream_selection_request, SC_PKT_STREAM_ID_ACCEPTED);
> +    if (err)
> +        goto fail;
> +    // send media packet request
> +    err = mms_safe_send_recv(mmst_ctx, send_media_packet_request, SC_PKT_MEDIA_PKT_FOLLOWS);
> +    if (err) {
> +        goto fail;
> +    }
> +
>      dprintf(NULL, "Leaving open (success)\n");
>      return 0;
>  fail:
[..]
> -    MMSContext *mms = h->priv_data;
> -    int result = 0;
> -
> -    /* Since we read the header at open(), this shouldn't be possible */
> -    assert(mms->header_parsed);
> -
> -    if (!mms->is_playing) {
> -        dprintf(NULL, "mms_read() before play().\n");
> -        clear_stream_buffers(mms);
> -        result = mms_safe_send_recv(mms, send_stream_selection_request, SC_PKT_STREAM_ID_ACCEPTED);
> -        if (result)
> -            return result;
> -        // send media packet request
> -        result = mms_safe_send_recv(mms, send_media_packet_request, SC_PKT_MEDIA_PKT_FOLLOWS);
> -        if (result) {
> -            return result;
> +    MMSTContext *mmst_ctx = h->priv_data;
> +    int result            = 0;
> +    MMSContext *mms       = mmst_ctx->ff_ctx;
> +    do {
> +        if(mms->asf_header_read_size < mms->asf_header_size) {
> +           result =  ff_mms_read_header(mms, buf, size);
> +        } else if(mms->remaining_in_len) {
> +            /* Read remaining packet data to buffer.
> +             * the result can not be zero because remaining_in_len is positive.*/
> +            result = ff_mms_read_data(mms, buf, size);
> +        } else {
> +            /* Read from network */
> +            int err = mms_safe_send_recv(mmst_ctx, NULL, SC_PKT_ASF_MEDIA);
> +            if (err == 0) {
> +                if(mms->remaining_in_len>mms->asf_packet_len) {
> +                    av_log(NULL, AV_LOG_ERROR,
> +                           "Incoming pktlen %d is larger than ASF pktsize %d\n",
> +                           mms->remaining_in_len, mms->asf_packet_len);
> +                    result= AVERROR_IO;
> +                } else {
> +                    // copy the data to the packet buffer.
> +                    result = ff_mms_read_data(mms, buf, size);
> +                    if (result == 0) {
> +                        dprintf(NULL, "read asf media paket size is zero!\n");
> +                        break;
> +                    }
> +                }
> +            } else {
> +                dprintf(NULL, "read packet error!\n");
> +                break;
> +            }
>          }
> -    }
> -    return read_mms_packet(mms, buf, size);
> +    } while(!result); // only return one packet.
> +    return result;
>  }

What's this?

> #2 and #3 patch is for mmsh configuration.
> #4 is mmsh protocol implementation.

You can probably combine these, they're interdependent.

Ronald