[PATCH] IPv6 support v.2

[Ronald S. Bultje]

Hi all,

after my previous udp patches, here's the updated patches to convert all of
ffmpeg over to the IPv6 protocol. I've tested it over TCP and UDP and
everything works as expected and as before (that is, given that you have the
patches from the previous thread to make udp work applied also).

ffmpeg-avinetaddr-ipv6.patch - add IPv6 host resolution and info API, this
deprecates resolve_host() (ipv4-only). Also converts tcp.c over to the new
API.
ffmpeg-ipv6-udp.patch - converts udp.c over to the new API, also removes all
duplicated code for CONFIG_IPV6 except for the multicase setup.
ffmpeg-ipv6-ffserver.patch - ports ffserver over to the new API - it's big
because ffserver is a lot of code.
ffmpeg-ipv6-rtsp.patch - ports the rtsp code (rtsp.c, rtp.c, sdp.c) over to
the new API.
ffmpeg-remove-resolve_host.patch - removes inet_aton() from the public API
(no longer needed) and removes resolve_host() (replaced by
inetaddr_resolve()).

Please test, review and apply, they're in a state where it's worth keeping
them (and it'd make my tree diff a lot smaller :-) ). I can also send each
one in a separate thread if you think that makes it easier, as long as they
go in.

Ronald

[Luca Abeni]

Hi Ronald,

Ronald S. Bultje wrote:
[...]

Post by Ronald S. Bultje
ffmpeg-ipv6-rtsp.patch - ports the rtsp code (rtsp.c, rtp.c, sdp.c) over to
the new API.

The sdp.c hunk looks ok, and I'll apply as soon as the required infrastructure
is in.
There is no change to rtp.c, right?
I am not the RTSP maintainer, but I had a look at the rtsp.c part. It looks
ok, but it contains things like
-#ifdef DEBUG
+//#ifdef DEBUG
that should be removed from the patch.

Post by Ronald S. Bultje
Please test, review and apply, they're in a state where it's worth keeping
them (and it'd make my tree diff a lot smaller :-) ).

I'll try to perform some tests after lunch.


Luca

[Ronald S. Bultje]

Hi,

Post by Ronald S. Bultje
ffmpeg-avinetaddr-ipv6.patch - add IPv6 host resolution and info API, this
deprecates resolve_host() (ipv4-only). Also converts tcp.c over to the new
API.

This API needed a small enhancement (new flag) to handle the address family
problem discussed in the rtp/ipv6 thread elsewhere. Updated patch attached.

AVInetAddr is a replacement API for resolve_host(), which is limited to
IPv4. AVInetAddr is fully IPv6-compatible. The API is similar to
resolve_host(), which means that relatively minor changes are needed to port
protocols over to the new API and become IPv6-compatible. The API consists
of:
- AVInetAddr: sort of a container for struct addrinfo, works on both
ipv4-only and ipv6-capable systems
- inetaddr_resolve: replaces resolve_host(), can either parse IPv4/6 names
w/o host resolution or resolve hosts on a name server
- inetaddr_alloc: for those cases where you need an empty AVInetAddr, e.g.
for use in accept()
- inetaddr_str: basically inet_ntop() and inet_ntoa().
- there's also a bunch of macros to directly access (set/get) the
port/address of AVInetAddr and some flags as last argument in
inetaddr_resolve(). To access the family, simply use
AVInetAddr::addr->sa_family.

Other changes:
- ports tcp.c over to the new API

(I have patches for udp, ffserver, rtsp and for removal of resolve_host()
also, I'll try 1-by-1 and see if that makes this whole submission process
somewhat faster...)

Ronald

[Ronald S. Bultje]

Hi,

Post by Ronald S. Bultje

Post by Ronald S. Bultje
ffmpeg-avinetaddr-ipv6.patch - add IPv6 host resolution and info API,
this deprecates resolve_host() (ipv4-only). Also converts tcp.c over to
the new API.

This API needed a small enhancement (new flag) to handle the address
family problem discussed in the rtp/ipv6 thread elsewhere. Updated patch
attached.
AVInetAddr is a replacement API for resolve_host(), which is limited to
IPv4. AVInetAddr is fully IPv6-compatible. The API is similar to
resolve_host(), which means that relatively minor changes are needed to port
protocols over to the new API and become IPv6-compatible. The API consists
- AVInetAddr: sort of a container for struct addrinfo, works on both
ipv4-only and ipv6-capable systems
- inetaddr_resolve: replaces resolve_host(), can either parse IPv4/6 names
w/o host resolution or resolve hosts on a name server
- inetaddr_alloc: for those cases where you need an empty AVInetAddr, e.g.
for use in accept()
- inetaddr_str: basically inet_ntop() and inet_ntoa().
- there's also a bunch of macros to directly access (set/get) the
port/address of AVInetAddr and some flags as last argument in
inetaddr_resolve(). To access the family, simply use
AVInetAddr::addr->sa_family.
- ports tcp.c over to the new API
(I have patches for udp, ffserver, rtsp and for removal of resolve_host()
also, I'll try 1-by-1 and see if that makes this whole submission process
somewhat faster...)

Anyone want to go through the review process with me (or: ping!)? I'm
willing to address any concern, but in the end I'll need some review help to
get it in. I really think IPv6 support would be a nice feature to have in
ffmpeg, so anyone interested in networking+ff, please go along with me so we
can try to get this stuff in. In addition, once in, udp.c will get a lot
simpler, which should be good for all of us also.

Ronald

[Luca Abeni]

Hi Ronald,

Ronald S. Bultje wrote:
[...]

Post by Ronald S. Bultje
Anyone want to go through the review process with me (or: ping!)? I'm
willing to address any concern, but in the end I'll need some review help to
get it in. I really think IPv6 support would be a nice feature to have in
ffmpeg, so anyone interested in networking+ff, please go along with me so we
can try to get this stuff in. In addition, once in, udp.c will get a lot
simpler, which should be good for all of us also.

Sorry, I've been busy with other things... But now I should have some time
for ffmpeg hacking. This afternoon I'll commit the rtsp change you need for
having ffplay working in macos (adding the server address to the rtp:// url
when opening the socket).

After that, I hope to help in getting the IPv6 patches in subversion (I feel
that it could be safer/simpler to follow a different approach, but I need to
have a better look at your patches).


Luca

[Nicolas George]

+int inetaddr_resolve(AVInetAddr **in_addr, const char *hostname,
+ int port, int flags)

I was going to advice the addition of a new argument, AVInetAddr
*other_addr, to require an address compatible with an address that we
already have. For example, a source address compatible with a destination
address. This was exactly the source of the macos bug, I believe.

But this is not good: if the source address is "localhost", which can mean
127.0.0.1 or ::1, then the source address family must be chosen according to
the destination address family. On the other hand, if the destination
address is an host name with both A and AAAA records, then the destination
address family must be chosen according to the source address family.

The conclusion is that, when dealing with potentially both a source address
and a destination address, both must be resolved in the beginning, and only
the compatible pairs should be kept.

+#define INETADDR_PORT(a) \
+ ((struct sockaddr_in *) a->addr)->sin_port

I believe this macro is wrong when dealing with addresses that are not IPv4.

+#define INETADDR_ADDR(a) \
+ a->addr->sa_data[a->addr->sa_family == AF_INET ? 2 : 6]

I do not understand this macro. In particular, what are 2 and 6?

Regards,

[Ronald S. Bultje]

Hi,

Post by Nicolas George

+int inetaddr_resolve(AVInetAddr **in_addr, const char *hostname,
+ int port, int flags)

I was going to advice the addition of a new argument, AVInetAddr
*other_addr, to require an address compatible with an address that we
already have.

I added flags to force that already, see AV_RESOLVE_IPV[46]ONLY.

Post by Nicolas George

+#define INETADDR_PORT(a) \
+ ((struct sockaddr_in *) a->addr)->sin_port

I believe this macro is wrong when dealing with addresses that are not IPv4.

No, the offset is the same for IPv4 and IPv6.

Post by Nicolas George

+#define INETADDR_ADDR(a) \
+ a->addr->sa_data[a->addr->sa_family == AF_INET ? 2 : 6]

I do not understand this macro. In particular, what are 2 and 6?

The offset of the address from the data in the sockaddr_in and sockaddr_in6
struct. In both cases, there's a 16-bit port number, but after that, the
address follows directly in the sockaddr_in structure, whereas there's
another 32 bits in between for sockaddr_in6. Hence the different offset for
IPv4 and IPv6.

Ronald

[Nicolas George]

Post by Ronald S. Bultje
I added flags to force that already, see AV_RESOLVE_IPV[46]ONLY.

I do not like that interface: that means that the caller has to set the
correct AV_* flag according to the AF_* value. Which implies it knows the
mapping between all possible AF_* and AV_RESOLVE_*ONLY. That breaks protocol
independence.

Post by Ronald S. Bultje
No, the offset is the same for IPv4 and IPv6.

There is absolutely no guarantee of that in any standard that I know.
Relying on it would make the code quite fragile.

Something like:

*(a->family == AF_INET ?
&((struct sockaddr_in *) a->addr)->sin_port :
&((struct sockaddr_in6 *) a->addr)->sin6_port)

would probably be much more reliable.

But doing it as a macro would require that the caller has all the necessary
headers. I think I would be more at ease with get_port and set_port macros.

Post by Ronald S. Bultje
The offset of the address from the data in the sockaddr_in and sockaddr_in6
struct. In both cases, there's a 16-bit port number, but after that, the
address follows directly in the sockaddr_in structure, whereas there's
another 32 bits in between for sockaddr_in6. Hence the different offset for
IPv4 and IPv6.

Again, hardcoding the offset of a structure member does not seem a sane way
of coding to me.

But I am wondering: what could the caller possibly do with just the address,
like that? To do something useful with the address, it would require at
least its size, and again, that breaks protocol independence.

Regards,

[Ronald S. Bultje]

Hi,

Post by Nicolas George

Post by Ronald S. Bultje
I added flags to force that already, see AV_RESOLVE_IPV[46]ONLY.

I do not like that interface: that means that the caller has to set the
correct AV_* flag according to the AF_* value. Which implies it knows the
mapping between all possible AF_* and AV_RESOLVE_*ONLY. That breaks protocol
independence.

Only those two are supported anyway. Calls like getaddrinfo() don't support
any others. But I can sort of see your point, I could add a macro like
AV_RESOLVE_FAMILY(addr) which sets the flag:

#define AV_RESOLVE_FAMILY(addr) \
addr->addr->sa_family == AF_INET ? AV_RESOLVE_IPV4ONLY :
(addr->addr->sa_family == AF_INET6 ? AV_RESOLVE_IPV6ONLY : 0)

A reverse macro for use inside inetaddr_resolve() could also be done,
although I'd consider it overkill already. Would that be OK for you?

Post by Nicolas George

Post by Ronald S. Bultje
No, the offset is the same for IPv4 and IPv6.

There is absolutely no guarantee of that in any standard that I know.
Relying on it would make the code quite fragile.
*(a->family == AF_INET ?
&((struct sockaddr_in6 *) a->addr)->sin6_port)
would probably be much more reliable.
But doing it as a macro would require that the caller has all the necessary
headers. I think I would be more at ease with get_port and set_port macros.

Your macro is equally fine with me, and does indeed look safer. I think the
header mess is why I did it this way. We can make two macros, one for if
it's there (inet6/in6.h) and one for if it isn't, it's not that bad.

Post by Nicolas George

Post by Ronald S. Bultje
The offset of the address from the data in the sockaddr_in and

sockaddr_in6

Post by Ronald S. Bultje
struct. In both cases, there's a 16-bit port number, but after that, the
address follows directly in the sockaddr_in structure, whereas there's
another 32 bits in between for sockaddr_in6. Hence the different offset

for

Post by Ronald S. Bultje
IPv4 and IPv6.

Again, hardcoding the offset of a structure member does not seem a sane way
of coding to me.
But I am wondering: what could the caller possibly do with just the address,
like that? To do something useful with the address, it would require at
least its size, and again, that breaks protocol independence.

It's for ffserver, it does ACL checks using this thing. Other than that, it
shouldn't be used, and it isn't used, except in inet_ntop() inside
inetaddr_str() (and yes that's how you're supposed to do it, apparently,
it's a bit ugly... Ohwell).

Ronald

[Nicolas George]

Post by Ronald S. Bultje
Only those two are supported anyway.

I think a goal to set for that code should be that supporting a new address
family would not require anything outside a few wrapper functions.

Post by Ronald S. Bultje
Calls like getaddrinfo() don't support
any others.

But they may. There have been implementations of getaddrinfo that could
return AF_UNIX addresses, for example. And there may be implementations
supporting new or old protocols.

Post by Ronald S. Bultje
But I can sort of see your point, I could add a macro like
#define AV_RESOLVE_FAMILY(addr) \
(addr->addr->sa_family == AF_INET6 ? AV_RESOLVE_IPV6ONLY : 0)
A reverse macro for use inside inetaddr_resolve() could also be done,
although I'd consider it overkill already. Would that be OK for you?

The more I think about it, the less I am happy with these AV_RESOLVE_*ONLY
flags. What is the reason for their presence? I see 3 possible:

a. the address will be used by a subsystem that only handles one address
family (and can not easily fixed to be protocol-independent);

b. the address needs to be compatible with another, for example for a
connect/bind pair;

c. the user explicitly requested so.

For a and b, using the AF_* value itself is fine: in a, the constant we need
is necessarily available, and in b, the value is in the address structure of
the address we need to be compatible with.

(And anyway, for b, I as pointed earlier, restricting the address family is
not correct.)

For c, we need constants that do not depend on system headers, but I believe
that a string constant would be preferable.

Post by Ronald S. Bultje
Your macro is equally fine with me, and does indeed look safer. I think the
header mess is why I did it this way. We can make two macros, one for if
it's there (inet6/in6.h) and one for if it isn't, it's not that bad.

To avoid the protocol mess, the best is probably to avoid macros and use
accessing functions.

By the way, the correct header for sockaddr_in6 is netinet/in.h; any system
which works otherwise is broken with regard with the standard.

Post by Ronald S. Bultje
It's for ffserver, it does ACL checks using this thing.

Could these thecks be done on the string representation rather than the
binary representation?

Post by Ronald S. Bultje
Other than that, it
shouldn't be used, and it isn't used, except in inet_ntop() inside
inetaddr_str() (and yes that's how you're supposed to do it, apparently,
it's a bit ugly... Ohwell).

I had the impression that getnameinfo (with NI_NUMERICHOST and
NI_NUMERICSERV) was the recommended API. Did you try so?

Regards,

[Ronald S. Bultje]

Hi,

Post by Nicolas George

Post by Ronald S. Bultje
Only those two are supported anyway.

I think a goal to set for that code should be that supporting a new address
family would not require anything outside a few wrapper functions.

That is indeed my goal also. However, I see a macro as API, and the current
ones are sufficient for the current use-cases, and can be extended for
later.

Post by Nicolas George

Post by Ronald S. Bultje
Calls like getaddrinfo() don't

support

Post by Ronald S. Bultje
any others.

But they may. There have been implementations of getaddrinfo that could
return AF_UNIX addresses, for example. And there may be implementations
supporting new or old protocols.

Imo, if they're not part of the standard and implemented in all widely
available ones, we shouldn't rely on them at all.

Post by Nicolas George

Post by Ronald S. Bultje
But I can sort of see your point, I could add a macro like
#define AV_RESOLVE_FAMILY(addr) \
(addr->addr->sa_family == AF_INET6 ? AV_RESOLVE_IPV6ONLY : 0)
A reverse macro for use inside inetaddr_resolve() could also be done,
although I'd consider it overkill already. Would that be OK for you?

The more I think about it, the less I am happy with these AV_RESOLVE_*ONLY
a. the address will be used by a subsystem that only handles one address
family (and can not easily fixed to be protocol-independent);
b. the address needs to be compatible with another, for example for a
connect/bind pair;
c. the user explicitly requested so.

Only (b). (c) could be interesting, but isn't my goal an isn't implemented
(yet). (a) won't happen.

Post by Nicolas George
For a and b, using the AF_* value itself is fine: in a, the constant we need
is necessarily available, and in b, the value is in the address structure of
the address we need to be compatible with.

It's used only once (literally). Adding one extra argument for single-time
use is stupid, hence the flag insitead of extra function argument.

Post by Nicolas George
(And anyway, for b, I as pointed earlier, restricting the address family is
not correct.)

I don't quite see why. This is exactly what is documented to do (?).

Post by Nicolas George

Post by Ronald S. Bultje
Your macro is equally fine with me, and does indeed look safer. I think

the

Post by Ronald S. Bultje
header mess is why I did it this way. We can make two macros, one for if
it's there (inet6/in6.h) and one for if it isn't, it's not that bad.

To avoid the protocol mess, the best is probably to avoid macros and use
accessing functions.

That's an opinion, and I do not share it. :-). If you want to do it, then
please submit patches. I think I mentioned this before, I've had those for
over a year now, get them in, don't continuously start over from the
beginning, it doesn't get me anywhere.

Post by Nicolas George

Post by Ronald S. Bultje
It's for ffserver, it does ACL checks using this thing.

Could these thecks be done on the string representation rather than the
binary representation?

"Patches welcome". I guess there's ways to do it. Since this works, I don't
care so much.

Post by Nicolas George

Post by Ronald S. Bultje
Other than that,

it

Post by Ronald S. Bultje
shouldn't be used, and it isn't used, except in inet_ntop() inside
inetaddr_str() (and yes that's how you're supposed to do it, apparently,
it's a bit ugly... Ohwell).

I had the impression that getnameinfo (with NI_NUMERICHOST and
NI_NUMERICSERV) was the recommended API. Did you try so?

Both work equally fine.

Ronald

[Ronald S. Bultje]

Hi,

Post by Ronald S. Bultje
ffmpeg-ipv6-udp.patch - converts udp.c over to the new API, also removes
all duplicated code for CONFIG_IPV6 except for the multicase setup.

And the attached patch updates this one to fix the address family mess
discussed elsewhere, and is updated for when that patch is applied.

This patch:
- removes all duplicated CONFIG_IPV6 code in udp.c
- ports the IPv4-code over to the new AVInetAddr API, thus making it a lot
smaller, simpler and still IPv6-compatible

Just in case people ask:
- I made a minor change in udp_set_remote_url() to set port=-1 to port=0,
this is because that is needed for getaddrinfo(), which returns an error if
port = -1. This was previously not needed because the ipv4-name resolution
API does not need a port as an argument
- A second small change is that I now check the return value of
udp_set_remote_url(), this is because I directly access the s->dest_addr
pointer below, so if it fails we should error out, else the code will
segfault on invalid hosts/ports. This was previously not needed because
dest_addr was not a pointer.
- lastly, I removed the av_mallocz() introdduced in a previous patch to zero
s->dest_addr, because s->dest_addr is now a pointer which I can just
initialize to NULL.

Apart from that, the patch looks big, but don't forget that most of that is
code removal. The actual changes are extremely minor and logical (apart from
what I mentioned above).

Ronald

[Luca Abeni]

Hi Ronald,

first of all, I just committed the remaining part of your patch for
fixing ffserver and ffplay on macos. Can you try a checkout and confirm
if they now work for you?

Then, I am beginning to look at your other IPv6 support patches...
(WARNING: from this point, I am expressing my own opinions. If someone else
disagrees and is willing to review these patches and help to commit the IPv6
code, I am happy to step back).

IMHO, the first thing we need to do is to cleanup some code (for example,
in udp.c), so that your patches can be made smaller, easier to review, and
safer to apply.

For example, udp.c currently contains a lot of code duplication between
IPv6 and IPv4. I believe that removing this code duplication is the first
step. I see that your ffmpeg-ipv6-udp.patch removes some duplicated code:
Ronald S. Bultje wrote:
[...]

Post by Ronald S. Bultje
- removes all duplicated CONFIG_IPV6 code in udp.c
- ports the IPv4-code over to the new AVInetAddr API, thus making it a lot
smaller, simpler and still IPv6-compatible
- I made a minor change in udp_set_remote_url() to set port=-1 to port=0,

[...]

Can you split the patch and send the part which just removes duplicated code?
If yes, then I think that part (just reorganization of the code, without any
change in the behaviour) can be committed soon.


Thanks,
Luca

[Ronald S. Bultje]

Hi,

Post by Luca Abeni
Can you split the patch and send the part which just removes duplicated code?
If yes, then I think that part (just reorganization of the code, without any
change in the behaviour) can be committed soon.

Sure, Quite clearly, that will result in temporary lack of ipv6 support
until my new patches have been applied, is that OK?

Ronald

[Luca Abeni]

Hi Ronald,

Post by Ronald S. Bultje
Hi,

Post by Luca Abeni
Can you split the patch and send the part which just removes duplicated code?
If yes, then I think that part (just reorganization of the code, without any
change in the behaviour) can be committed soon.

Sure, Quite clearly, that will result in temporary lack of ipv6 support
until my new patches have been applied, is that OK?

No, this is not what I meant... I was thinking about code reorganization
only, without any change in the behaviour (so, without removing IPv6
support).
Let me make an example... In udp.c, we have things like:
int udp_set_remote_url(URLContext *h, const char *uri)
{
#ifdef CONFIG_IPV6
return udp_ipv6_set_remote_url(h, uri);
#else
UDPContext *s = h->priv_data;
char hostname[256];
int port;

url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &port, NULL, 0, uri);

/* set the destination address */
if (resolve_host(&s->dest_addr.sin_addr, hostname) < 0)
return AVERROR(EIO);
s->dest_addr.sin_family = AF_INET;
s->dest_addr.sin_port = htons(port);
return 0;
#endif
}
and
static int udp_ipv6_set_remote_url(URLContext *h, const char *uri) {
UDPContext *s = h->priv_data;
char hostname[256];
int port;
struct addrinfo *res0;
url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &port, NULL, 0, uri);
res0 = udp_ipv6_resolve_host(hostname, port, SOCK_DGRAM, AF_UNSPEC, 0);
if (res0 == 0) return AVERROR(EIO);
memcpy(&s->dest_addr, res0->ai_addr, res0->ai_addrlen);
s->dest_addr_len = res0->ai_addrlen;
freeaddrinfo(res0);
return 0;
}

Now, udp_ipv6_set_remote_url() is very similar to the code in
udp_set_remote_url()... This is an example of the code duplication I
was referring to... Isn't this the code duplication you addressed in
your patch?
I think this kind of duplication can be removed without any change in
the functionalities (so, without loosing IPv6 support).


Thanks,
Luca

[Ronald S. Bultje]

Hi Luca,

Post by Luca Abeni

Post by Ronald S. Bultje
Sure, Quite clearly, that will result in temporary lack of ipv6 support
until my new patches have been applied, is that OK?

No, this is not what I meant... I was thinking about code reorganization
only, without any change in the behaviour (so, without removing IPv6
support).

[..]

Post by Luca Abeni
Now, udp_ipv6_set_remote_url() is very similar to the code in
udp_set_remote_url()... This is an example of the code duplication I
was referring to... Isn't this the code duplication you addressed in
your patch?
I think this kind of duplication can be removed without any change in
the functionalities (so, without loosing IPv6 support).

Dropping either code path will result in:
- losing ipv6 support
or
- losing support for OSes that do not have ipv6

I'm OK with either, although I can't help but wonder if we're not just going
in circles to get my patches in.

As for the "why inetaddr_*() instead of just the ipv6- (-independent) code,
it's basically for legacy-support. FF also supports gcc 2.95, so... If you
want getaddrinfo() directly, go for it, but then why did the code paths
exist to begin with?

Ronald

[Luca Abeni]

Hi Ronald,

Post by Ronald S. Bultje

Post by Luca Abeni
Now, udp_ipv6_set_remote_url() is very similar to the code in
udp_set_remote_url()... This is an example of the code duplication I
was referring to... Isn't this the code duplication you addressed in
your patch?
I think this kind of duplication can be removed without any change in
the functionalities (so, without loosing IPv6 support).

- losing ipv6 support
or
- losing support for OSes that do not have ipv6

I believe that some redundant code can be removed (reworking the code
a little bit) without losing support for anything or changing any
functionality... But I have no patches yet to prove this claim.
I hope to send some example patches soon to clarify my idea (just give
me the time to test some ideas).


Luca

[Luca Abeni]

Hi,

Luca Abeni wrote:
[...]

Post by Luca Abeni
I believe that some redundant code can be removed (reworking the code
a little bit) without losing support for anything or changing any
functionality... But I have no patches yet to prove this claim.
I hope to send some example patches soon to clarify my idea (just give
me the time to test some ideas).

And here are the patches. Lightly tested, on r10952, for both unicast and
multicast UDP, either with or without --disable-ipv6.

I generated the patches in a short time, so maybe some of them can have
typos or can be written in a better way, but I believe they show how to
remove duplicated code without (hopefully :) breaking anything.

- address-len.diff removes some stupid ifdef by using the dest_addr_len
field in both the IPv4-only and the generic code. In this way, we decrease
the differences between the two cases
- fix-multicast-mess.diff removes an useless field in struct UDPContext, and
uses the same code in the IPv4-only and the generic code, renaming some
functions with appropriate names
- remove_from.diff removes some more ifdefs by using recv() instead of recvfrom()
(the "from" parameter was discarded anyway...)
- simplify-set-remote-url.diff removes some code duplictaion in udp_set_remote_url()

All the patches should introduce no changes in the behaviour of the functions,
and I believe that after applying them the code is more clear and readable,
and the differences between the protocol-independent code and the IPv4-only
are greatly reduced. Some work is still needed (especially in udp_open()), but
I think the patches give an idea about my plan of action... Do people agree
with it?


Thanks,
Luca

[Michael Niedermayer]

Post by Luca Abeni
Hi,
[...]

Post by Luca Abeni
I believe that some redundant code can be removed (reworking the code
a little bit) without losing support for anything or changing any
functionality... But I have no patches yet to prove this claim.
I hope to send some example patches soon to clarify my idea (just give
me the time to test some ideas).

And here are the patches. Lightly tested, on r10952, for both unicast and
multicast UDP, either with or without --disable-ipv6.
I generated the patches in a short time, so maybe some of them can have
typos or can be written in a better way, but I believe they show how to
remove duplicated code without (hopefully :) breaking anything.
- address-len.diff removes some stupid ifdef by using the dest_addr_len
field in both the IPv4-only and the generic code. In this way, we decrease
the differences between the two cases

looks ok

Post by Luca Abeni
- fix-multicast-mess.diff removes an useless field in struct UDPContext, and
uses the same code in the IPv4-only and the generic code, renaming some
functions with appropriate names

split this please, at least all renames and cosmetics should be seperated

Post by Luca Abeni
- remove_from.diff removes some more ifdefs by using recv() instead of recvfrom()
(the "from" parameter was discarded anyway...)

ok if it works

Post by Luca Abeni
- simplify-set-remote-url.diff removes some code duplictaion in udp_set_remote_url()

ok if it works

[...]

[Luca Abeni]

Hi Michael,

Michael Niedermayer wrote:
[...]

Post by Luca Abeni

Post by Luca Abeni
I generated the patches in a short time, so maybe some of them can have
typos or can be written in a better way, but I believe they show how to
remove duplicated code without (hopefully :) breaking anything.
- address-len.diff removes some stupid ifdef by using the dest_addr_len
field in both the IPv4-only and the generic code. In this way, we decrease
the differences between the two cases

looks ok

[...]

Thanks for your feedback. I have to go right now, but I'll take care of this
ASAP (hopefully tomorrow morning).



Thanks,
Luca

[Ronald S. Bultje]

Hi,

Post by Luca Abeni
I generated the patches in a short time, so maybe some of them can have
typos or can be written in a better way, but I believe they show how to
remove duplicated code without (hopefully :) breaking anything.
- address-len.diff removes some stupid ifdef by using the dest_addr_len
field in both the IPv4-only and the generic code. In this way, we decrease
the differences between the two cases

[..]

So now that all those are applied, what's step2? I mean, all this
re-arranging of a few lines is fun, but the rest of ffmpeg still doesn't
speak ipv6, my patches are not going in so that's apparently not right
either, and there's no clear outline of where this is going. We need a plan
and action. This "let's do nothing and wait for the Right Solution[tm] to
come in" isn't working.

- in os_support.c, resolve_host() needs to be converted to be
ipv6-compatible or removed; the inet_aton() backup needs to be removed
- all duplicate code in udp.c needs to be removed, right now it's just
duplicated even more
- tcp.c, ffserver, sdp.c, rtsp.c need to speak ipv6
- api needs to be able to return sensible error codes and applications need
to be able to report those

Any plans?

Ronald

[Luca Abeni]

Hi Ronald

Post by Ronald S. Bultje
Hi,

Post by Luca Abeni
I generated the patches in a short time, so maybe some of them can have
typos or can be written in a better way, but I believe they show how to
remove duplicated code without (hopefully :) breaking anything.
- address-len.diff removes some stupid ifdef by using the dest_addr_len
field in both the IPv4-only and the generic code. In this way, we decrease
the differences between the two cases

[..]
So now that all those are applied, what's step2?

My understanding was that you was going to post some updated patches after
I committed my ones (you said: "I'll say it this way: there's multiple
ways to get there, and I think your patches are a good start in one such
direction to fix the issue, and I'll gladly pick up from there and resolve
the whole thing").
But at this point I suggest you to wait for the libavnet changes before
refreshing your patches.

Post by Ronald S. Bultje
I mean, all this
re-arranging of a few lines is fun, but the rest of ffmpeg still doesn't
speak ipv6, my patches are not going in

Maybe I just missed them (and in this case, let me apologize in advance),
but I did not see the updated patches after my changes.

Post by Ronald S. Bultje
so that's apparently not right
either, and there's no clear outline of where this is going. We need a plan
and action. This "let's do nothing and wait for the Right Solution[tm] to
come in" isn't working.
- in os_support.c, resolve_host() needs to be converted to be
ipv6-compatible or removed

The "protocol independent code" should not use resolve_host(), I think...
If ffmpeg is not compiled with --disable-ipv6, udp.c does not use resolve_host().
tcp.c should probably be fixed to do the same.
I think there was an agreement on directly using getaddrinfo()?

Post by Ronald S. Bultje
- all duplicate code in udp.c needs to be removed, right now it's just
duplicated even more

Point out some duplicated code, and I'll try to do something about it.

Post by Ronald S. Bultje
- tcp.c, ffserver, sdp.c, rtsp.c need to speak ipv6

The sdp.c part should be easy. Just send a patch for sdp.c (using getaddrinfo)
and I'll commit it.

Post by Ronald S. Bultje
- api needs to be able to return sensible error codes and applications need
to be able to report those

Once again, I am confused about this part: how is it related to IPv6?
Reporting sensible error codes should be something completely orthogonal to
the IPv6 stuff...


Luca

[Ronald S. Bultje]

Hi Luca,

Post by Luca Abeni
My understanding was that you was going to post some updated patches after
I committed my ones (you said: "I'll say it this way: there's multiple
ways to get there, and I think your patches are a good start in one such
direction to fix the issue, and I'll gladly pick up from there and resolve
the whole thing").

Ok, but I still need to know how:

I could duplicate udp.c's ipv6_resolve_host() everywhere, generalize it and
keep it either in udp.c and export it or move it back into os_support.c
(instead of resolve_host()), or I could remove it completely and integrate
the call into the function caller in udp.c and duplicate that code into
tcp.c.

For udp.c, I would highly prefer to not only remove the ipv4-code path, but
also integrate all the single-use functions that we have now back into their
respective callers (udp_port(), udp_set_url()); this would basically undo
most of the work that you did so far, but I'd consider it a cleanup, since
if you remove the ipv4-code, the result is a bit like spaghetti...

Then there's a tenfold of code in sdp.c, ffserver.c and rtsp.c involving
changing in_addr / sockaddr_in into generalized containers holders and
inet_aton -> inet_pton or getnameinfo; my patches handled most of these
already and would probably still be useful afterwards (although I'd need to
change them to use addrinfo directly instead of this avnet stuff).

In the end, a lot of things and various ways to go, since I did 3 approaches
so far (uses error code from gethostbyname, switch to gethostbyname_r and
switch to getaddrinfo in a wrapper) and none of them got me anywhere, I'd
like a clear path for me before I start, I'm not going to waste another
month on these patches and see them rejected again.

But at this point I suggest you to wait for the libavnet changes before

Post by Luca Abeni
refreshing your patches.

OK.

Ronald

[Luca Abeni]

Hi Ronald,

Post by Ronald S. Bultje
Hi Luca,

Post by Luca Abeni
My understanding was that you was going to post some updated patches after
I committed my ones (you said: "I'll say it this way: there's multiple
ways to get there, and I think your patches are a good start in one such
direction to fix the issue, and I'll gladly pick up from there and resolve
the whole thing").

I could duplicate udp.c's ipv6_resolve_host() everywhere

I think duplication is not an option.

Post by Ronald S. Bultje
generalize it and
keep it either in udp.c and export it or move it back into os_support.c

I believe that if if can be generalized without increasing its complexity
too much, then it should be generalized and moved in a generic "network.c"
file, which contains all the networking code shared between UDP and TCP.
I also believe that os_support.c should disappear...

Post by Ronald S. Bultje
or I could remove it completely and integrate
the call into the function caller in udp.c and duplicate that code into
tcp.c.

I do not like this solution (because of code duplication, and because I
believe that functions should be reasonably small).

Post by Ronald S. Bultje
For udp.c, I would highly prefer to not only remove the ipv4-code path, but
also integrate all the single-use functions that we have now back into their
respective callers (udp_port(), udp_set_url()); this would basically undo
most of the work that you did so far, but I'd consider it a cleanup, since
if you remove the ipv4-code, the result is a bit like spaghetti...

When I posted my patches, I clearly stated that I was only trying to help,
and if the patches were not useful, I did not want to insist for having them
applied.
It seemed to me that people agreed about the patches. Now, you basically
want to revert them. If this was your goal, you could have stated it since
the beginning, so that I could have avoided the work of writing the patches,
testing them, and committing them.
We clearly have different opinions about spaghetti programming, and I believe
that splitting a big function in smaller ones having meaningful names improves
the readability of the code. But again, this is just my opinion.

Post by Ronald S. Bultje
Then there's a tenfold of code in sdp.c, ffserver.c and rtsp.c involving
changing in_addr / sockaddr_in into generalized containers holders and
inet_aton -> inet_pton or getnameinfo; my patches handled most of these
already and would probably still be useful afterwards (although I'd need to
change them to use addrinfo directly instead of this avnet stuff).

Yes, I think so.

Post by Ronald S. Bultje
In the end, a lot of things and various ways to go, since I did 3 approaches
so far (uses error code from gethostbyname, switch to gethostbyname_r and
switch to getaddrinfo in a wrapper) and none of them got me anywhere, I'd
like a clear path for me before I start, I'm not going to waste another
month on these patches and see them rejected again.

Maybe the point is that we should proceed by small steps:
1) ensure that the "getaddrinfo" code works well on the most used OSs (hint:
mingw seems to provide getaddrinfo(), but the configure script is not able
to detect it, and when compiling with mingw CONFIG_IPV6 is disabled).
2) remove the IPV4-only code from udp.c
3) wait some time, and see if people complain. Implement a getaddrinfo replacement
in an external library (maybe the libossupport - or whatever is the name - that
Michael is requesting?), for the (hopefully few) systems that do not provide it.
Someone said that this should be fairly easy... Maybe he can provide some help on this?
4) convert tcp.c
5) convert rtsp.c and sdp.c
6) fix ffserver.c

Again, this is just my opinion. I do not want to force anyone to follow it.
If people agree that this is the way to go, I can help following the path I
indicated.


Luca

[Michael Niedermayer]

Post by Luca Abeni
Hi Ronald,

[...]

Post by Luca Abeni

Post by Ronald S. Bultje
In the end, a lot of things and various ways to go, since I did 3 approaches
so far (uses error code from gethostbyname, switch to gethostbyname_r and
switch to getaddrinfo in a wrapper) and none of them got me anywhere, I'd
like a clear path for me before I start, I'm not going to waste another
month on these patches and see them rejected again.

mingw seems to provide getaddrinfo(), but the configure script is not able
to detect it, and when compiling with mingw CONFIG_IPV6 is disabled).
2) remove the IPV4-only code from udp.c
3) wait some time, and see if people complain. Implement a getaddrinfo replacement
in an external library (maybe the libossupport - or whatever is the name -

libossupport is a nice name :)

Post by Luca Abeni
that Michael is requesting?),

iam not requesting it, i just strongly think that os_support.* and all
similar hacks belong into a lib different from libav*
if you can change the code so os_support.* is no longer needed and it still
works on all platforms where it did work before then thats perfectly fine as
well

Post by Luca Abeni
for the (hopefully few) systems that do not provide it.
Someone said that this should be fairly easy... Maybe he can provide some help on this?
4) convert tcp.c
5) convert rtsp.c and sdp.c
6) fix ffserver.c
Again, this is just my opinion. I do not want to force anyone to follow it.
If people agree that this is the way to go, I can help following the path I
indicated.

i agree in general with it but id be more evil and just disable the IPV4 code
instead of "ensure that the "getaddrinfo" code works well ..." that is if it
works on your system ...

the problem is that very few people will test the getaddrinfo() code as long
as they "dont have to"
the same is true with fixing the ipv* code, if it doesnt work people just use
the v4 code, if theres no v4 code they are much more likely to submit a patch

[...]

[Ronald S. Bultje]

Hi,

Post by Michael Niedermayer
i agree in general with it but id be more evil and just disable the IPV4 code
instead of "ensure that the "getaddrinfo" code works well ..." that is if it
works on your system ...

Good, so let's throw the ipv4-code out of udp.c, move ipv6_resolve_host()
into a network.c and do the same thing to tcp.c and wait for hell to come in
and get us. Should be 5 minutes of work. :-). Disable udp.c if CONFIG_IPV6
is not set, or just make CONFIG_NETWORK require CONFIG_IPV6.

Ronald

[Luca Abeni]

Hi Michael,

Michael Niedermayer wrote:
[...]

Post by Michael Niedermayer

Post by Luca Abeni
3) wait some time, and see if people complain. Implement a getaddrinfo replacement
in an external library (maybe the libossupport - or whatever is the name -

libossupport is a nice name :)

Post by Luca Abeni
that Michael is requesting?),

iam not requesting it

Sorry, I used a bad wording. I should have said "suggesting".

Post by Michael Niedermayer
i just strongly think that os_support.* and all
similar hacks belong into a lib different from libav*

I agree with this, and I'll work on moving this code in libossupport


[...]

Post by Michael Niedermayer

Post by Luca Abeni
Again, this is just my opinion. I do not want to force anyone to follow it.
If people agree that this is the way to go, I can help following the path I
indicated.

i agree in general with it but id be more evil and just disable the IPV4 code
instead of "ensure that the "getaddrinfo" code works well ..." that is if it
works on your system ...

Well, I can already see that we would have problems on mingw systems...
That's why I'd like to have a fix for that before removing the IPv4 code.

But if someone else removes the IPv4 code I will not complain (I just do not
want to be responsible for its removal now).


Luca

[Dave Yeo]

Post by Luca Abeni
Hi Michael,
[...]

Post by Michael Niedermayer

Post by Luca Abeni
3) wait some time, and see if people complain. Implement a getaddrinfo replacement
in an external library (maybe the libossupport - or whatever is the name -

libossupport is a nice name :)

Post by Luca Abeni
that Michael is requesting?),

iam not requesting it

Sorry, I used a bad wording. I should have said "suggesting".

Post by Michael Niedermayer
i just strongly think that os_support.* and all
similar hacks belong into a lib different from libav*

I agree with this, and I'll work on moving this code in libossupport
[...]

Post by Michael Niedermayer

Post by Luca Abeni
Again, this is just my opinion. I do not want to force anyone to follow it.
If people agree that this is the way to go, I can help following the path I
indicated.

i agree in general with it but id be more evil and just disable the IPV4 code
instead of "ensure that the "getaddrinfo" code works well ..." that is if it
works on your system ...

Well, I can already see that we would have problems on mingw systems...
That's why I'd like to have a fix for that before removing the IPv4 code.
But if someone else removes the IPv4 code I will not complain (I just do not
want to be responsible for its removal now).
Luca

FYI, OS/2 currently does not have getaddrinfo and I doubt that it will
ever have IPv6 support.
Dave

[Diego Biurrun]

Post by Dave Yeo
FYI, OS/2 currently does not have getaddrinfo and I doubt that it will
ever have IPv6 support.

I'd like to note here that this should not influence the decision about
IPv6 support in any way. While it's nice and desirable to support as
many platforms as possible, supporting fringe platforms must not put a
burden on the general case, hinder technical progress or make the main
platforms harder to maintain.

Diego

[Ronald S. Bultje]

Hi,

Post by Diego Biurrun

Post by Dave Yeo
FYI, OS/2 currently does not have getaddrinfo and I doubt that it will
ever have IPv6 support.

I'd like to note here that this should not influence the decision about
IPv6 support in any way. While it's nice and desirable to support as
many platforms as possible, supporting fringe platforms must not put a
burden on the general case, hinder technical progress or make the main
platforms harder to maintain.

Agreed. Also, note that there are usually already add-on libs for such
platforms (also for earlier win** versions) to provide said functions, we
can just link to those on such "fringe platforms".

Ronald

[Dave Yeo]

Post by Ronald S. Bultje
Hi,

Post by Diego Biurrun

Post by Dave Yeo
FYI, OS/2 currently does not have getaddrinfo and I doubt that it will
ever have IPv6 support.

I'd like to note here that this should not influence the decision about
IPv6 support in any way. While it's nice and desirable to support as
many platforms as possible, supporting fringe platforms must not put a
burden on the general case, hinder technical progress or make the main
platforms harder to maintain.

Agreed. Also, note that there are usually already add-on libs for such
platforms (also for earlier win** versions) to provide said functions, we
can just link to those on such "fringe platforms".
Ronald

I also agree that it shouldn't hinder progress which is why I haven't
mentioned it until now. If it is easy to support OS/2 it would be nice
to do and I was already considering an extra lib to add extra support.
I'll have to research the add-on libs.
Dave

[Luca Abeni]

Hi Dave,

Post by Dave Yeo

Post by Luca Abeni
Hi Michael,
[...]

Post by Luca Abeni
3) wait some time, and see if people complain. Implement a getaddrinfo replacement
in an external library (maybe the libossupport - or whatever is the name -

[...]

Post by Dave Yeo
FYI, OS/2 currently does not have getaddrinfo and I doubt that it will
ever have IPv6 support.

Ok; so you will just need to enable the "getaddrinfo replacement" in libossupport,
and to link such library (obviously, we cannot do anything about IPv6 support in
OS/2, but if everything works as I expect you should be able to use the ffmpeg's
networking code without problems).


Luca

[Ronald S. Bultje]

Hi,

Post by Luca Abeni
But if someone else removes the IPv4 code I will not complain (I just do not
want to be responsible for its removal now).

Can I make CONFIG_NETWORK depend on CONFIG_IPV6 then, just to cover for the
general future direction here? Makes the rest of the patch a bit easier.
(This question is probably particularly for Mans / Diego.)

Ronald

[Diego Biurrun]

Post by Ronald S. Bultje

Post by Luca Abeni
But if someone else removes the IPv4 code I will not complain (I just do not
want to be responsible for its removal now).

Can I make CONFIG_NETWORK depend on CONFIG_IPV6 then, just to cover for the
general future direction here? Makes the rest of the patch a bit easier.
(This question is probably particularly for Mans / Diego.)

I don't have an opinion on this.

Diego

[Måns Rullgård]

Post by Ronald S. Bultje
Hi,

Post by Luca Abeni
But if someone else removes the IPv4 code I will not complain (I just do not
want to be responsible for its removal now).

Can I make CONFIG_NETWORK depend on CONFIG_IPV6 then, just to cover for the
general future direction here? Makes the rest of the patch a bit easier.

Are you suggesting we require IPv6 support? IMHO that's not so good,
unless you can convince me that everybody that matters has the
necessary headers.

[Ronald S. Bultje]

Hi,

Post by Måns Rullgård

Post by Ronald S. Bultje
Can I make CONFIG_NETWORK depend on CONFIG_IPV6 then, just to cover for

the

Post by Ronald S. Bultje
general future direction here? Makes the rest of the patch a bit easier.

Are you suggesting we require IPv6 support? IMHO that's not so good,
unless you can convince me that everybody that matters has the
necessary headers.

Not exactly, but everybody will need the getaddrinfo() API, which is the API
that allows IPv6 (in addition to other network protocols), whereas the
currently used API only does IPv4 (gethostbyname()). That's probably what
you meant, but so it doesn't require IPv6.

The idea proposed by Michael, Luca and others is to use getaddrinfo()
directly and to provide a fallback getaddrinfo() implementation in a
separate library (or to link it in if it exists already) for those systems (
e.g. OS/2) that lack it.

The easiest way to do that is to make CONFIG_NETWORK depend on CONFIG_IPV6
and to add configure checks for those platform that require the special code
if they lack getaddrinfo() itself, such as to set up the linking to the new
lib providing the getaddrinfo() wrapper (Michael suggested providing a
libossupport library if that's needed), etc.

The idea of a wrapper function to wrap IPv4/IPv-independent code (a la
resolve_host()) was rejected because it looks too much like getaddrinfo()
and is redundant on all those systems that have it, doing it like in
udp.c(#ifdef CONFIG_IPV6 spaghetti) will lead to a gigantic mess (I'd
rather not
do that) and other than that, I don't really think there's a good way to do
this.

Ronald

[Rich Felker]

Post by Ronald S. Bultje
Hi,

Post by Måns Rullgård

Post by Ronald S. Bultje
Can I make CONFIG_NETWORK depend on CONFIG_IPV6 then, just to cover for

the

Post by Ronald S. Bultje
general future direction here? Makes the rest of the patch a bit easier.

Are you suggesting we require IPv6 support? IMHO that's not so good,
unless you can convince me that everybody that matters has the
necessary headers.

Not exactly, but everybody will need the getaddrinfo() API, which is the API
that allows IPv6 (in addition to other network protocols), whereas the
currently used API only does IPv4 (gethostbyname()). That's probably what
you meant, but so it doesn't require IPv6.
The idea proposed by Michael, Luca and others is to use getaddrinfo()
directly and to provide a fallback getaddrinfo() implementation in a
separate library (or to link it in if it exists already) for those systems (
e.g. OS/2) that lack it.

I agree that support for gethostbyname should be removed asap. Using
it clobbers internal state of the C library resolver and thus it
should never be called from library code. This is just like rand(),
signal(), etc...

Rich

[Nicolas George]

Post by Luca Abeni
For example, udp.c currently contains a lot of code duplication between
IPv6 and IPv4. I believe that removing this code duplication is the first
step.

Nitpick: this is not IPv6 vs. IPv4, this is protocol-independent vs.
IPv4-only. This means that in ideal circumstances, the former completely
supersedes the latter. Ideal circumstances mean two things:

- The protocol-independent version has been properly reviewed and debugged.

- The operating system has the necessary functions.

The key to achieve the first point is that everyone uses the code.

For the second point, the "necessary functions" are getaddrinfo, and
secondarily getnameinfo. These function have nothing magic, nor
system-dependant, nor anything. It is perfectly possible to implement them
on top of the legacy functions (gethostbyname and cousins); Stevens did it
in UNP, it takes the best part of a chapter, but it works. Fortunately,
ffmpeg (like most other programs) only uses a very small subset of
getaddrinfo cases, and writing an implementation suitable for its needs
would be just a matter of a few lines.

Therefore, one possible course of action would be:

- Simply drop the IPv4-only code.

- If people complain that ffmpeg does not build anymore on their old
operating system, write a limited implementation of getaddrinfo.

Another solution, which is probably better, would be to move all
network-related code somewhere together in libavutil.

This would require reviewing the code in all ffmpeg to see what API would
actually be needed, and careful design of that API, but this would make
maintenance a lot more easier.

Regards,

[Luca Abeni]

Hi Nicolas,

Post by Nicolas George

Post by Luca Abeni
For example, udp.c currently contains a lot of code duplication between
IPv6 and IPv4. I believe that removing this code duplication is the first
step.

Nitpick: this is not IPv6 vs. IPv4,

I agree with you. By IPv6, I meant "the code that is currently under
"CONFIG_IPV6".

Post by Nicolas George
this is protocol-independent vs. IPv4-only.

Ok. And I believe that the protocol-independent code and the IPv4-only code
can be made very similar. Then, we can think about removing the IPv4-only
code.

Post by Nicolas George
This means that in ideal circumstances, the former completely
supersedes the latter.

Again, I agree... But we do not live in a perfect world. I fear that
committing a big patch which remove the IPv4-only code and switches to
the protocol-independent code might break someone's setup (we had 0
feedback from windows and beos users, for example). So, I'd like to
see this transition happening in small steps... In this way, if something
break we will have less changes to check.

Post by Nicolas George
- The protocol-independent version has been properly reviewed and debugged.
- The operating system has the necessary functions.
The key to achieve the first point is that everyone uses the code.

This is what currently happens, I hope. CONFIG_IPV6 is enabled by default,
and I think we fixed the big problems with it so people should have no
reason to use --disable-ipv6.

Post by Nicolas George
For the second point, the "necessary functions" are getaddrinfo, and
secondarily getnameinfo. These function have nothing magic, nor
system-dependant, nor anything. It is perfectly possible to implement them
on top of the legacy functions (gethostbyname and cousins); Stevens did it
in UNP, it takes the best part of a chapter, but it works. Fortunately,
ffmpeg (like most other programs) only uses a very small subset of
getaddrinfo cases, and writing an implementation suitable for its needs
would be just a matter of a few lines.
- Simply drop the IPv4-only code.
- If people complain that ffmpeg does not build anymore on their old
operating system, write a limited implementation of getaddrinfo.

As I said, I do not like this idea too much, and I'd prefer to arrive to
this result by committing small patches.

Post by Nicolas George
Another solution, which is probably better, would be to move all
network-related code somewhere together in libavutil.

I am not sure if network-related code would be accepted in libavutil.
And I do not see why moving it from libavformat to libavutil would
change anything ;-)


Anyway, as I wrote in the previous email these are just my personal
opinions; if someone else volunteers to take care of properly
integrating the IPv6 patches, I will be very happy to leave this
task to him/her.



Thanks,
Luca

[Nicolas George]

Post by Luca Abeni
This is what currently happens, I hope. CONFIG_IPV6 is enabled by default,
and I think we fixed the big problems with it so people should have no
reason to use --disable-ipv6.

I think that too.

Post by Luca Abeni

Post by Nicolas George
- Simply drop the IPv4-only code.
- If people complain that ffmpeg does not build anymore on their old
operating system, write a limited implementation of getaddrinfo.

As I said, I do not like this idea too much, and I'd prefer to arrive to
this result by committing small patches.

Then the opposite order could be ok:

- Implement a version of getaddrinfo and getnameinfo suitable for ffmppeg's
needs.

- Drop the IPv4-only code.

This would require a separate test for the IPv6 multicast structures and
macros.

Post by Luca Abeni
I am not sure if network-related code would be accepted in libavutil.
And I do not see why moving it from libavformat to libavutil would
change anything ;-)

In fact, I missed the point: all code in ffmpeg that uses the network do so
by creating an udp: or tcp: URL. The code is already separated and grouped
all in the same place.

Post by Luca Abeni
Anyway, as I wrote in the previous email these are just my personal
opinions; if someone else volunteers to take care of properly
integrating the IPv6 patches, I will be very happy to leave this
task to him/her.

I just looked at Ronald's code, and I am wondering about his
inetaddr_resolve function: as far as I can see, it does exactly the same as
getaddrinfo, only with a different API. Is it so?

If it is, why not keep the getaddrinfo API? This would make less code, and
in particular, no extra code at all in the good case (= modern
standard-compliant OS).

Regards,

[Ronald S. Bultje]

Hi,

Post by Nicolas George
- Implement a version of getaddrinfo and getnameinfo suitable for ffmppeg's
needs.
- Drop the IPv4-only code.

[..]

Post by Nicolas George
I just looked at Ronald's code, and I am wondering about his
inetaddr_resolve function: as far as I can see, it does exactly the same as
getaddrinfo, only with a different API. Is it so?

In part (largely), yes. However, once in, I plan to extend it with good
error handling (see my antique errno patches if you want to know how I got
at this point).

As for making a getaddrinfo() for systems that do have it, I guess that's
another way, but then I wouldn't be able to generalize error handling into
the wrapper, plus that I would risk (and be asking for) symbol clashes,
which is especially bad since I want to export this stuff and have ffserver
use the exported symbols (see other antique patches) rather than the current
hacks. I originally just changed resolve_host() to be ipv6-compatible (and
before that, to have error handling). Only later did I rename it to
inetaddr_*() as the API changed too much.

Anyway, there's multiple ways to get there, I'd prefer this, but it'd be
nice to have a fixed path to take rather than the turns I continuously have
to take now (I hope you can see how it is a tad depressing to not make any
progress on this front for over a year now). If there's no strong
objections, I'd honestly prefer getting this in and continuing from here,
the patches are not so bad imo. We waste too much time on this simple stuff,
I have lots of stuff I would be doing after this (e.g. errno patch
re-visiting, and actually continue my work on userspace applications that
make this stuff available to the average new Linux user that barely knows
what avi is, how to compile ffmpeg or what the verb to compile actually
signifies), but all this gets pushed back more and more as we re-go over old
issues that are not so interesting anymore.

Ronald

[Michael Niedermayer]

Post by Ronald S. Bultje
Hi,

Post by Nicolas George
- Implement a version of getaddrinfo and getnameinfo suitable for ffmppeg's
needs.
- Drop the IPv4-only code.

[..]

Post by Nicolas George
I just looked at Ronald's code, and I am wondering about his
inetaddr_resolve function: as far as I can see, it does exactly the same as
getaddrinfo, only with a different API. Is it so?

In part (largely), yes. However, once in, I plan to extend it with good
error handling (see my antique errno patches if you want to know how I got
at this point).
As for making a getaddrinfo() for systems that do have it, I guess that's
another way, but then I wouldn't be able to generalize error handling into
the wrapper, plus that I would risk (and be asking for) symbol clashes,
which is especially bad since I want to export this stuff and have ffserver
use the exported symbols (see other antique patches) rather than the current
hacks. I originally just changed resolve_host() to be ipv6-compatible (and

the problems are that
1. its not libavformats job at all to provide any way to resolve names to
IPs, this really is outside of libavformats area, and would be a huge hack
by itself. So you can expect more resistance here no matter if AVWhatever
or getaddrinfo() libavformat has no busness exporting these
2. if there were a seperate lib for working around missing things on various
platforms then i think it would be better to export
a standard API instead of some private one
3. name clashes can only happen if a application links to 2 such getaddrinfo
implementations, and neither can be in a standard lib as in that case
our getaddrinfo code would not have been compiled

this boils down to how much are we willing to add to work around such missing
functions
if someone would post a patch to remove all rednudant code and just leave the
single IPv4/6 code id approve it as long as it works on recent
linux+macosx+bsd

also i think lack of the standard functions has to be shown to be real on
some major platform before any form of getaddrinfo() can be added

[...]

[Ronald S. Bultje]

Hi,

Post by Michael Niedermayer
1. its not libavformats job at all to provide any way to resolve names to
IPs, this really is outside of libavformats area, and would be a huge hack
by itself. So you can expect more resistance here no matter if AVWhatever
or getaddrinfo() libavformat has no busness exporting these

That's fine, I can live with that...

Post by Michael Niedermayer
also i think lack of the standard functions has to be shown to be real on
some major platform before any form of getaddrinfo() can be added

Win2k? Not sure about BeOS or the smaller OSes that ffmpeg appears to
support one way or another. Not that I care much about BeOS, but Win2k could
be a possible interest point for me, at least for the time being, it still
has a large userbase and is - in a way - worth keeping compat with. If you
disagree, that's ok and I'll use getaddrinfo() directly (I may still add a
single wrapper for resolving hosts a la resolve_host() / inetaddr_resolve()
or that getaddrinfo()-wrapper in udp.c), some calls can be merged together
such that it takes less code, I'll just remove AVInetAddr and the other
funcs).

Ronald

[Michael Niedermayer]

On Thu, Nov 08, 2007 at 12:45:13PM -0500, Ronald S. Bultje wrote:
[...]

Post by Ronald S. Bultje

Post by Michael Niedermayer
also i think lack of the standard functions has to be shown to be real on
some major platform before any form of getaddrinfo() can be added

Win2k? Not sure about BeOS or the smaller OSes that ffmpeg appears to
support one way or another. Not that I care much about BeOS, but Win2k could
be a possible interest point for me, at least for the time being, it still

does the network stuff work on win2k at all currently?
and somehow i think that its cygwins/mingws job to provide minimal
implementations of missing functionality, instead of every application
itself containing it (1 vs n code, maintaince, bugs, bug fixes, ...)

for other OSs its similar IMO, the "tradition" of providing every missing
standard function in every application is just idiotic

but again, iam not against a seperate lib in svn which does exactly that,
that is provide such minimal missing standard functionality for OSs which
lack it
such a lib could be used by other applications (even ones unrelated to
audio/video) and i think that would be quite nice ...

[...]

[François Revol]

Post by Michael Niedermayer
[...]

Post by Ronald S. Bultje

Post by Michael Niedermayer
also i think lack of the standard functions has to be shown to be real on
some major platform before any form of getaddrinfo() can be added

Win2k? Not sure about BeOS or the smaller OSes that ffmpeg appears to
support one way or another. Not that I care much about BeOS, but Win2k could
be a possible interest point for me, at least for the time being, it still

does the network stuff work on win2k at all currently?
and somehow i think that its cygwins/mingws job to provide minimal
implementations of missing functionality, instead of every
application
itself containing it (1 vs n code, maintaince, bugs, bug fixes, ...)

It should still work on BeOS, with the pending required patch that is.

Post by Michael Niedermayer
for other OSs its similar IMO, the "tradition" of providing every missing
standard function in every application is just idiotic

It's just what "porting" means... believe me I had much harder things
to port already.

Post by Michael Niedermayer
but again, iam not against a seperate lib in svn which does exactly that,
that is provide such minimal missing standard functionality for OSs which
lack it
such a lib could be used by other applications (even ones unrelated to
audio/video) and i think that would be quite nice ...

That's what I did for BeOS once, I wrote a libposix, but it was quite
painful to maintain. IIRC autotools-based apps already have some
replacement funcs (getpass, ...), we could pick some of those.
The poll() workaround could go there as well.

François.

[Diego Biurrun]

Post by François Revol

Post by Michael Niedermayer
[...]

Post by Ronald S. Bultje

Post by Michael Niedermayer
also i think lack of the standard functions has to be shown to be real on
some major platform before any form of getaddrinfo() can be added

Win2k? Not sure about BeOS or the smaller OSes that ffmpeg appears to
support one way or another. Not that I care much about BeOS, but Win2k could
be a possible interest point for me, at least for the time being, it still

does the network stuff work on win2k at all currently?
and somehow i think that its cygwins/mingws job to provide minimal
implementations of missing functionality, instead of every
application
itself containing it (1 vs n code, maintaince, bugs, bug fixes, ...)

It should still work on BeOS, with the pending required patch that is.

Post by Michael Niedermayer
for other OSs its similar IMO, the "tradition" of providing every
missing standard function in every application is just idiotic

It's just what "porting" means... believe me I had much harder things
to port already.

As Michael said, traditionally porting has meant reinventing the wheel
over and over again. This is free software, if we have control over the
complete platform, let's fix things once in the right place.

Diego

[Rich Felker]

Post by François Revol

Post by Michael Niedermayer
for other OSs its similar IMO, the "tradition" of providing every missing
standard function in every application is just idiotic

It's just what "porting" means... believe me I had much harder things
to port already.

"Porting" is a word I consider harmful. There's a reason we have
portability standards like ISO C, POSIX, etc. and their counterparts
for other languages: it means you can rely on a reasonable amount of
standard interfaces across fairly diverse systems with possibly very
different internal implementations of those interfaces based on the
requirements of the platform.

Certainly there will always be some things that need system-specific
implementations, but having an OS gratuitously omit important
already-standardized interfaces so that every app has to supply its
own is just lame. Note that lots of times when apps do this, their
supplied version is actually BUGGY.

I ran into this issue (buggy app-included implementations) with bash
just yesterday: because my system does not have the GNU asprintf
function, bash insisted on using its own entire printf replacement
rather than implementing asprintf on top of the (standard) snprintf.
And of course they got it wrong, so bash's printf(1) with the %x
format was printing 0x80000000 as -80000000...

Rich

[rachel li]

Hi, I read the code about utiles.c the fuction

int av_write_trailer(AVFormatContext *s)
{
int ret, i;

for(;;){
AVPacket pkt;
ret= av_interleave_packet(s, &pkt, NULL, 1);
if(ret<0) //FIXME cleanup needed for ret<0 ?
goto fail;
if(!ret)
break;

truncate_ts(s->streams[pkt.stream_index], &pkt);
ret= s->oformat->write_packet(s, &pkt);

av_free_packet(&pkt);

if(ret<0)
goto fail;
if(url_ferror(&s->pb))
goto fail;
}

if(s->oformat->write_trailer)
ret = s->oformat->write_trailer(s);
fail:
if(ret == 0)
ret=url_ferror(&s->pb);
for(i=0;i<s->nb_streams;i++)
av_freep(&s->streams[i]->priv_data);
av_freep(&s->priv_data);
return ret;

}

for(;;),why modify for the loop? I remmerber while(s->packet_buffer)

the loop for(::)do what ?I found the outfile

[Nicolas George]

Post by Ronald S. Bultje
As for making a getaddrinfo() for systems that do have it, I guess that's
another way, but then I wouldn't be able to generalize error handling into
the wrapper,

That is a good reason.

Post by Ronald S. Bultje
plus that I would risk (and be asking for) symbol clashes,

They probably can be avoided with a #define.

Post by Ronald S. Bultje
which is especially bad since I want to export this stuff and have ffserver
use the exported symbols (see other antique patches) rather than the current
hacks.

That is also a very good reason IMHO. Although now you have to convince
Michael.

Regards,

[Ronald S. Bultje]

Hi,

Post by Ronald S. Bultje

Post by Ronald S. Bultje
which is especially bad since I want to export this stuff and have

ffserver

Post by Ronald S. Bultje
use the exported symbols (see other antique patches) rather than the

current

Post by Ronald S. Bultje
hacks.

That is also a very good reason IMHO. Although now you have to convince
Michael.

As you may have noticed, I don't think he likes it very much. Maybe I'll
make a lavio (this has been suggested before by Diego and maybe others to
store URI-related stuff) to get this stuff into... Anyone have any ideas as
for this?

Ronald

[Diego Biurrun]

Post by Ronald S. Bultje

Post by Nicolas George

Post by Ronald S. Bultje
which is especially bad since I want to export this stuff and have
ffserver use the exported symbols (see other antique patches)
rather than the current hacks.

That is also a very good reason IMHO. Although now you have to convince
Michael.

As you may have noticed, I don't think he likes it very much. Maybe I'll
make a lavio (this has been suggested before by Diego and maybe others to
store URI-related stuff) to get this stuff into... Anyone have any ideas as
for this?

This would be very much welcome. Refactoring libavformat would earn you
a lot of good karma. Ideally, libavformat should only contain muxers
and demuxers. Of course it's quite far away from this right now...

Diego

[Nicolas George]

Post by Ronald S. Bultje
Maybe I'll
make a lavio (this has been suggested before by Diego and maybe others to
store URI-related stuff) to get this stuff into...

Ouch. If you call it libavio, what name would be left for the library where
oss, video4linux, x11grab, dv1394 and cousins will be moved?

Regards,

[Aurelien Jacobs]

Post by Nicolas George

Post by Ronald S. Bultje
Maybe I'll
make a lavio (this has been suggested before by Diego and maybe others to
store URI-related stuff) to get this stuff into...

Ouch. If you call it libavio, what name would be left for the library where
oss, video4linux, x11grab, dv1394 and cousins will be moved?

Maybe libavdevice. It would include support for various kind of audio and
video rendering and grabbing devices.

Aurel

[Ronald S. Bultje]

Hi,

Post by Aurelien Jacobs

Post by Nicolas George

Post by Ronald S. Bultje
Maybe I'll
make a lavio (this has been suggested before by Diego and maybe others

to

Post by Nicolas George

Post by Ronald S. Bultje
store URI-related stuff) to get this stuff into...

Ouch. If you call it libavio, what name would be left for the library

where

Post by Nicolas George
oss, video4linux, x11grab, dv1394 and cousins will be moved?

Maybe libavdevice. It would include support for various kind of audio and
video rendering and grabbing devices.

I don't think those have the same scope, but we can always make two libs.

Ronald

[Michael Niedermayer]

Hi

Post by Ronald S. Bultje
Hi,

Post by Ronald S. Bultje

Post by Ronald S. Bultje
which is especially bad since I want to export this stuff and have

ffserver

Post by Ronald S. Bultje
use the exported symbols (see other antique patches) rather than the

current

Post by Ronald S. Bultje
hacks.

That is also a very good reason IMHO. Although now you have to convince
Michael.

As you may have noticed, I don't think he likes it very much. Maybe I'll
make a lavio (this has been suggested before by Diego and maybe others to
store URI-related stuff) to get this stuff into... Anyone have any ideas as
for this?

i think there are a few libs which could be created

* low level protocols (tcp,udp,file,oss,alsa,v4l)
* high level protocols (http, ftp, mms, ...)

* a lib providing standard functionality which is missing on the target OS
(exported names though should be prefixed with something to avoid name
clashes)

all just an idea, maybe there are better ways to split things ...

[...]

[Michael Niedermayer]

On Fri, Nov 09, 2007 at 03:46:44AM +0100, Michael Niedermayer wrote:
[...]

Post by Michael Niedermayer
* a lib providing standard functionality which is missing on the target OS
(exported names though should be prefixed with something to avoid name
clashes)

and this could also contain additional non standard functions or wrapers
which are more convenient if the standard ones have some _significant_
disadvantage. such non standard functions though should always have names
which makes it clear that they are not standard (not like what gnu libc does)
or such non standard APIs could even be in a seperate lib as well

again, just random ideas ...

[...]

[Nicolas George]

Post by Michael Niedermayer
i think there are a few libs which could be created
* low level protocols (tcp,udp,file,oss,alsa,v4l)
* high level protocols (http, ftp, mms, ...)

Your ideas are very interesting, but I am not sure about the low level/high
level split.

From an application standpoint, raw TCP and HTTP are one and the same: a
raw stream of bytes (with a small type hint), that needs to go through a
demuxer if it is supposed to be audio-video content. V4L, on the other hand,
is a structured stream of frames.

The current implementation reflects this: TCP and HTTP are both
URLProtocol-s, while video4linux is an AVInputFormat.

I believe that a split in separate library should at least follow the
interface distinction: I really do not see why ALSA and UDP should be in the
same library, for example.

I quite like Aurelien's suggestion "libavdevice" for OSS/ALSA/V4L... And
libavproto(col) is an obvious name for protocols.

Regards,

[Luca Abeni]

Hi all,


Michael Niedermayer wrote:
[...]

Post by Michael Niedermayer
i think there are a few libs which could be created
* low level protocols (tcp,udp,file,oss,alsa,v4l)
* high level protocols (http, ftp, mms, ...)
* a lib providing standard functionality which is missing on the target OS
(exported names though should be prefixed with something to avoid name
clashes)
all just an idea, maybe there are better ways to split things ...

So, what happened to this? Anyone working on those libraries?

I am leaving for the weekend in about one or two hours, and I'll not be
online until monday, but I'll have some time to hack ffmpeg.
If noone else is working on this, I'll split libavformat.

I am thinking about 2 possibilities:
1) a first lib for low level protocols, and a second one for high level
protocols, as suggested by Michael
2) a first library for network protocols (udp, tcp, http, rtp, rtsp) and a second
one for grabbing from other devices (audio, v4l, v4l2, x11grab & friends)

Anyone has other ideas? Which one is preferred?

In my plan, I will simply move code from one library to another, without changing
any API (I hope this can be done ;-). After this split, the only URLProtocols
remaining in libavformat will be file_protocol and pipe_protocol.

Of course, if someone else is already working on this I'll find something else
to hack ;-)

Comments?


Thanks,
Luca

[Stefano Sabatini]

Post by Ronald S. Bultje
Hi all,
[...]

Post by Michael Niedermayer
i think there are a few libs which could be created
* low level protocols (tcp,udp,file,oss,alsa,v4l)
* high level protocols (http, ftp, mms, ...)
* a lib providing standard functionality which is missing on the target OS
(exported names though should be prefixed with something to avoid name
clashes)
all just an idea, maybe there are better ways to split things ...

So, what happened to this? Anyone working on those libraries?
I am leaving for the weekend in about one or two hours, and I'll not be
online until monday, but I'll have some time to hack ffmpeg.
If noone else is working on this, I'll split libavformat.
1) a first lib for low level protocols, and a second one for high level
protocols, as suggested by Michael
2) a first library for network protocols (udp, tcp, http, rtp, rtsp) and a second
one for grabbing from other devices (audio, v4l, v4l2, x11grab & friends)
Anyone has other ideas? Which one is preferred?

What about a libavsource intended as a generalization of libavdevice
(proposed in the second option)?

See:
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/57997/focus=58158

It should make possible to "grab" not only from physical devices but
also from automatic pattern generators, which would be useful for
testing/debugging (not to mention the coolness of having a psychedelic
pattern video/audio generator).

Post by Ronald S. Bultje
In my plan, I will simply move code from one library to another, without changing
any API (I hope this can be done ;-). After this split, the only URLProtocols
remaining in libavformat will be file_protocol and pipe_protocol.
Of course, if someone else is already working on this I'll find something else
to hack ;-)
Comments?

My 2 cents.

Best regards.

[Michel Bardiaux]

Post by Stefano Sabatini

Post by Ronald S. Bultje
Hi all,
[...]

Post by Michael Niedermayer
i think there are a few libs which could be created
* low level protocols (tcp,udp,file,oss,alsa,v4l)
* high level protocols (http, ftp, mms, ...)
* a lib providing standard functionality which is missing on the target OS
(exported names though should be prefixed with something to avoid name
clashes)
all just an idea, maybe there are better ways to split things ...

So, what happened to this? Anyone working on those libraries?
I am leaving for the weekend in about one or two hours, and I'll not be
online until monday, but I'll have some time to hack ffmpeg.
If noone else is working on this, I'll split libavformat.
1) a first lib for low level protocols, and a second one for high level
protocols, as suggested by Michael
2) a first library for network protocols (udp, tcp, http, rtp, rtsp) and a second
one for grabbing from other devices (audio, v4l, v4l2, x11grab & friends)
Anyone has other ideas? Which one is preferred?

What about a libavsource intended as a generalization of libavdevice
(proposed in the second option)?
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/57997/focus=58158
It should make possible to "grab" not only from physical devices but
also from automatic pattern generators, which would be useful for
testing/debugging (not to mention the coolness of having a psychedelic
pattern video/audio generator).

There is no need to add that *in* libavformat. There is the vloopback
device for such effects.

Post by Stefano Sabatini

Post by Ronald S. Bultje
In my plan, I will simply move code from one library to another, without changing
any API (I hope this can be done ;-). After this split, the only URLProtocols
remaining in libavformat will be file_protocol and pipe_protocol.
Of course, if someone else is already working on this I'll find something else
to hack ;-)
Comments?

My 2 cents.
Best regards.

--
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:***@mediaxim.be

Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/

[Luca Abeni]

Hi Michel,

Post by Michel Bardiaux

Post by Stefano Sabatini
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/57997/focus=58158
It should make possible to "grab" not only from physical devices but
also from automatic pattern generators, which would be useful for
testing/debugging (not to mention the coolness of having a psychedelic
pattern video/audio generator).

There is no need to add that *in* libavformat. There is the vloopback
device for such effects.

This is an interesting idea... Do you know about a similar loopback device
for audio? If it exists, we can generate audio/video test patterns for
testing A/V synch issues (that would be very useful for my streaming tests :)


Thanks,
Luca

[Michel Bardiaux]

Post by Luca Abeni
Hi Michel,

Post by Michel Bardiaux

Post by Stefano Sabatini
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/57997/focus=58158
It should make possible to "grab" not only from physical devices but
also from automatic pattern generators, which would be useful for
testing/debugging (not to mention the coolness of having a psychedelic
pattern video/audio generator).

There is no need to add that *in* libavformat. There is the vloopback
device for such effects.

This is an interesting idea... Do you know about a similar loopback device
for audio?

Alas, no! I have been looking for such a device for days now, and havent
found any yet.

I already use a vlc plugin I have written to decode video from a TS,
give it to vloopback, then use a TV-grabber designed for TV-boards. That
works well, so I consider it a proof-of-concept, and would love to do
the same for audio.

Note that I am quite unfamiliar with ALSA devices, so maybe there is
something in alsa...

Post by Luca Abeni
If it exists, we can generate audio/video test patterns for
testing A/V synch issues (that would be very useful for my streaming tests :)

Good idea!
--
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:***@mediaxim.be

Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/

[Nicolas George]

Post by Michel Bardiaux
There is no need to add that *in* libavformat. There is the vloopback
device for such effects.

While vloopback does the work, I do not believe it fills all the needs for
such a pseudo-format:

- It is system-dependant (unless I am mistaken, vloopback is a part of V4L,
which is Linux-only; or are you talking of something else with the same
name?).

Well, piping raw video to ffmpeg does not have the same problem.

- It does not provide audio-video sync.

- It relies on more complex code path, which is not a good thing when the
purpose is debugging.

- It require a separate tool, which will either never be near at hand, or
will clutter the command namespace.

- It will never be as simple as "-f test_pattern -i x".

On the other hand, a format in libavdevice would be very easy to use,
available by default with a specific namespace, with a negligible overhead,
which can be altogether avoided if binary size is an issue.

All things considered, I believe it would be very practical to have it.

Regards,

[Diego Biurrun]

Post by Luca Abeni
I am leaving for the weekend in about one or two hours, and I'll not be
online until monday, but I'll have some time to hack ffmpeg.
If noone else is working on this, I'll split libavformat.
1) a first lib for low level protocols, and a second one for high level
protocols, as suggested by Michael

I think you might easily run into problems trying to find a distinction
between low-level and high-level protocols...

Post by Luca Abeni
2) a first library for network protocols (udp, tcp, http, rtp, rtsp) and a second
one for grabbing from other devices (audio, v4l, v4l2, x11grab & friends)
Anyone has other ideas? Which one is preferred?

I'd go for the second or just move everything that is not a muxer or
demuxer out of libavformat into a single lib.

Post by Luca Abeni
In my plan, I will simply move code from one library to another, without changing
any API (I hope this can be done ;-). After this split, the only URLProtocols
remaining in libavformat will be file_protocol and pipe_protocol.

Why keep them?

Diego

[Luca Abeni]

Hi all,

Post by Diego Biurrun

Post by Luca Abeni
I am leaving for the weekend in about one or two hours, and I'll not be
online until monday, but I'll have some time to hack ffmpeg.
If noone else is working on this, I'll split libavformat.

I did some experiments, and I have a patch almost ready. I'll post it
tomorrow, after fixing some things, and updating the patch to option 2),
since it seems that people like it better.


[...]

Post by Diego Biurrun

Post by Luca Abeni
In my plan, I will simply move code from one library to another, without changing
any API (I hope this can be done ;-). After this split, the only URLProtocols
remaining in libavformat will be file_protocol and pipe_protocol.

Why keep them?

Because without at least the file protocol libavformat is not usable.
In my plan, libavformat should only depend on libavcodec and libavutil.
Then, we will have a libavnet and libavgrab (or whatever names we prefer)
depending on libavformat, which will provide additional protocols and grabbing
formats.


Luca

[Nicolas George]

Post by Luca Abeni
I am leaving for the weekend in about one or two hours,

I guess my mail will be too late then.

Post by Luca Abeni
and I'll not be
online until monday, but I'll have some time to hack ffmpeg.
If noone else is working on this, I'll split libavformat.

That would be good news.

Post by Luca Abeni
2) a first library for network protocols (udp, tcp, http, rtp, rtsp) and a
second one for grabbing from other devices (audio, v4l, v4l2, x11grab &
friends)

Only grabbing? I had the idea that output devices would have their place in
the same library as well.

Post by Luca Abeni
Anyone has other ideas? Which one is preferred?

I am all in favor of 2.

Post by Luca Abeni
In my plan, I will simply move code from one library to another, without
changing any API (I hope this can be done ;-).

What do you intend to do concerning the registering of pseudo-formats? Keep
a single list or a separate list? If this is a single list, register with
av_register_all?

Regards,

[Luca Abeni]

Hi Nicolas,

Nicolas George wrote:
[...]

Post by Nicolas George

Post by Luca Abeni
2) a first library for network protocols (udp, tcp, http, rtp, rtsp) and a
second one for grabbing from other devices (audio, v4l, v4l2, x11grab &
friends)

Only grabbing? I had the idea that output devices would have their place in
the same library as well.

Ok; I said grabbing because right now it contains mainly "grabbing demuxers".
But I have no problems in calling it with a different name and adding output
devices or other things.

Post by Nicolas George

Post by Luca Abeni
Anyone has other ideas? Which one is preferred?

I am all in favor of 2.

Ok; I'll post a patch for 2) tomorrow.

Post by Nicolas George

Post by Luca Abeni
In my plan, I will simply move code from one library to another, without
changing any API (I hope this can be done ;-).

What do you intend to do concerning the registering of pseudo-formats? Keep
a single list or a separate list? If this is a single list, register with
av_register_all?

I plan to export an avnet_register_all() function from libavnet and an
avgrab_register_all() function from libavgrab. If users want to register
all the networking and grabbing formats, they can call such functions.
Otherwise, I think a user can also register a single protocol and/or
format by directly calling av_register_*.



Luca

[Luca Abeni]

Hi Nicolas,

Nicolas George wrote:
[...]

Post by Nicolas George

Post by Luca Abeni

Post by Nicolas George
- Simply drop the IPv4-only code.
- If people complain that ffmpeg does not build anymore on their old
operating system, write a limited implementation of getaddrinfo.

As I said, I do not like this idea too much, and I'd prefer to arrive to
this result by committing small patches.

- Implement a version of getaddrinfo and getnameinfo suitable for ffmppeg's
needs.
- Drop the IPv4-only code.

I am considering this approach... But before dropping the IPv4-only code I want
to be reasonably sure that the protocol-independent code is ok.
The point of the patches I just sent is to make sure that the code under
"CONFIG_IPV6" is equivalent to the "old" code.

I am currently looking at udp_ipv6_set_local(), and I have some doubts...
For example, the protocol-independent code seems to forget about SO_REUSEADDR
(question: does "setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, ...)" work on
IPv6 sockets too, or is a different setsockopt needed for the IPv6 case?)
Also, I see the following code:

res0 = udp_ipv6_resolve_host(0, s->local_port, SOCK_DGRAM, family, AI_PASSIVE);
if (res0 == 0)
goto fail;
for (res = res0; res; res=res->ai_next) {
udp_fd = socket(res->ai_family, SOCK_DGRAM, 0);
if (udp_fd > 0) break;
perror("socket");
}

if (udp_fd < 0)
goto fail;

if (bind(udp_fd, res0->ai_addr, res0->ai_addrlen) < 0) {

Is this correct? Shouldn't the bind() be performed using "res" instead of "res0"?
I mean: doesn't the current code risk to open the socket for a family and to
bind to an address belonging to a different family?


Thanks,
Luca

[Nicolas George]

Post by Luca Abeni
For example, the protocol-independent code seems to forget about SO_REUSEADDR
(question: does "setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, ...)" work on
IPv6 sockets too, or is a different setsockopt needed for the IPv6 case?)

It is the same for all socket families (that is supposed to be the meaning
of SOL_SOCKET: works for all sockets, and in that case that is true).

You are right, the call is missing.

Post by Luca Abeni
res0 = udp_ipv6_resolve_host(0, s->local_port, SOCK_DGRAM, family, AI_PASSIVE);
if (res0 == 0)
goto fail;
for (res = res0; res; res=res->ai_next) {
udp_fd = socket(res->ai_family, SOCK_DGRAM, 0);
if (udp_fd > 0) break;
perror("socket");
}
if (udp_fd < 0)
goto fail;
if (bind(udp_fd, res0->ai_addr, res0->ai_addrlen) < 0) {
Is this correct? Shouldn't the bind() be performed using "res" instead of "res0"?

I think you found another bug indeed.

And I believe that the perror (which should probably be a av_log) should be
called only when all addresses have failed.

And this snippet will give very strange results if the standard input have
been closed: the correct test is probably udp_fd >= 0.

Regards,

[Luca Abeni]

Hi Nicolas,

On Thu, 2007-11-08 at 18:41 +0100, Nicolas George wrote:
[...]

Post by Nicolas George

Post by Luca Abeni
if (bind(udp_fd, res0->ai_addr, res0->ai_addrlen) < 0) {
Is this correct? Shouldn't the bind() be performed using "res" instead of "res0"?

I think you found another bug indeed.

Ok, thanks for confirming. Now I know that I did not misunderstand the
code ;-)
BTW, I think that the whole udp_ipv6_set_local() is quite broken: why
should "socket(res->ai_family, SOCK_DGRAM, 0)" fail? After all, "res"
has been returned by getaddrinfo()... So, is getaddrinfo() supposed to
return addresses belonging to unsupported families?
I understand why getaddrinfo() can return more than 1 address, but it
seems to me that the "for (res = res0; res; res=res->ai_next)" loop in
udp_ipv6_set_local() is useless...

I also agree with your comment about perror().

So, before removing the "old code", I'd really like to be sure that the
protocol-independent code is ok... And it seems to me that this code
still needs some work.


Luca

[Nicolas George]

Post by Luca Abeni
BTW, I think that the whole udp_ipv6_set_local() is quite broken: why
should "socket(res->ai_family, SOCK_DGRAM, 0)" fail? After all, "res"
has been returned by getaddrinfo()... So, is getaddrinfo() supposed to
return addresses belonging to unsupported families?

I think it can. For example, imagine a GNU/Linux system: the glibc knows of
IPv6, but the Linux kernel can be built with IPv6 disabled (or as a module,
not loaded). In that case, getaddrinfo could reasonably return the IPv6
address although the kernel will not be able to use it: creating a socket is
not the only reason to resolve a name.

And even if the goal is to create a socket, the return code is relevant: for
example, if I type "ssh she-seel" (she-seel is my laptop, it has an usual
IPv6 global unicast address, but no public IPv4 address), and it fails, a
different error message will help me to solve the problem:

Host not found -> I made a mistake typing the host name, or the DNS is broken
Address family not supported -> the kernel lacks IPv6 support
No route to host -> the host lacks IPv6 connectivity

Regards,

[Luca Abeni]

Hi Nicolas,

Post by Nicolas George

Post by Luca Abeni
BTW, I think that the whole udp_ipv6_set_local() is quite broken: why
should "socket(res->ai_family, SOCK_DGRAM, 0)" fail? After all, "res"
has been returned by getaddrinfo()... So, is getaddrinfo() supposed to
return addresses belonging to unsupported families?

I think it can. For example, imagine a GNU/Linux system: the glibc knows of
IPv6, but the Linux kernel can be built with IPv6 disabled (or as a module,
not loaded). In that case, getaddrinfo could reasonably return the IPv6
address although the kernel will not be able to use it

Well, I guess the manpage is wrong, then. It says "...IPv6 socket
address structures can be created if IPv6 support is available"

Ok, I'll not trust manpages again


[...]

Post by Nicolas George
And even if the goal is to create a socket, the return code is relevant: for
example, if I type "ssh she-seel" (she-seel is my laptop, it has an usual
IPv6 global unicast address, but no public IPv4 address), and it fails, a

I was specifically talking about udp_ipv6_set_local()...


Luca

[Nicolas George]

Post by Luca Abeni
Well, I guess the manpage is wrong, then. It says "...IPv6 socket
address structures can be created if IPv6 support is available"

This can be understood as "available _in the libc_", and not "available in
the libc and the kernel". I quickly checked in the glibc sources, and I did
not see any check for kernel support of IPv6 sockets.

Post by Luca Abeni
I was specifically talking about udp_ipv6_set_local()...

I was trying to explain why getaddrinfo can reasonably return addresses in a
family not supported by the kernel. udp_ipv6_set_local must be written
accordingly.

Regards,