[FFmpeg-devel] Realmedia patch
Michael Niedermayer
michaelni
Thu Aug 21 06:05:09 CEST 2008
On Mon, Aug 18, 2008 at 10:21:35AM -0400, Ronald S. Bultje wrote:
> Hi,
>
> I lost count of how many I've done, but since I didn't receive review
> on the last ones I thought I'd just post whatever svn diff gives me
> after my recent updates.
>
> I recently got stream selection to work up to a level where ffplay
> rtsp://realmediastream -ast X works for whatever value X. Currently,
> taking ffplay rtsp://... -stats is the best way to figure out the
> streams, it reads the MDPR (rm) header to fill out the fields (bitrate
> field is not in here, so that field is currently missing, I can add
> that from the ASM rulebook later but that'd complicate the patch...).
>
> Dynamic stream switching ('a' key in ffplay) doesn't work but I
> haven't really looked if servers support that. If they don't (which I
> expect), it'd mean having to re-setup a RTSP stream, which may be
> slightly overkill, especially since people want to rewrite the RTSP
> demuxer anyway. Probably not worth it, at least for now.
>
> Is anyone willing to go help me get this in svn? Preferrably one of
> the not-so-busy RTSP people? I can also separate the original RTSP
> patch from the stream-selection patch if preferred.
>
> Ronald
[...]
> /**
> * @returns 0 on success, <0 on error, 1 if protocol is unavailable.
> */
> static int
> -make_setup_request (AVFormatContext *s, const char *host, int port, int protocol)
> +make_setup_request (AVFormatContext *s, const char *host, int port,
> + int protocol, RealSetupRequestData *real_data)
> {
I think a const char *real_challenge would be enough no RealSetupRequestData
seems needed
[...]
> + else rule_nr++;
> + }
> + }
> +
> + ff_rdt_subscribe_for_bandwidth(
trailing whitespace
[...]
> @@ -1263,24 +1403,25 @@
>
> av_log(s, AV_LOG_DEBUG, "hello state=%d\n", rt->state);
>
> + if (!(rt->real_stream && rt->no_streams_chosen_yet)) {
> + if (rt->state == RTSP_STATE_PAUSED) {
> - if (rt->state == RTSP_STATE_PAUSED) {
> + snprintf(cmd, sizeof(cmd),
> + "PLAY %s RTSP/1.0\r\n",
> + s->filename);
> - snprintf(cmd, sizeof(cmd),
> - "PLAY %s RTSP/1.0\r\n",
> - s->filename);
> + } else {
> - } else {
cosmetics ...
[...]
> +static int
> +rdt_parse_sdp_line (AVFormatContext *s, int stream_index,
> + void *d, const char *line)
> +{
> + AVStream *orig_stream = s->streams[stream_index];
> + rdt_data *rdt = d;
> + const char *p = line;
> +
> + if (av_strstart(p, "OpaqueData:buffer;", &p)) {
> + ByteIOContext *pb = rdt->rmctx->pb;
> + int n_streams, n_hdrs, i;
> + AVStream *stream;
> +
> + rdt->header_data = rdt_parse_b64buf(&rdt->header_data_size, p);
> +
> + /* read full MLTI/mdpr chunk */
> + url_open_buf (&pb, rdt->header_data, rdt->header_data_size, URL_RDONLY);
> + rdt->rmctx->pb = pb;
> + if (get_le32 (pb) != MKTAG ('M', 'L', 'T', 'I'))
> + return -1;
> + n_streams = get_be16(pb);
> + for (i = 0; i < n_streams; i++) {
> + if (i == 0)
> + stream = orig_stream;
> + else {
> + stream = av_new_stream (s, 0);
> + stream->codec->codec_type = orig_stream->codec->codec_type;
missing stream != NULL check
besides, this stream creation in parse_sdp_a_line() is something that
should be commented by one of the lucas. I do not feel confident enough
that this is the correct approuch ...
[...]
> +extern AVInputFormat rm_demuxer;
shouldnt that be in some header?
> +static void *
> +rdt_new_extradata (void)
> +{
> + static AVInputFormat rdt_demuxer = {
> + "rdt",
> + NULL_IF_CONFIG_SMALL("RDT demuxer"),
> + sizeof(RMContext),
> + NULL, NULL, NULL, NULL, NULL, NULL
> + };
> + rdt_data *rdt = av_mallocz (sizeof (rdt_data) +
> + FF_INPUT_BUFFER_PADDING_SIZE);
> +
> + if (!rdt_demuxer.read_close)
> + rdt_demuxer.read_close = rm_demuxer.read_close;
> + av_open_input_stream(&rdt->rmctx, NULL, "", &rdt_demuxer, NULL);
> + rdt->first = 1;
> + rdt->prev_ts = -1;
> + rdt->prev_sn = -1;
> +
> + return rdt;
> +}
i do not know what this is supposed to do but it does not look very
much like it would be the cleanest way for it.
the way read_close is set alone is scary ...
So please elaborate ...
[...]
> +void
> +ff_rdt_calc_response_and_checksum(char *response, char *chksum, char *challenge)
the arguments should be const as appropriate and look like chksum[123] so
the size of the arrays is clearly known
> +{
> + int ch_len = strlen (challenge), i;
> + unsigned char zres[16],
> + buf[128] = { 0xa1, 0xe9, 0x14, 0x9d, 0x0e, 0x6b, 0x3b, 0x59 };
> +#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' };
> +
> + /* some (length) checks */
> + if (ch_len == 40) /* what a hack... */
> + ch_len = 32;
> + else if (ch_len > 56)
> + ch_len = 56;
> + memcpy(buf + 8, challenge, ch_len);
> +
> + /* xor challenge bytewise with xor_table */
> + for (i = 0; i < XOR_TABLE_SIZE; i++)
> + buf[8 + i] ^= xor_table[i];
> +
> + av_md5_sum(zres, buf, 64);
> +
> + /* convert zres to ascii string */
> + for (i = 0; i < 16; i++) {
> + response[i * 2] = hex_table[zres[i] >> 4];
> + response[i * 2 + 1] = hex_table[zres[i] & 0xf];
> + }
grep hex */*.c shows that there are several existing data->hex converters
thus this is duplicate
[...]
> +static int
> +rdt_fit_asm_cond (asm_rulebook_expression *expr, int bandwidth)
> +{
> + switch (expr->cond) {
> + case CONDITION_EQ:
> + return bandwidth == expr->val;
> + case CONDITION_NEQ:
> + return bandwidth != expr->val;
> + case CONDITION_GT:
> + return bandwidth > expr->val;
> + case CONDITION_GEQ:
> + return bandwidth >= expr->val;
> + case CONDITION_LT:
> + return bandwidth < expr->val;
> + case CONDITION_LEQ:
> + return bandwidth <= expr->val;
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +rdt_fit_asm_rule (asm_rulebook_rule *rule, int bandwidth)
> +{
what is all this stuff good for? (keep in mind i know little about
rtp&rtsp&rdt&sdp, iam just reviewing because noone else is ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20080821/497c2e0d/attachment.pgp>
More information about the ffmpeg-devel
mailing list