Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty metadata when streaming to icecast with opus encoder #70

Closed
JustArchi opened this issue Jun 26, 2017 · 25 comments
Closed

Empty metadata when streaming to icecast with opus encoder #70

JustArchi opened this issue Jun 26, 2017 · 25 comments
Labels

Comments

@JustArchi
Copy link

JustArchi commented Jun 26, 2017

Hello.

Consider following audio_output:

audio_output {
    type "shout"
    encoder "opus"
    name "Test Radio"
    host "localhost"
    port "8000"
    mount "/test.opus"
    password "hackme"
    always_on "yes"
    quality "10"
    format "48000:16:2"
    public "yes"
}

With above audio output, streaming to icecast server works totally fine and I verified that data is indeed encoded into opus. Sadly, there is no metadata being announced, which makes it impossible to guess song that is currently being played - only name of the radio station is being displayed.

Since that info is available in other encoders, mainly vorbis, could it be added into opus encoding as well? I'd like to use opus encoder with MPD metadata.

Thank you in advance.

@MaxKellermann
Copy link
Member

Unfortunately, Opus does not support Vorbis comments, therefore adding tag support requires implementing it from scratch. But what MPD falls back to is Icy tags, which it enables if your streaming client asks for it. Please confirm that MPD did not send Icy tags, even though your client requested it!

@JustArchi
Copy link
Author

I have no clue how to confirm that.

Verbose MPD log:

Jul 21 00:44 : zeroconf: No global port, disabling zeroconf
Jul 21 00:44 : state_file: Loading state file /var/lib/mpd/state
Jul 21 00:44 : playlist: queue song 93:"IRON ATTACK!/2008.05.25 [MIA-007] Blade Of Ancient Temple [例大祭5]/WALTZ OF PUPPET.ogg"
Jul 21 00:44 : decoder_thread: probing plugin vorbis
Jul 21 00:44 : decoder_thread: probing plugin oggflac
Jul 21 00:44 : decoder_thread: probing plugin opus
Jul 21 00:44 : decoder: audio_format=48000:16:2, seekable=true
Jul 21 00:44 : inotify: initializing inotify
Jul 21 00:44 : output: opened plugin=shout name="Archi's Touhou Radio [HQ 128K Opus]" audio_format=48000:16:2
Jul 21 00:44 : inotify: watching music directory
Jul 21 00:44 : state_file: Saving state file /var/lib/mpd/state

But I noticed something that can be a clue why icy tags are not working - when I try to update metadata from Icecast admin panel, I get:

Message: Mountpoint will not accept URL updates

Return Code: 1

Probably Icecast for some reason doesn't have icy metadata on for opus mountpoint, and since MPD doesn't write opus metadata either, fallback doesn't work by default.

I'm wondering if we can't enable metadata write in opus ogg container until we can get proper opus metadata support. Last time I checked, VLC nicely displayed ogg metadata that were included in opus container, even though it's not the "default" way of doing that. What I mean is that VLC can handle both opus metadata, as well as ogg metadata, and since we're using ogg container for opus already, maybe we can use it as a workaround until we can implement proper support for opus metadata.

Anyway, I'll try to reach icecast folks over not accepting URL updates, as that's part of the issue I'm having. However, having proper support for opus metadata in MPD can be nice as well, so I'm leaving the issue open, unless you want to close it of course.

In any case, thanks a lot for your response and entire work on MPD! 💓.

@JustArchi
Copy link
Author

JustArchi commented Aug 28, 2017

If it helps, I compiled latest icecast from source (version 2.4.99.1) and I took a look into the logs.

[2017-08-28  18:43:33] DBUG connection/_handle_authentication_mount_normal Trying <mount type="normal"> specific authenticators for client 0x55ed3d97a400.
[2017-08-28  18:43:33] DBUG connection/_handle_authentication_mount_default Trying <mount type="default"> specific authenticators for client 0x55ed3d97a400.
[2017-08-28  18:43:33] DBUG auth/auth_addref ...refcount on auth_t (null) is now 3
[2017-08-28  18:43:33] DBUG auth/auth_add_client Trying to add client 0x55ed3d97a400 to auth 0x55ed3d8e4c90's (role legacy-global-source) queue.
[2017-08-28  18:43:33] DBUG auth/auth_addref ...refcount on auth_t (null) is now 4
[2017-08-28  18:43:33] INFO auth/auth_add_client adding client 0x55ed3d97a400 for authentication on 0x55ed3d8e4c90
[2017-08-28  18:43:33] DBUG auth/queue_auth_client ...refcount on auth_t (null) is now 4
[2017-08-28  18:43:33] DBUG auth/__handle_auth_client client 0x55ed3d97a400 on auth 0x55ed3d8e4c90 role legacy-global-source processed: ok
[2017-08-28  18:43:33] DBUG connection/_handle_get_request Got client 0x55ed3d97a400 with URI /admin/metadata
[2017-08-28  18:43:33] DBUG connection/_handle_get_request Client 0x55ed3d97a400 requesting admin interface.
[2017-08-28  18:43:33] DBUG admin/admin_handle_request Admin request (/admin/metadata)
[2017-08-28  18:43:33] DBUG admin/admin_handle_request Got command (metadata)
[2017-08-28  18:43:33] INFO admin/admin_handle_request Received admin command metadata on mount "/touhou.opus"
[2017-08-28  18:43:33] DBUG admin/command_metadata Got metadata update request
[2017-08-28  18:43:33] DBUG fserve/fserve_add_client Adding client 0x55ed3d97a400 to file serving engine
[2017-08-28  18:43:33] DBUG fserve/fserve_add_pending fserve handler waking up
[2017-08-28  18:43:33] DBUG auth/auth_release ...refcount on auth_t (null) is now 3
[2017-08-28  18:43:33] DBUG client/client_destroy Called to destory client 0x55ed3d97a400
[2017-08-28  18:43:33] DBUG auth/auth_release ...refcount on auth_t (null) is now 2
[2017-08-28  18:43:33] DBUG fserve/fserv_thread_function fserve handler exit

Most important part is right here:

[2017-08-28  18:43:33] DBUG admin/admin_handle_request Admin request (/admin/metadata)
[2017-08-28  18:43:33] DBUG admin/admin_handle_request Got command (metadata)
[2017-08-28  18:43:33] INFO admin/admin_handle_request Received admin command metadata on mount "/touhou.opus"
[2017-08-28  18:43:33] DBUG admin/command_metadata Got metadata update request

This means that MPD properly tries to use icy tags, however, icecast doesn't support them for Opus streams.

Message: Mountpoint will not accept URL updates

But according to this, there is some support for Opus tags inside icecast, probably from the stream itself, via OpusTags header. I understand that MPD does not transmit that header to icecast just yet, as it's not implemented and it tries to update metadata based on URL metadata instead. Perhaps it could be considered as useful enhancement in the future?

@1985a
Copy link

1985a commented Sep 28, 2017

Thank you for posting this. I have the same issues since a few months but my English is not very good at this time, due to my mother language is Spanish and I felt that maybe MaxKellermann wouldn't understand me.

And thanks Max for making MPD a great program to handle music.

@cathugger
Copy link
Contributor

I've been trying to modify opus encoder's code to add tags support. So far I got it working initially (it sends proper metadata when I connect via http) but it's still incorrect for changing track.
I think I didn't properly terminate old stream, mpv throws errors about currupted packets on track change and doesn't show metadata.

@JustArchi
Copy link
Author

JustArchi commented Oct 31, 2017

That's great to hear @cathugger! Sadly I'm not able to help since I have no clue myself how it should work, but I'm very happy to hear that somebody decided to pick it up. Please don't hesitate to send PR once you get it fully working, and all the best! 👍

@cathugger
Copy link
Contributor

@JustArchi I don't have too much clue myself tbh, I barely got it work initially but because of not knowing all details I wasn't able to make it work properly no matter how much random tweaks I threw at it.
I'd need to properly dig into it to get some clue how it's supposed to work (track switching is hinted in standard itself), but I'm not sure if I have time for that atm...

@cathugger
Copy link
Contributor

@JustArchi offtopic, but don't use "quality" parameter for opus codec, as it's ignored. use "bitrate" instead.

@cathugger
Copy link
Contributor

https://github.com/cathugger/MPD/tree/opustags it's incomplete but somehow works on vlc, I'm pretty sure I'm doing something wrong still

@JustArchi
Copy link
Author

I've compiled latest MPD from source with your changes and I'm very positively shocked - it does indeed seem to work just fine with my VLC! 💓

You've done incredible, I'm keeping this build around for a while and looking for future improvements. Please send @MaxKellermann a pull request once you feel that you're satisfied with it so we can have it reviewed and merged into the upstream. I'll also be happy to test any changes you do in the meantime.

In short, huge kudos to you! 💓

@cathugger
Copy link
Contributor

after looking around for a bit, I stumbled upon https://trac.videolan.org/vlc/ticket/18401 and https://jmvalin.ca/misc_stuff/chain_works.opus which is supposed to be well-encoded but mpv throws exactly same errors as my for code..
I know that my code is currently horrible but it looks like not all players are currently supporting it either.

@cathugger
Copy link
Contributor

at this point it seems that having a flag to opt-in opus stream chaining support would be desirable

@cathugger
Copy link
Contributor

should I name option "chaining" or "opustags"? I'm currently thinking about "chaining", as it's more technical and could be applied for flac-in-ogg too (incase that would get implemented in future).

@cathugger
Copy link
Contributor

it seems to kinda work for vlc but it still complains about some mismatches, and I'm unsure about clients which doesn't support chained opus yet (like mplayer and mpv), they probably will actually play pre-skip data as silence as they don't parse headers properly and don't take pre-skip value in them into account...
we should also consider using https://github.com/xiph/libopusenc as it could do pretty much all the chaining work for us, but it's not yet in packages of most distros, so maybe do separate encoder using that?

@cathugger
Copy link
Contributor

Anyways, I'll wait for any of mpd devs to review current code before making PR.

@cathugger
Copy link
Contributor

cathugger commented Nov 7, 2017

I did some tests with various players and tracks which are meant to be played in gapless way.
Here's my results:

  • mplayer and mpv doesn't recognize headers and produce small crack sound regardless if I include pre-skip or not. maybe with pre-skip it's smaller but I'm not sure
  • vlc tries to recognize headers and successfully updates metadata, but still produces small crack regardless. not sure about pre-skip effect there either
  • firefox works flawlessly regardless if I include pre-skip or not
  • gst-play works flawlessly regardless of pre-skip
  • chromium produces noticeable gap with pre-skip, without pre-skip it produces noticeable crack. I think it produces gap in either way, just with pre-skip enabled it's bigger.

So https://wiki.xiph.org/OggOpusImplementation seems to be accurate.
What do you think about any of this, @MaxKellermann ?

@cathugger
Copy link
Contributor

  • foobar2000 plays fine with or without pre-skip, updates tags properly, I couldn't notice gaps

@cathugger
Copy link
Contributor

I changed my mind about using "chaining" to indicate feature, as support of clients will depend on particular codec.
Vorbis chaining got good support at this point, and mpd encodes ogg vorbis streams in this way already.
It seems that ogg/flac support would be different too.
Therefore, I'll change option to enable this to "opustags".

@JustArchi
Copy link
Author

@MaxKellermann Could you help us with any of this? I'd love to have this sorted out, and it seems @cathugger already managed to cover majority of what has to be fixed, we're only after that stutter now, that I also reproduced quite reliably. I love how we have half-baked implementation ready, proving that what we're doing here is in fact possible, we just need somebody with enough knowledge to glue this all together and fix once for good 🙂

@cathugger
Copy link
Contributor

@JustArchi I'm actually quite confident with my current implementation, but poor client support is quite disappointing.
It's not going to get better unless clients are fixed.
I could rebase my stuff on top of current master (files affected didn't change much) but I wanted to hear opinion from @MaxKellermann first.

@cathugger
Copy link
Contributor

this ticket seems to be forgotten at this point.
I guess I'll just make PR.

@MaxKellermann
Copy link
Member

Oh, yes, please. Always PR if you have code, even if you're not sure if it's good enough yet.

@cathugger
Copy link
Contributor

#205

@cathugger
Copy link
Contributor

@JustArchi it got merged to master

@JustArchi
Copy link
Author

Since the feature eventually made it in, for which I'm very grateful, I'll close the issue for now since there is no ongoing work required for this.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants