[FFmpeg-devel] Realmedia patch

Luca Abeni lucabe72
Sat Sep 13 17:08:28 CEST 2008


Hi Ronald,

some comments:
Ronald S. Bultje wrote:
[...]
> So I've tried an approach as in $attached. It's a hacky patch but sort
> of gets the point accross.
> 
> - it implements a RTSPPacketType, which is currently just RTP or RDT.
> This is used alongside the RTSPProtocol (UDP, TCP, multicast) to set
> up a SETUP request. The response Transport: is used for packet
> parsing.

Why RTSPPacketType and packet_type? According to rfc2326, this is called
"transport". The thing called RTSPProtocol by libavformat is what rfc2326
calls "transport/profile/lower-transport".
It looks like your patch wants to split "transport" and "lower-transport"
in two different variables. If so, you should use the correct names and
types, and probably refactor the RTSP code a little bit before adding
other modifications (for example, having an "RTSP_PROTOCOL_RTP_UDP"
RTSPProtocol contraddicts this approach)


> - I had to split (temporarily) ff_rdt_subscribe_to_rule() (so I added
> rule2()) to fit a new API where I try to get rdt_data out of
> non-RTPDynamicPayloadHandler vfuncs. After multirules.patch, this
> function disappears, so please disregard that for now, it's a hack and
> intended that way because it'll disappear

Ok; I am not looking at it. But please, do it in a separate patch.


> - rdt_data is still in ff_rdt_parse_packet() but will disappear from
> there too.

Ok


> My idea here is that apparently RDT packets can contain
> non-RM media

Yes, this is my understanding too.


> (and thus the IsRealDataType may well refer to the RM
> content *after* the RDT or RTP packet header)

We should really undestand what "IsRealDataType" is

> but what I'd like to do
> is use the RTPDynamicPayloadHandler.parse_packet() vfunc (which has
> access to rdt_data) the way I did in my very first submission many
> months ago, and use ff_rdt_parse_packet() as a publicly accessible
> function which calls dynpayhdnl.parse_packet() after parsing the RDT
> packet handler.

This makes sense. But then you should decouple the "payload handling"
function from RTP (right now, the code assumes that this function is
going to parse the payload of an RTP packet).


> - I renamed RTSP_SERVER_RDT to _REAL because it's not RDT-specific

This is ok, but it must go in a separate patch.


> Sorry, lots of notes, and I know the patch contains whitespace
> cleanups and reindents etc, I won't commit like this, I'd just like
> some comments on the general approach before I get it in a committable
> shape.

I think you can commit the RTSP_SERVER_RDT -> RTSP_SERVER_RTSP hunk.
This will simplify the patch, and make it easier to review.



			Thanks,
				Luca




More information about the ffmpeg-devel mailing list