[FFmpeg-devel] [PATCH/RFC] sdp/udp: Refactoring of code, don't require a ?multicast option

[Luca Abeni]

Hi Martin,

On 10/05/2010 03:37 PM, Martin Storsj? wrote:
> Hi,
>
> Currently, the sdp code requires the url to have a ?multicast option in
> order to produce a proper multicast SDP, even if none of the other
> protocol code requires such an option. The udp code checks the actual IP
> address to see if it is a multicast address or not.

It's done like this because one of the original goals was to make the SDP
functions independent from the networking functions (so that they can be
reused in other projects, the SDP generator can be used even when networking
is not configured in ffmpeg, etc...). So, I wanted to avoid calling
is_multicast_address().

I've also been tempted to remove the multicast check from sdp.c, leaving to
the caller the responsibility to properly set the TTL... But this might end
up allowing to generate a lot of broken SDPs, so I am not sure if it's a
good idea ;-)

Anyway, the original goal changed (or, I failed in achieving it), so I
think that the change you propose is reasonable.

> In order for the sdp code to be able to do the same, some code needs to be
> moved out of udp.c for sdp.c to reuse it.
>
> General question: What would a suitable place for the is_multicast_address
> be? We don't have any network.c yet, should we add one?

Not sure... Maybe we need a file for the network "helper functions".


> We have a bunch of
> network code in os_support.c, but that's only fallbacks for missing system
> functions, not normal code as such. Patch #2 moves this to network.h,
> which probably isn't the right place.

I leave the answer to this question to others... :)
is_multicast_address() is small enough, so having it as inline in network.h
might not be that bad...


> And a question more precisely to Luca A: The solution in patch #3 isn't
> all that pretty, but to be able to make it prettier, sdp_get_address and
> resolve_destination would have to be merged. Would you be ok with that?

Ok, I can see the problem... Let me think about it a little bit (yes, adding
a "ttl" parameter to resolve_destination() is not pretty... But I'd still
like to have all the "network dependent" things in one single place...).


				Luca