[FFmpeg-devel] [RFC] libavformat: URL reassembly with IPv6 addresses

[Martin Storsjö]

On Thu, 4 Mar 2010, Luca Abeni wrote:

> Martin Storsj? wrote:
> [...]
> > > I looked at it now ;-)
> > > Looks ok; I just have 2 comments (uhmm... Maybe 3 :):
> > > 1) It seems to me that the authorization only makes sense when a
> > >    protocol is specified... No? So, I'd put the
> > > 	+    if (authorization)
> > > 	+        av_strlcatf(str, size, "%s:", authorization);
> > >    inside the "if (proto)"
> > 
> > Hmm, I don't see these fields as all that tightly coupled,
> 
> Well, my point is that a protocol is needed for exchanging usernames
> and passwords... So, having authorisation entries without protocols
> does not look like a good idea to me.
> Anyway, this is all questionable, so this comment can be skipped ;-)

Yes, a protocol normally is needed for exchanging usernames, but this 
function may be used for creating URL-like fragments, too.

In HTTP, for example, you need to send the Host: header, which needs the 
same escaping. When requesting something from a server with a numerical 
IPv6 address, that header line looks like this:
Host: [1234:5678::1]:8080

So we use the same function, with a host name and port number, but no 
protocol, auth or path. Given this rationale, I guess there could be 
situations where one would want to assemble a URL-like fragment with auth, 
without a protocol.

> [...]
> > > Moreover, I would not export this function, so I'd put the
> > > prototype somewhere else than in avformat.h. And I am not
> > > sure if utils.c is the right place for implementing it (maybe,
> > > it can be implemented in some C file that already includes
> > > network.h).
> > 
> > It's not publicly exported at the moment, it's within #ifdef
> > HAVE_AV_CONFIG_H in avformat.h, where a few other internal, libavformat
> > common functions are declared.
> 
> Uh... Interesting. I was under the impression that such functions should
> be in internal.h... :)

Ah, that's probably also a good candidate for placing this. That would 
require adding a few #include "internal.h" to most of the C files I'm 
changing, though.

When discussing with Ronald, we concluded that url_split isn't public at 
the moment, and this probably should go next to url_split, but I'm open to 
other opinions.

// Martin