Hi,

What version of Windows? Does it have an IPv6 stack installed?

I will research the issue, however, if memory serves me, it should have worked.

"It sounds like we might have to rethink how to handle this if my initial
findings are correct. If so, maybe we could make it so that the first client on
IPv4 results in an IPv4 socket being created, and vice versa for IPv6."

That will require too many changes, the best solution is to have two sockets, one for each protocol, when resolving the address we point the client to correct one.

Same problem on 32-bit XP and Vista, both had IPv6 installed and enabled on the interfaces.

Haven't got time to look at the code in detail right now, got an approaching deadline, but I'll sync up again over the weekend.

Created attachment 111376 [details] [review]
uses one socket for each IP version

"I will research the issue, however, if memory serves me, it should have worked."

I was wrong, too bad, we have to go the ugly way :(

This solution was in 1st version of the IPv6 patch, it was a bad idea to remove it after all. Note: I haven't tested this yet!

There's another possible solution, if we can use an IP4 address formated as ::FFFF:a.b.c.d on a AF_INET6 socket, we would instead of creating an aditional socket, store the type of socket we have, and tell gst_udp_get_addr about this, so it will format the address correctly and reject IPv6 address when the socket type is AF_INET.

Thoughts?

I have thought in a 3rd solution, use a socket object, something like:

struct gst_udp_socket {
  int sock4;
  int sock6;
};

int gst_udp_socket(struct gst_udp_socket* sobj, int family, int protocol)
{
  sobj->sock4 = socket (AF_INET, protocol, 0);
  sobj->sock6 = socket (AF_INET6, protocol, 0);
  ...
}

ssize_t gst_udp_sendto(struct gst_udp_socket *sobj, const void* buffer, size_t length, int flags, const struct socketaddr_storage *dst)
{
   switch (dst->ss_family)
   ...
}

...

Thoughts?

Created attachment 111405 [details] [review]
add support for windows limitation of AF_INET6 not allowing to send to AF_INET

I have tested the 1st and this new solution on Linux and it seems to work properly.

The patch attached improves the 1st solution with some ideas from the 3rd solution, I think this is the best aproach.

Thoughts?

"The patch attached improves the 1st solution with some ideas from the 3rd
solution, I think this is the best aproach."

Changed ideas, I will resubmit without messing with gstnetutils and without the GstUdpSocket thing...

Created attachment 111407 [details] [review]
add support for Windows not supporting sending throw AF_INET6 to AF_INET

This sould be the final patch, after being properly tested of course!

Tested and working on Linux. Only need confirmation for Windows(In reply to comment #8)
> Created an attachment (id=111407) [edit]
> add support for Windows not supporting sending throw AF_INET6 to AF_INET
> 
> This sould be the final patch, after being properly tested of course!
> 

Tested and working on Linux.
Ole André Vadla Ravnås can you test this for IP4 unicast and multicast and IP6 multicast and unicast?

After some research, I come to the conclusion that Windows is stupid to the point of not properly support Ipv6 dual stack mode.

Created attachment 111556 [details] [review]
fix for Windows no supporting IPv6 dual stack

Some cosmetic improvements to the previous fix.

*** Bug 578378 has been marked as a duplicate of this bug. ***

I looked into this a bit too - looks like Vista has a dual-stack that works properly, but XP doesn't. Given that we want to work on XP, we should probably do something like this (even though it's an ugly way to do things).

There are backwards-compatibility problems with it though...

(In reply to comment #11)
> Created an attachment (id=111556) [edit]
> fix for Windows no supporting IPv6 dual stack
> Some cosmetic improvements to the previous fix.

Not completely correct. Infact in function gst_udp_get_addr (file gstudpnetutils.c) you have ambiguity due to the fact that getaddrinfo could return two entry one with IPv6 address and one with IPv4 address.
The real case is "localhost" (that is the default host) that returns two struct addrinfo on Vista : the first one with AF_INET6 and the second one with AF_INET.
If you want to use IPv4 that fails since it returns IPv6 address (::1 instead of 127.0.0.1)

That depends if you pass the AF_UNSPECIEF o(In reply to comment #14)
> (In reply to comment #11)
> > Created an attachment (id=111556) [edit]
> > fix for Windows no supporting IPv6 dual stack
> > Some cosmetic improvements to the previous fix.
> 
> Not completely correct. Infact in function gst_udp_get_addr (file
> gstudpnetutils.c) you have ambiguity due to the fact that getaddrinfo could
> return two entry one with IPv6 address and one with IPv4 address.
> The real case is "localhost" (that is the default host) that returns two struct
> addrinfo on Vista : the first one with AF_INET6 and the second one with
> AF_INET.
> If you want to use IPv4 that fails since it returns IPv6 address (::1 instead
> of 127.0.0.1)
> 

That can be solved by passing a parameter specifying the family:

AF_UNSPEC -> Any
AF_INET   -> IPv4
AF_INET6  -> IPv6

(In reply to comment #14)
> Not completely correct. Infact in function gst_udp_get_addr (file
> gstudpnetutils.c) you have ambiguity due to the fact that getaddrinfo could
> return two entry one with IPv6 address and one with IPv4 address.
> 

That should not be an issue because we should expect that the address given should be preferably an IP address and not a host name. Keep in mind that an host name can be resolved to multiple addresses and udpsink offers no interface to handle this properly. However there should be an option to disable IPv6.

I will revise the patch later.

Created attachment 132545 [details] [review]
IPv6 dual-stack workaround

Updated patch for the git repository

Created attachment 132553 [details] [review]
IPv6 dual-stack workaround

Some fixes to the previous patch

(In reply to comment #16)
> (In reply to comment #14)
> > Not completely correct. Infact in function gst_udp_get_addr (file
> > gstudpnetutils.c) you have ambiguity due to the fact that getaddrinfo could
> > return two entry one with IPv6 address and one with IPv4 address.
> > 
> That should not be an issue because we should expect that the address given
> should be preferably an IP address and not a host name. Keep in mind that an
> host name can be resolved to multiple addresses and udpsink offers no interface
> to handle this properly. However there should be an option to disable IPv6.
> I will revise the patch later.

Actually , the default "host" property of multiudpsink is "localhost" and not "127.0.0.1", and this is resolved on Vista/2008 Server with two different address "::1" and "127.0.0.1". In my PC the first is returned (AF_INET6) by getaddrinfo and that is why a patch is needed to gst_udp_get_addr (as you suggested before).
Regarding multiple address you can simply keep it that way (choose the first address with the same family).
Thanks in advance

Then we need a property for udpsink client to specified the desired family and leave it at default of AF_UNSPEC. I will provide another patch for that later.

(In reply to comment #19)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > Not completely correct. Infact in function gst_udp_get_addr (file
> > > gstudpnetutils.c) you have ambiguity due to the fact that getaddrinfo could
> > > return two entry one with IPv6 address and one with IPv4 address.
> > > 
> > That should not be an issue because we should expect that the address given
> > should be preferably an IP address and not a host name. Keep in mind that an
> > host name can be resolved to multiple addresses and udpsink offers no interface
> > to handle this properly. However there should be an option to disable IPv6.
> > I will revise the patch later.
> 
> Actually , the default "host" property of multiudpsink is "localhost" and not
> "127.0.0.1", and this is resolved on Vista/2008 Server with two different
> address "::1" and "127.0.0.1". In my PC the first is returned (AF_INET6) by
> getaddrinfo and that is why a patch is needed to gst_udp_get_addr (as you
> suggested before).
> Regarding multiple address you can simply keep it that way (choose the first
> address with the same family).
> Thanks in advance
> 

Or even better, we give priority to IPv4 over IPv6 in gst_udp_get_addr. Anyway, address should be preferably an IP and not an hostname, so a guess this will the best approach.

Comments?

(In reply to comment #20)
> Then we need a property for udpsink client to specified the desired family and
> leave it at default of AF_UNSPEC. I will provide another patch for that later.
> 
> (In reply to comment #19)
> > (In reply to comment #16)
> > > (In reply to comment #14)
> > > > Not completely correct. Infact in function gst_udp_get_addr (file
> > > > gstudpnetutils.c) you have ambiguity due to the fact that getaddrinfo could
> > > > return two entry one with IPv6 address and one with IPv4 address.
> > > > 
> > > That should not be an issue because we should expect that the address given
> > > should be preferably an IP address and not a host name. Keep in mind that an
> > > host name can be resolved to multiple addresses and udpsink offers no interface
> > > to handle this properly. However there should be an option to disable IPv6.
> > > I will revise the patch later.
> > 
> > Actually , the default "host" property of multiudpsink is "localhost" and not
> > "127.0.0.1", and this is resolved on Vista/2008 Server with two different
> > address "::1" and "127.0.0.1". In my PC the first is returned (AF_INET6) by
> > getaddrinfo and that is why a patch is needed to gst_udp_get_addr (as you
> > suggested before).
> > Regarding multiple address you can simply keep it that way (choose the first
> > address with the same family).
> > Thanks in advance
> > 
> 

Created attachment 132668 [details] [review]
give priority to IP4 in gst_udp_get_addr

Created attachment 132670 [details] [review]
udp win32 dual-stack alternate

Note: none of these latest patches have been tested, not even compiled!

Comment on attachment 132670 [details] [review]
udp win32 dual-stack alternate

the GetVersionEx(&info) call is missing

Created attachment 135135 [details] [review]
Handle dual-stack vs. two-stack IPv4/IPv6 in multiudpsink better

This patch does the following:
 - Attempts to portably detect if we have a dual-stack system (linux, mac, winvista)
 - If we do, use that as we used to.
 - Adds a bunch of fixes for dual-stack mode - e.g. you must add v4 address as v4-mapped v6 addresses on mac (and vista, I think)
 - If not dual-stack capable, fall back to using two sockets. 
 - Add properties for dealing with the extra socket

So on platforms (linux, mostly) where the current implementation works, this should do the same thing - no changes to the API, etc.

On mac and vista, the API should remain the same, but it should now work.

On older windows, if you want ipv6 support, you need to deal with two sockets, but most apps won't care, because they're not using the properties anyway.


I think this is a good compromise (unlike the previously proposed patches, this doesn't change API unconditionally)

Adding wim to CC list; I'd like him to review this.

Also note: this needs more testing on windows, I can test on XP but I don't have vista. Can anyone else on this bug help out with this?

Just wanted to update this ticket that using 0.10.16 on OSX this same problem as originally described exists. Only ipv6 addresses work. I reversed the order of the types on sockets from ipv6 then ipv4 to ipv4 then ipv6 and it now works. I realize that is not really a fix at all, but I just wanted to mention that I see the same broken behavior of OSX 10.5.8

Created attachment 150916 [details] [review]
MikeS' patch for IPv6, tested on mac os x x86_64 Snow Leopard only.

Tested on Mac OS X x86_64 Snow Leopard only.

Created attachment 155910 [details] [review]
Fixes for udp on windows vista / 7

Hi all

I have attached a patch that applies to current git from the patch above and from https://bugzilla.gnome.org/show_bug.cgi?id=604870

I now have working udp traffic on my windows 7 machine.

It needs to be tested on linux and windows XP to make sure I didn't break anything when re-arranging / merging patches based off old code.

I would appreciate it if someone could review how the ss_family is assigned to the sink in the gst_multiudpsink_init_send function, to make sure i didn't miss a case / how it should be handled. The rest of the code is pretty much as is from the other patches.

I'll do the test on Windows XP and I'll probably find a Vista machine too

I hereby confirm that the patch also seems to work on Mac OS X 10.5.8 (Leopard) as well. That is, this works (IPv4):

gst-launch filesrc location=/dev/urandom ! udpsink host=127.0.0.1
gst-launch udpsrc ! fakesink dump=1

And this works (lPv6):

gst-launch filesrc location=/dev/urandom ! udpsink
gst-launch udpsrc uri="udp://[::1]:4951" ! fakesink dump=1

Apparently, udpsrc opens an IPv4 socket by default, while udpsink opens a socket to localhost by default (which evaluates to ::1 (IPv6) on my machine).

But, hey, it is working again after quite some time!

after doing a:

git clone git://anongit.freedesktop.org/gstreamer/gst-plugins-good

and trying to apply the patch (the last one from Richard Spiers):

git apply --check fix_win_udp.patch

i get a:

error: patch failed: gst/udp/gstmultiudpsink.c:829
error: gst/udp/gstmultiudpsink.c: patch does not apply

any hint on what im doing wrong? (im not very experienced with git, but seens that all what i had to do is this)

im stuck on the same problem and looking forward to resolv it :-). Im trying to make the udpsink work on windows XP, if someone can help me apply the patch i can confirm if it is working properly (im using it to build a voip stream on windows).

> git apply --check fix_win_udp.patch
> error: patch failed: gst/udp/gstmultiudpsink.c:829
> error: gst/udp/gstmultiudpsink.c: patch does not apply
> 
> any hint on what im doing wrong? (im not very experienced with git, but seens
> that all what i had to do is this)

Try:  git am foo.patch

(yes, not very intuitive.)

didn't worked :-(

git am fix_win_udp.patch

Applying: Fixes for udp on windows vista/7, bugs 604870 and 534243
/home/katcipis/gst-plugins-good/.git/rebase-apply/patch:80: trailing whitespace.
  
/home/katcipis/gst-plugins-good/.git/rebase-apply/patch:114: trailing whitespace.
	
/home/katcipis/gst-plugins-good/.git/rebase-apply/patch:160: trailing whitespace.
#endif 
/home/katcipis/gst-plugins-good/.git/rebase-apply/patch:162: trailing whitespace.
  
/home/katcipis/gst-plugins-good/.git/rebase-apply/patch:246: trailing whitespace.
    
error: gst/udp/gstmultiudpsink.c: does not match index
error: gst/udp/gstmultiudpsink.h: does not match index
error: gst/udp/gstudpnetutils.c: does not match index
error: gst/udp/gstudpnetutils.h: does not match index
error: gst/udp/gstudpsrc.c: does not match index
Patch failed at 0001 Fixes for udp on windows vista/7, bugs 604870 and 534243
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Sjoerd got me the source files already patched, i tried to compile them and them i got a problem, the following names where not defined:

AI_V4MAPPED
AI_ALL
IPV6_V6ONLY

i don't know if it is because i dont have some ipv6 files installed on my mingw environment, with some research i defined these values on hand to ( actually a friend/co worker found those values for me :-) ):

#define IPV6_V6ONLY    27
#define AI_V4MAPPED    8
#define AI_ALL        10

now I'm being able to send and receive audio on windows trough udp. I tested the following pipes one running on one machine and other running on another machine and worked fine, both machines where windows XP:

Send audio:
gst-launch-0.10.exe -v directsoundsrc ! audioconvert ! audioresample ! audio/x-raw-int,signed=true,
channels=1,rate=8000,width=16,endianness=1234 ! alawenc ! rtppcmapay ! gstrtpbin ! udpsink host=[IP] port=5000

Receive audio (direct sound dont work properly at 8000hz):
gst-launch-0.10.exe -v udpsrc port=5000 auto-multicast=0 caps=application/x-rtp,media=audio,payload=96,clock-rate=8000,encoding-name=PCMA ! gstrtpbin ! rtppcmadepay ! alawdec ! audioresample ! audioconvert ! audio/x-raw-int,signed=true,width=16,depth=16,rate=44100,channels=1 ! directsoundsink

*** Bug 602312 has been marked as a duplicate of this bug. ***

Tested on Windows XP/Vista/7

(In reply to comment #29)
> 
> I have attached a patch that applies to current git from the patch above and
> from https://bugzilla.gnome.org/show_bug.cgi?id=604870

You left a print in gst_udp_set_ttl() :)
printf("Sockfd is %d, ss_family is %d, ttl is %d, is_multicast is %d\n",sockfd,ss_family,ttl,is_multicast);

Modifying title, since it happens on both windows and macosx.

Richard, could you provide a patch that applies against current head of -good ? Several changes have gone into master since your patch.

Mike, you assigned this patch to yourself. Do you still intend to fix it ?

Like I said in the bug: I want someone to do some code review, and I want someone to test on vista. I gave up on finding people to do either after several months of trying.

Created attachment 164898 [details] [review]
Ignore ttl errors on win32

I did test it on XP, Vista and Windows 7. In the last release of WinBuilds I included this patch and many people has been using it successfully, except on Windows XP where in some cases setting the ttl option throws an error, but ignoring it made the element at least usable (see attached patch).
I can try to rebase everything and double check on XP/Vista/Windows7.

(In reply to comment #42)
> Created an attachment (id=164898) [details] [review]
> Ignore ttl errors on win32
> 
> I did test it on XP, Vista and Windows 7. In the last release of WinBuilds I
> included this patch and many people has been using it successfully, except on
> Windows XP where in some cases setting the ttl option throws an error, but
> ignoring it made the element at least usable (see attached patch).
> I can try to rebase everything and double check on XP/Vista/Windows7.

i used it on a small voip pipeline on windows xp (sending/receiving audio using rtp) and i can confirm that works fine (i didnt used the ttl option, cant confirm anything about that).

Reopening as I guess the requested patch has been provided.

Could someone update the patch to 0.11 if necessary? The UDP plugin (and the TCP plugin for that matter) now use GIO for all networking stuff, I don't know if something is still necessary there or if it simply works now. Also it might make sense to fix this in GIO to provide the same behaviour on all platforms.

Bruno, Michael, Richard, Andoni, ping, does anyone get chance to update patches ?

Today I've assessed the patches from Mike, Uday, and Richard. Mike did the bulk of the initial work, Uday posted a replacement for Mike's patch that includes consideration for Mac, and Richard posted a replacement for Uday's patch that includes consideration for Windows. That is, these are cumulative patches. From what I can tell, each new patch seems to add the things it needs without disturbing what was present in the previous. I cannot say with 100% certainty that they don't break each other, this is just a gut feeling after reading each patch side-by-side and line-by-line.

Since the patches don't apply cleanly to the current git anymore, I don't think it's worth trying to sort out the incrementals. Instead, Richard's patch should be adapted to current git based on its intent. I plan to do this soon.

So, how do we proceed here? Shall we reject patches based on comment #47 and assign this bug to Justin?

I think the issue may still persist, but the patches probably all need work, since this code was re-written based on top of libgio's network stack for 1.0. There are some investigations going on, let's leave this bug open for now, the investigation and patches might still be useful for forensic purposes. I'll mark it as NEW for now.

i have same error
one guy wrote that in 64bit os the size of everything is twice larger. but not sure if correct.

also i suggest to default to ipv4. which is most in use in routers today

The proposed solution is to always use separate sockets for IPv4 and IPv6. Just needs someone to actually implement it :)

seems like it is already implemented in the patches,
what is wrong, what is missing?

It has to be ported to GSocket and made to apply against latest GIT master.

This should be fixed now, by default an IPv4 and IPv6 socket is created and depending on the address family of the destination one or another is used for sending.

The application can provide a socket to the element via the "socket" property (which can be IPv4 or IPv6 or anything), and an IPv6 socket additionally via the "socket-v6" property. If there's only an IPv6 socket provided, IPv4 destinations will be send over the IPv6 socket.

commit b1af93f791909c883f0da97b784d3e03e810bdcf
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Thu Apr 25 12:12:23 2013 +0200

    (multi)udpsink: Use separate sockets for IPv4 and IPv6
    
    https://bugzilla.gnome.org/show_bug.cgi?id=534243

commit 0b552150ceaf67a899f4e17b4d0fed74585b9e62
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Thu Apr 25 10:44:44 2013 +0200

    dynudpsink: Use separate sockets for IPv4 and IPv6
    
    https://bugzilla.gnome.org/show_bug.cgi?id=534243

Comment on attachment 164898 [details] [review]
Ignore ttl errors on win32

Something like this is in 1.0 already, it will be ignored independent of the OS

Comment on attachment 132668 [details] [review]
give priority to IP4 in gst_udp_get_addr

We always use the first result of the resolver, I don't think there's a reason to prefer one over another.

Comment on attachment 132553 [details] [review]
IPv6 dual-stack workaround

Something like this is committed now, just rewritten from scratch because of the port to GIO/GSocket

*** Bug 696057 has been marked as a duplicate of this bug. ***

*** Bug 630894 has been marked as a duplicate of this bug. ***