[FFmpeg-devel] [PATCH][0/4]: MMS base and MMSH implementation

Michael Niedermayer michaelni
Mon Mar 10 22:26:26 CET 2008


On Mon, Mar 10, 2008 at 03:47:06PM +0100, Bj?rn Axelsson wrote:
> On Thu, 2008-03-06 at 22:44 +0100, Michael Niedermayer wrote:
[...]
> 
> > > +    /*@{*/
> > > +    uint8_t *asf_header;            ///< ASF header buffer.
> > > +    int asf_header_size;            ///< Size of stored ASF header.
> > > +    int asf_header_read_pos;        ///< Header read offset. See read_packet().
> > > +    int header_parsed;              ///< Flag: header has been fully parsed.
> > > +    AVFormatContext private_av_format_ctx; ///< Private header data (generic).
> > > +    ByteIOContext   private_av_format_pb;  ///< Private header data (IO buf).
> > > +    ASFContext      asf_context;           ///< Private header data (ASF).
> > 
> > private of what? we have an asf demuxer (which you shouldnt touch) a 
> > subprotocol (mmsh), this mms stuff, an underlaying protocol (tcp/http)..
> > is tha asf stuff here the one actually demuxing things or some private
> > seperate thing unrelated to the demuxer used to help avoid code duplication.
> > All this should be clearly documented.
> > 
> > And after looking at the code, half of the above look like local variables
> > which shouldnt be in the context.
> 
> Point taken, and the stored contexts have been replaced with the
> extracted data.

very good, this is a big improvment


[...]
> > [...]
> > > +/** We must lie and say that we are NSPlayer, or the server will give us a
> > > + * redirector file (video/x-ms-wvx) instead of the actual stream. Also, it
> > > + * seems that the server will use different protocol variants depending on
> > > + * the reported client version. */
> > > +#define USERAGENT "NSPlayer/4.1.0.3856"
> > > +// #define USERAGENT "NSPlayer/7.1.0.3055"
> > 
> > What is different?
> 
> I honestly don't know. That alternative USERAGENT comes from the very
> first versions of the code, and I have no idea what would be different.
> I simply removed it for now.

I ve no good feeling about this, it looks like our code would only work with
servers which workaround NSPlayer/4.1.0.3856 bugs.


[...]
> +/** startup/play/pause state as controlled by the application */
> +typedef enum {
> +    MMS_CTRL_STARTUP, ///<Streaming has not yet started.
> +    MMS_CTRL_PLAY,    ///<Streaming is currently active.
> +    MMS_CTRL_PAUSE,   ///<Streaming is currently paused.
> +} MMSControlState;
> +
> +/** unconnected/ok/end-of-file/error state of connection */
> +typedef enum {
> +    MMS_STREAM_UNCONNECTED, ///<Connection is not yet established.
> +    MMS_STREAM_OK,          ///<Everything is ok for streaming.
> +    MMS_STREAM_EOF,         ///<Stream has reached the end, otherwise ok.
> +    MMS_STREAM_ERROR,       ///<An unrecoverable error has occured.
> +} MMSStreamState;

Iam very alergic to your state machines. Error and eof are 2 completely
seperate things they can both be true or just one or none.


[...]
> +/** @name Utility functions used by subprotocols. */
> +/*@{*/
> +/** Open or reopen (for http) the remote connection. */
> +int  ff_mms_open_connection(MMSContext *mms);
> +/** Convert from an AVStream's AVDiscard value to MMS stream switch code. */
> +int  ff_mms_get_switch_code(enum AVDiscard discard);
> +/** Get the next packet from the server. If the returned packet type is
> + * MMS_SC_PACKET_TYPE_ASF_MEDIA or MMS_SC_PACKET_TYPE_ASF_HEADER, the payload
> + * is available in mms->asf_buffer. */
> +MMSSCPacketType ff_get_next_packet(MMSContext *mms);
> +/*@}*/

Please put an empty line between the functions this is just one big
unreadable meatball.


[...]
> +/** Read at most one media packet (or a whole header). */
> +static int read_packet(MMSContext *mms, uint8_t *buf, int buf_size)

document the return value


> +{
> +    int size_to_copy;
> +
> +//    av_log(mms, AV_LOG_DEBUG,
> +//            "*** read packet %p needs %d bytes at %lld...\n",
> +//            buf, buf_size, url_ftell(&mms->av_format_ctx->pb));
> +    for(;;) {
> +        if(mms->stream_status == MMS_STREAM_EOF)
> +            return 0;
> +        else if(mms->stream_status == MMS_STREAM_ERROR)
> +            return AVERROR(EIO);

> +        else if(mms->asf_header_read_pos < mms->asf_header_length) {
> +            /* Read from ASF header buffer */
> +            size_to_copy = FFMIN(buf_size,
> +                    mms->asf_header_length - mms->asf_header_read_pos);
> +            memcpy(buf, mms->asf_header + mms->asf_header_read_pos,
> +                    size_to_copy);
> +            mms->asf_header_read_pos += size_to_copy;
> +            return size_to_copy;
> +        } else if(mms->asf_buffer_length) {
> +            /* Read from media packet buffer */
> +            size_to_copy = FFMIN(buf_size, mms->asf_buffer_length);
> +            memcpy(buf, mms->asf_buffer_read_ptr, size_to_copy);
> +            mms->asf_buffer_length -= size_to_copy;
> +            mms->asf_buffer_read_ptr += size_to_copy;
> +            return size_to_copy;

Why is the header not read into the same buffer as other packets?
Above is code duplication and it also duplicates what fifo.c does.
And above all the code is less readable than a fifo_read() call.
Also a fifo has the advantage of being dynamically allocated and
not limited to the randomly guessed max packet size.


[...]
> +/** Possible content-types in responses from the streaming server */
> +typedef enum {
> +    MMSH_ERROR = -1,            ///< Input, parsing or HTTP error.
> +    MMSH_NO_CONTENT,            ///< Content-less response.
> +    MMSH_X_MMS_FRAMED,          /**< "application/x-mms-framed" -
> +                                     ASF stream (with header) follows */
> +    MMSH_OCTET_STREAM,          /**< "application/octet-stream" -
> +                                     seen from older servers. Both header and
> +                                     media stream follows. */
> +    MMSH_VND_MS_WMS_HDR_ASFV1,  /**< "application/vnd.ms.wms-hdr.asfv1" -
> +                                     an ASF header follows */
> +    MMSH_UNKNOWN,               ///< Content-type specified but unknown
> +} MMSHContentType;

ERROR is not a content type, also what is the sense of this remapping?
Why dont you just pass the string if its needed?


> +
> +/** Read a single HTTP response line. */

... and return it in buffer
also document the return value


[...]
> +MMSProtocol ff_mmsh_mmsprotocol = {
> +    .name                 = "mmsh",
> +    .init                 = mmsh_init,

> +    .send_startup_message = send_startup_request,
> +    .play_from            = request_streaming_from,
> +    .get_server_response  = get_http_packet,

Name them like .X = mmsh_X please.
And when you are already at it dont hesitate to change them to better
names.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080310/f6ddfdd3/attachment.pgp>



More information about the ffmpeg-devel mailing list