[vlc-devel] [PATCH] http: handle shoutcast/icecast (ICY) metadata

[Ludovic Fauvet]

On Tue, Feb 23, 2016, at 02:45, Rémi Denis-Courmont wrote:
> Le 2016-02-22 19:24, Ludovic Fauvet a écrit :
> >> My point was that icyx://... URLs are handled by the old HTTP plugin
> >> just fine.
> >>
> >> I don't have any strong opinion on separating ICY from old HTTP.
> >
> > Following your previous suggestions I'm reimplementing the module as 
> > a
> > stream filter instead. It's most certainly the best choice both for 
> > code
> > clarity and layer separation. Only few lines will be required in the
> > HTTP code to fetch some headers.
> 
> I doubt that would really work, or rather, to make it work, you would 
> presumably need about as many ugly hacks as you would with an access.

Of course, the access still has work to do and without passing all
headers to other modules in the chain there's not way to improve the
situation.

> In particular, you would still need to deal with the first Icy-Name and 
> Icy-Genre values in the response header, and with the ICY protocol in 
> place of HTTP also in the response header. And you'd need a private 
> access/stream control, or something, to carry the Icy-MetaInt value. 
> That would not only need changes in the access, but also any potentially 
> intermediate stream filters.
> 
> >> > It goes without saying this will
> >> > cause another kind of duplication (4 HTTP implementation instead 
> >> of
> >> > 3)
> >> > and that it won't work with HTTP/2.0 (do we care?).
> >>
> >> ICY is _not_ HTTP. It's not proper HTTP/1.x and it most certainly 
> >> won't
> >> be HTTP/2.0. If you connect to an ICY server using http:// URI with
> >> current vlc.git, the plugin just fails to parse the server response.
> >> Open() fails and then the core falls back to the old plugin. In 
> >> fact, it
> >> still works, albeit with 2 extra RTTs.
> >
> > Nope, it _does_ work with the new module
> 
> If you use "http" scheme, the HTTPS plugin will try first, but it will 
> reject the "ICY" protocol value in the HTTP response message header. 
> Compare modules/access/http.c:1029.
> 
> However if you have an actual HTTP server adding ICY metadata, then the 
> metadata do get dropped. That should be trivial to fix with a access 
> redirect to icyx://...

Yes, like the patch you pushed few days ago. But this enforce the
dependency on the legacy module. I don't think we'll want to keep it
around forever.

> > but not in HTTP2 mode of course.
> 
> I doubt we will ever see HTTP/2 over TCP anyway. Apache supports it, 
> but none of the browsers do nor do they officially plan to support it. 
> So probably nobody will deploy it.

Indeed.

Anyway, I've still made a patch to move icy parsing to a stream filter
because I wanted to see how difficult it would be to modularize this
part.
The patch isn't perfect yet but I don't want to spend too much time
working on it if it will never be accepted. 

As usual, comments and advice are welcome.

-- 
Ludovic Fauvet
www.videolan.org