[FFmpeg-devel] Realmedia patch

[Michael Niedermayer]

On Thu, Aug 21, 2008 at 12:46:52AM -0400, Ronald S. Bultje wrote:
> 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.

ok, then this seems to be needed ...


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

rm.h is fine


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

what about putting AVInputFormat rdt_demuxer in rmdec.c ?


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

hmmmm
finding 2 matching sets of rules is a matter of strcmp() or what am i
missing?


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/5a3fdde8/attachment.pgp>