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

[Ronald S. Bultje]

Hi,

On Dec 29, 2007 7:43 PM, Michael Niedermayer <michaelni at gmx.at> wrote:

> On Fri, Dec 28, 2007 at 04:19:55PM -0500, Ronald S. Bultje wrote:
> > 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
>

Basically same as MMS, being able to select one bitrate for a stream, e.g.
the server can stream video at 8 bitrates, but only 1 can be selected, and
one has to be selected when we open the stream in the server. I'm not
exactly sure how it was decided this should be fixed for MMS, but I'll have
a look (i.e. I didn't fix this one yet).


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


Then the challenge key negotiation fails. I left it as-is for now, since
that works.


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


Done.


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


I moved the function to rtp_rm.c (with declaration in rtp_internal.h).


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


Both fixed.


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


I'm not exactly sure, the response/checksum calculation function is from the
(LGPL) live555 project. However, I removed the writing into challenge and
just made it use length, I hope that partially resolves the problem.


> > +        /* 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!


Removed.


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

Fixed.


> > +    if (s->real_data_type) {
> > +        static uint32_t prev_ts = -1, prev_sn = -1; // FIXME
>
> yes you have to fix this


Oops, sorry. I added a function parse_header to the dynamic payload handler
context, similar to parse_packet, and filled that for the RTP
implementations. Then the prev_sn/ts are moved into the rdt_data struct, so
they're no longer static.


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


Fixed.


> > +typedef struct _rdt_data {
>
> No initial underlines please! We are not a system library.


Fixed.


> > +// return 0 on packet, no more left, 1 on packet, 1 on partial
> packet...
> > +static int
>
> doxy, parse error


Fixed.


> > +        if (*p == '\"') { p++; len -= 2; };
>
> Try to use the enter key please!


Fixed (here and several other occurrences).


> > +        rdt->header_data_size = len * 6 / 8;
>
> Simplify fraction please.


Fixed.


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


I factored it out, and the memcpy didn't appear to be needed, so I did it
without, so the above comments no longer apply.

New patch (again, w/o the AVStream.discard implemented) attached, I hope it
resolves most other issues. Second patch is the reindent after the first.

Ronald
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rtsp-realmedia.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080101/f375f343/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rtsp-realmedia-reindent.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080101/f375f343/attachment.asc>