[FFmpeg-devel] Realmedia patch

[Ronald S. Bultje]

Hi Michael,

I'll first address your questions so we can review further before
sending new patches:

On Thu, Aug 21, 2008 at 12:05 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Aug 18, 2008 at 10:21:35AM -0400, Ronald S. Bultje wrote:
>> +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 ...

So this is indeed kind of, uhm, well, hackish maybe :]. The idea is
that the SDP tells us which bitrates (stream rules) are possible for
this single RTSP stream. Per stream, you can then subscribe to these
stream-rules which you can read. I'm currently selecting one (I don't
think many is practically possible anyway). The number of rules is
available only in the SDP, so I need to create the additional
AVStreams per extra stream-rules somewhere during/after the SDP
reading.

Better approaches welcome, of course.

[..]
>> +extern AVInputFormat rm_demuxer;
>
> shouldnt that be in some header?

Would you like me to create an allformats.h, or is rm.h enough?

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

This vfunc is for private data allocation/initialization in the RTP
dynamic stream-id modules (see also rtp_h264.c). Since this one uses
rmdec.c, I'm trying to use the rmdec.c functions. I don't need the
rmdec.c read_header, since the only part of the RM header that is in
the RTP stream is the MDPR header, and that particular sub-function
was made public in rmdec.c. The rest of the data is allocated during
data-reading or is part of RMContext and allocated during
av_open_input_stream(). For deallocation of this data is done in
rmdec_read_close(), so that function needs to be accessed. I can
either make it non-static or use this hack, I decided to do the hack.
I can use either approach.

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

The ASM rulebook contains stream restrictions, usually just bitrate.
This can be used for things like bitrate selection of stream rules
within a RTP stream. OK, that could be as easy as just creating N
AVStreams and selecting the AVDISCARD_DEFAULT one, but that doesn't
work in practice: In the RTP stream I'm using as a practice (BBC or
so), rules are repeated twice. Selecting one results in an incomplete
stream that fails to be decoded. You need to parse the ASM rules,
select the first *and* second one that share the same parameters (e.g.
same bitrate, according to the ASM rulebook) and subscribe to *both*.
Then, the incoming data can be decoded. I don't know why. The above
code helps in ASM rulebook parsing so that all this can be done. (I
also plan to use the ASM rulebook data to set
AVCodecContext:min/max/avg_bitrate per streamrule, but that's not done
yet.)

I'll make the other modifications as suggested, thanks for the initial review.

Ronald