[FFmpeg-devel] rtsp.c: a=rtpmap: parsing implementation?

[Luca Abeni]

Hi Ronald,

Ronald S. Bultje wrote:
[...]
>>> You'll see the a=rtpmap: line is applied to all RTSPStreams, not just
>>> the current one.
>> I did not look at the full code, but according to the code you pasted
>> above the line is not applied to all the streams. It is only applied to
>> the stream with the specified payload type (and the for() loop should
>> just be used to search for the correct stream).
>> And this looks consistent with the RFC.
> 
> Aha, so the RFC says this is OK... So here's a hypothetical SDP (or
> see [1] for a real one) that would "open twice" with this:
> 
> m=audio 0 RTP/AVP 96
> a=rtpmap:96 x-asf-pf/1000
> m=audio 0 RTP/AVP 96
> a=rtpmap:96 x-asf-pf/1000
> [.. this can repeat several times ..]

Ok... So the SDP contains two identical streams. I do not know what the 
standards say about this, but I guess this situation was not considered 
when the original code was written. The "dynamic payload" stuff in the 
current rtsp.c might be broken (it considers the payload <-> codec 
relation as valid in the whole SDP, while the SDP you posted seems to 
indicate that is a "per media" relation).


> I suppose according to the RFC, omitting the second rtpmap would be
> fine, right?

I do not know... Maybe it is needed to indicate that for the second 
media payload type 96 is still ASF.


> do you think we could come up with a SDP parsing
> implementation that supports both without opening the RTP parser
> (sdp_parse_rtpmap()) N times for the stream represented by m= line 0?

I do not know. My impression is that the original demuxer was designed 
with different assumptions in mind... Now you are hacking it to support 
new features, but this introduces new problems and inconsistencies. As I 
previously said, I think that it would be better to re-design it 
(starting from the RFCs and then adding the needed extensions).


> The patch below (left in the reply for convenience) probably isn't
> right to support both, it basically switches from supporting
> preferentially "your" SDP to supporting only "my" SDP, which is worse.

I think that there should not be "my SDP" or "your SDP"... We should 
have a single type of SDP that is generic enough to support all the 
usage cases. It seems to me that in this particular case your patch is 
ok for all the possible SDPs... No?
And I still believe that if you go this path the "if 
(rtsp_st->sdp_payload_type == payload_type) {" should be removed... 
(this also applies to the patch you just posted).

Anyway, I am leaving all this stuff to the maintainer.


				Luca