diff mbox

[FFmpeg-devel] Icecast protocol implementation doesn't parse "/" as a valid mountpoint

Message ID 1548861262.11352.2.camel@bmcs.one
State New
Headers show

Commit Message

c0re Jan. 30, 2019, 3:14 p.m. UTC 
On Tue, 2019-01-22 at 22:06 +0100, Marvin Scholz wrote:
> On 22 Jan 2019, at 20:29, c0re wrote:
> 
> > Hi all,
> > 
> > Today as I tried to implement transcoding a videofeed to an
> > icecast
> > audio-only stream I have encountered some behavior that, in my
> > opionion, leaves room for improvement.
> > 
> > The setup used in this scenario consists of 
> > * an Open Broadcaster Studio (OBS) instance sending a video/audio
> > feed
> > * an nginx endpoint to receive and re-stream said feed to various
> > hosts (including localhost) 
> > * and lastly an AzuraCast 
> > (https://github.com/AzuraCast/AzuraCast)instance which employs 
> > Icecast2.4 for audio broadcasting.
> > 
> > AzuraCast allows streamer/DJ accounts to live-broadcast to any of
> > its
> > radio-stations by connecting through a Broadcasting software of
> > your
> > choice (noteworthy mentions are BUTT or Mixxx) to a dedicated port
> > (e.g. 8005) and a static[1] mountpoint "/" and providing
> > credentials.
> 
> This sounds quite weird, it would make a lot more sense to just use
> the
> same port and different mountpoints for this.
> The lack of multiple mountpoints is an old SHOUTcast limitation and
> Icecast does not suffer from it.

Having looked further into the issue I can now say that the behavior
is derived from Liquidsoaps' handling of so-called harbors.
If you google around a bit you'll see that the trailing slash (harbor
named = "") is quite a popular scheme around there. 
So indeed the issue isn't _exactly_ icecast specific. 

> > 
> > [1]static as in there is no easy way for me to change this design
> > consideration)
> > 
> > The way in which the icecast protocol is currently implemented in
> > ffmpeg, it will check for characters after the trailing slash and
> > assume this as a valid mountpoint.
> > 
> > A trailing slash by itself will always be discarded with the
> > following
> > error: 
> > 
> > 	"No mountpoint (path) specified!" 
> > 
> > see: https://ffmpeg.org/doxygen/2.5/icecast_8c_source.html 
> > [lines 157-161]
> > 
> > so icecast://source:pass@host:port/ is considered invalid by
> > ffmpeg (I
> > cannot think of a quick and dirty workaround either).
> 
> Hi,
> yes indeed Icecast does allow a mountpoint of /, but I would rather
> not
> allow that in the FFMpeg Icecast protocol helper, as IMO using / for
> a
> mountpoint is not a good idea. Users could accidentally use / as
> mount-
> point without this check and would get confused why it does not work
> out
> of the box (as by default / is an internal redirect to the status
> page).
> 
> I wonder why AzuraCast decides to use / as mountpoint and why it
> does
> not even offer you a way to change it.
> 
> Regards,
> Marvin Scholz

I filed an issue with AzuraCast which now has been resolved and an
implementation for custom named Liquidsoap harbors (which translate to
icecast streaming mount-points) is now fully implemented. 

For what it's worth I also gave the ol' hack a go and patched
icecast.c and compiled from source.
Lo and behold... the change of a single character delivered the
desired result without any apparent drawbacks. 


         goto cleanup;

======================================================================

I leave it up to anyone whether or not that is the more sane
implementation (according to spec) or not. :)

Kind Regards
c0re

> > 
> > To my understanding assigning a lone slash as a mountpoint name
> > isn't
> > considered invalid by the IceCast specification and therefore I
> > suggest that it would be an enhancement to lift this current
> > limitation on the way that mountpoints are parsed in an
> > ffmpeg+icecast
> > instruction.
> > 
> > kind regards,
> > c0re_______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Comments

Carl Eugen Hoyos Jan. 30, 2019, 8:15 p.m. UTC | #1 
2019-01-30 16:14 GMT+01:00, c0re <c0re@bmcs.one>:

> diff --git a/libavformat/icecast.c b/libavformat/icecast.c

We can only read patches made with "git format-patch".

Carl Eugen
Marvin Scholz Jan. 30, 2019, 8:33 p.m. UTC | #2 
On 30 Jan 2019, at 21:15, Carl Eugen Hoyos wrote:

> 2019-01-30 16:14 GMT+01:00, c0re <c0re@bmcs.one>:
>
>> diff --git a/libavformat/icecast.c b/libavformat/icecast.c
>
> We can only read patches made with "git format-patch".

I think this was just to illustrate how easy it
would be to change the behavior to work in the
expected way.

So far there did not seem to be much demand
in being able to do this and due to the mentioned
concerns (see my previous mail) I am not really
convinced this should be changed. But I am happy
to discuss this.

This was the first time someone actually asked
to allow to stream to mount '/' with the Icecast
protocol in ffmpeg, at least that I am aware of.

Icecast has mountpoints for a reason and not
using them seems quite weird.

>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

======================================================================

diff --git a/libavformat/icecast.c b/libavformat/icecast.c
index c93b06b553..aca043f59b 100644
--- a/libavformat/icecast.c
+++ b/libavformat/icecast.c
@@ -155,7 +155,7 @@  static int icecast_open(URLContext *h, const char
*uri, int flags)
              s->pass ? s->pass : "");
 
     // Check for mountpoint (path)
-    if (!path[0] || strcmp(path, "/") == 0) {
+    if (!path[0] || strcmp(path, "") == 0) {
         av_log(h, AV_LOG_ERROR, "No mountpoint (path) specified!\n");
         ret = AVERROR(EIO);