[FFmpeg-devel] [PATCH] Add an RTSP muxer

[Martin Storsjö]

On Sat, 9 Jan 2010, Luca Abeni wrote:

> I finally have some time for having a better look at your patches, and try
> them... I have no DSS installation, so I cannot test the RTSP muxer right now
> (I am installing a DSS in a virtual machine, so I hope to be able to test the
> muxer in a short time).

Thanks for taking the time to look at it!

IIRC, using this doesn't require too much config of the DSS - after 
getting it up and running, you should be able to send streams to it using 
this muxer, and then just connect to the same URL with a player.

> In the meanwhile, here are some untested comments on the code (I just quote
> the modified code, not the exact patch):
> 1) You do something like
> +                if (rt->is_output) {
> +                     AVFormatContext *rtpctx = rtsp_st->transport_priv;
> +                     /* Don't call av_write_trailer if av_write_header
> +                      * hasn't been called yet. Use ->priv_data to check
> +                      * for this. */
> +                     if (rtpctx->priv_data)
> +                         av_write_trailer(rtpctx);
> Maybe this "if" can be avoided? (what happens if we try to call
> av_write_trailer() anyway? I think it will just fail, no? Otherwise, can we
> ensure that transport_priv is not set if av_write_header has not been called?)

If av_write_trailer is called on an AVFormatContext that hasn't had 
av_write_header called yet, bad things may happen. In case of the RTP 
muxer, priv_data is null and rtp_write_trailer dereferences it. (The same 
applies for most other muxers, too, av_write_trailer mustn't be called if 
av_write_header hasn't been called.)

> 2) I think the "rtp_pb" field of RTSPStream is unneeded, right?

Yes, that's one of the patches that can be dropped if this approach is 
chosen.

> 3) Instead of doing
> +            if (!av_new_stream(rtpctx, 0)) {
> +                err = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> and
> +            /* Remove the local codec, link to the original codec
> +             * context instead, to give the rtp muxer access to
> +             * codec parameters. */
> +            av_free(rtpctx->streams[0]->codec);
> +            rtpctx->streams[0]->codec = st->codec;
> Cannot you simply do
> 	rtpctx->streams[0] = st;
> 	rtpctx->nb_streams = 1;

No, unfortunately not. There's a lot of internal logic within 
av_write_frame (or specifically, within compute_pkt_fields2) that update 
parameters within AVStream. The packet has been fed through this function 
once when passed from the user into rtsp_write_packet, and if using the 
exact same AVStream object, we'd try to write the same packet once again 
into the same AVStream, resulting in "non monotone timestamps" errors, 
among others.

Exactly this point would have been good to check how it's done with other 
chained muxers, had there been examples of such. :-)

> 4) In rtsp_write_packet(), fd_max looks useless

Probably yes, those parts are copied from rtsp_read_packet without too 
much thought.

> 5) In rtsp_write_packet(), before invoking av_write_frame() you call
> av_rescale_q() to convert the timebase. I think this can be avoided by setting
> the timebase of the RTSP's AVStream equal to the timebase of the RTP muxer's
> AVStream. Basically, after invoking av_write_header() for the RTP muxer you
> can properly set the timebase for the corresponding RTSP AVStream.

Hmm, that's a good idea, I'll have to test it.

Generally, is the amount of ugliness due to using chained muxers small 
enough for you to prefer this version over the initial one using an 
internal muxer interface?


> Also, I am wondering if the initialisation of the RTP Muxer's AVFormatContext
> can be done in rtsp_open_transport_ctx() (instead of initialising it earlier
> and only calling av_write_header() in rtsp_open_transport_ctx()). Basically,
> in this way the SDP would be created based on the RTSP AVFormatContext (as
> your initial patch did), and the RTP muxers would be initialised only later.
> I am not sure if this approach would simplify the code or not...

That would ease at least some things, yes. Then all the creation of the 
chained RTP muxers and their AVFormatContext could happen at the correct 
place, within rtsp_open_transport_ctx.

As you pointed out in some earlier comment, creating SDPs based on URLs 
starting with anything else than rtp:// should probably be disallowed, I 
thought the more correct approach would be to generate the SDP from the 
RTP muxers, instead of from the RTSP context and its AVStreams. But if 
you're OK with creating the SDP based on the RTSP context and URL, I think 
this can be a bit simplified.

That also implies that the hostname->IP conversion needed for the SDP 
either has to be in the SDP creator (as in my patch), or we need to set 
AVFormatContext->filename to some temporary URL containing numeric IPs 
while creating the SDP, and then later reverting to the correct one again.

Having the resolve within the SDP creator is a good safeguard IMO, but 
that patch should at least wait until the getaddrinfo/getnameinfo stuff 
from my other thread is applied, so that I can clean it up to use more 
flexible APIs.

// Martin