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

Luca Abeni lucabe72
Sat Jan 9 21:07:15 CET 2010


Hi Martin,

On 09/01/10 20:12, Martin Storsj? wrote:
[...]
> 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.
Yes, the problem was installing it... DSS wants to mess with /usr/ so I 
decided to install it in a virtual machine (and here the fun begun: 
finding a small-enough distribution that can support DSS, installing all 
the needed packages, adding users by hand because of bugs in DSS install 
script, etc...). Maybe it's time for a lighter, libav-based server ;-)


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

Ah, right... I'll try to see if the code can be modified so that 
rtpctx->priv_data is filled only when the header is written.


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

Right... I did not think about this.


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

Well, there currently are no other chained muxers, but I think in the 
generic case we want to av_new_stream() and then to initialise the 
AVCodecContext by copying the parameters from the "previous" codec in 
the chain. For example, some weeks ago I was trying to chain a LATM 
muxer with an RTP muxer: in this case, the codec in the RTP muxer is not 
exactly identical to the codec in the LATM muxer (the codec ID changes).

Maybe, it's worth doing the same also in this case?

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

I would say that the chained muxer here looks ok, but maybe I am biased. 
I am also convinced that some of the ugliness is due to how the RTSP 
(de)muxer is structured, and this can be improved... But I am not sure 
until I can try some tests (hopefully tomorrow).

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

But you point out some other problems that I did not consider... I'll 
have a better look at this tomorrow.


			Thanks,
				Luca



More information about the ffmpeg-devel mailing list