[vlc-devel] [PATCH] Fix for SAP mishandling of duplicates via different addresses

[Rémi Denis-Courmont]

   Hello,

On Wed, 19 Oct 2011 14:21:56 +0100, Alexander Clouter <alex at digriz.org.uk>
wrote:
> We broadcast a number of IPTV channels at my organisation and want the
> channels available to internal and nearby external users. As a result
> we announce these channels via SAP on *both* 239.255.255.255
> (SAP_V4_LOCAL_ADDRESS) and 239.195.255.255 (SAP_V4_ORG_ADDRESS)...

I really don't see the point, when you could use the largest scope for
everyone.

> VLC does not like this too much.


> The function modules/services_discovery/sap.c::ParseSAP() has an
> IsSameSession() check that overlooks the possibility that the same
session
> could arrive via different SAP listener sockets and as a result rewrites
> the entire SAP playlist once every 30 seconds

If the two announces are actually identical and VLC should match them and
not rewrite anything. This is not what your patch does.

Or they are different and IsSameSession() should not match them,
irrelevant of the multicast group they were received from. Your patch does
not do that either.

> (we announce at a period of
> five seconds and your 'trust' cycle of six leads to 30 seconds).

This sounds more like a bug in the time/trust logic, that I never really
understood.

> Once rewritten, any channel being viewed is terminated.

This is a known bug in the playlist code. Feel free to fix it.

> This repeats every thirty seconds, getting annoying, real quickly. :)
> 
> The solution is to additionally track the SAP listener socket that the
> packet arrived on, and only go through the 'trust' codepath if it comes
in
> on the same socket; ignoring duplicates (packets that match the
conditions
> of IsSameSession) arriving via different sockets.
> 
> I am pretty sure this is a safe fix as nothing in RFC2974 (section 5,
> 'Session Modification') implies that broadcasting the same SDP payload
on
> different SAP addresses is incorrect; hopefully I have not missed any
> clauses ;)

Sure, but your patch seems to contradict that by actively fighting this.

-- 
Rémi Denis-Courmont
http://www.remlab.net/