[FFmpeg-devel] [PATCH] Realmedia / RTSP (RDT)

[Michael Niedermayer]

On Fri, Dec 28, 2007 at 04:19:55PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> attached patch is my latest for $subj. The flags/nostatic patches are
> required before the main patch (rtsp-realmedia.patch), they're kind of
> logical, and the reindent should be done afterward. With the patch, one can
> receive RTSP streams from Helix/Real servers.
> 
> One thing it does wrong is stream selection, if anyone could help me with
> that (hint, code, anything), it'd be greatly appreciated. 

Elaborate on the problem please, if its about selecting one stream of several
options, see AVFormatContext.discard


> The second thing
> people may have concerns about is the fact that I expose us as "realplayer"
> in the client-id during the OPTIONS command. I can try a naked (client-id:
> "ffmpeg" or so) OPTIONS and use this as a fallback if that fails if people
> prefer, but I don't really know what people's opinions are re: that. If
> there's anything else wrong, please let me know. My final intent is still to
> get this in the ffmpeg tree. :-).

Please use LIBAVFORMAT_IDENT if it works if not use whatever is needed.
Either way do not add code to use one and in case of failure use another.


[...]

> -                av_new_packet(pkt, len);
> -                memcpy(pkt->data, buf, len);
> -            }
> +            av_new_packet(pkt, len);
> +            memcpy(pkt->data, buf, len);

cosmetic


[...]
> +static void
> +real_calc_response_and_checksum(char *response, char *chksum, char *challenge)
> +{

Document via doxygen please at least the sizes needed for the output arrays.


> +    int ch_len, i;
> +    unsigned char zres[16], buf[128];
> +#define XOR_TABLE_SIZE 37
> +    static const unsigned char xor_table[XOR_TABLE_SIZE] = {
> +        0x05, 0x18, 0x74, 0xd0, 0x0d, 0x09, 0x02, 0x53,
> +        0xc0, 0x01, 0x05, 0x05, 0x67, 0x03, 0x19, 0x70,
> +        0x08, 0x27, 0x66, 0x10, 0x10, 0x72, 0x08, 0x09,
> +        0x63, 0x11, 0x03, 0x71, 0x08, 0x08, 0x70, 0x02,
> +        0x10, 0x57, 0x05, 0x18, 0x54 },
> +    hex_table[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
> +        '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
> +
> +    /* initialize return values */
> +    memset(response, 0, 64);
> +    memset(chksum, 0, 34);
> +
> +    /* initialize buffer */
> +    memset(buf, 0, 128);
> +    AV_WB32(buf, 0xa1e9149d);
> +    AV_WB32(buf+4, 0x0e6b3b59);

uint8_t buf[128]= [0xA1, 0xE9, 0x14, ..., 0x59};


> +
> +    /* some (length) checks */
> +    if (challenge != NULL) {

This check is unneeded.


> +        ch_len = strlen (challenge);
> +

> +        if (ch_len == 40) /* what a hack... */ {
> +            challenge[32]=0;
> +            ch_len=32;
> +        }

Please provide a comment explainig what and why this is done.
If it is a hack in the sense of lazyness of the author its rejected!

Also i dont think writing into challenge is clean!


> +        if (ch_len > 56) ch_len=56;
> +

> +        /* copy challenge to buf */
> +        memcpy(buf+8, challenge, ch_len);

Thank you for the explanation but every c devel knows that memcpy copies.
Comments should say whats not obvious from the code. Not repeat the already
obvious!

Also all rdt code should be in seperate files, not rtsp.c/rtp.c where possible


[...]
> Index: ffmpeg/libavformat/rtsp.h
> ===================================================================
> --- ffmpeg.orig/libavformat/rtsp.h	2007-12-28 14:55:28.000000000 -0500
> +++ ffmpeg/libavformat/rtsp.h	2007-12-28 14:57:37.000000000 -0500
> @@ -58,7 +58,7 @@
>      int64_t range_start, range_end;
>      RTSPTransportField transports[RTSP_MAX_TRANSPORTS];
>      int seq; /**< sequence number */
> -    char session_id[512];
> +    char session_id[512], challenge[64];

;\n please


[...]


>  
> +    if (s->real_data_type) {
> +        static uint32_t prev_ts = -1, prev_sn = -1; // FIXME

yes you have to fix this


> +        if (buf[0] != 0x40 && buf[0] != 0x41 && buf[0] != 0x42) {

< 0x40 || > 0x42


[...]
> +typedef struct _rdt_data {

No initial underlines please! We are not a system library.


[...]
> +// return 0 on packet, no more left, 1 on packet, 1 on partial packet...
> +static int

doxy, parse error


[...]

> +        if (*p == '\"') { p++; len -= 2; };

Try to use the enter key please!


> +        rdt->header_data_size = len * 6 / 8;

Simplify fraction please.


> +        rdt->header_data = av_malloc(rdt->header_data_size +
> +            FF_INPUT_BUFFER_PADDING_SIZE);
> +        tmp = av_malloc (len + 1);

> +        strncpy(tmp, p, len);
> +        tmp[len] = '\0';

av_strlcpy()


> +        av_base64_decode(rdt->header_data, tmp, rdt->header_data_size);
> +        av_free (tmp);

char tmp[len + 1];


> +    } else if (av_strstart(p, "RMFF 1.0 Flags:buffer;", &p)) {
> +        int len = strlen(p);
> +        if (*p == '\"') { p++; len -= 2; }
> +        rdt->flags_data_size = len * 6 / 8;
> +        rdt->flags_data = av_malloc(rdt->flags_data_size +
> +            FF_INPUT_BUFFER_PADDING_SIZE);
> +        tmp = av_malloc (len + 1);
> +        strncpy(tmp, p, len);
> +        tmp[len] = '\0';
> +        av_base64_decode(rdt->flags_data, tmp, rdt->flags_data_size);
> +        av_free (tmp);
> +    }

this looks somehow similar to the previous, i guess you can factor some code
out ...


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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20071230/772427c8/attachment.pgp>