Open Bug 965653 Opened 11 years ago Updated 2 years ago

Support PulseAudio metadata

Categories

(Core :: Audio/Video: cubeb, enhancement, P4)

Product:
Core
Component:
Audio/Video: cubeb
Platform:
All
Linux
Type:
enhancement

Tracking

()

People

(Reporter: BenB, Unassigned)

Details

(Keywords: good-first-bug)

Attachments

(3 files)

SongBird via PulseAudio showing current song and artist in PulseAudio volume manager
(deleted), image/png
Details
set AudioStream name to URI spec
(deleted), patch
Details | Diff | Splinter Review
Screenshot, playing YouTube, on Firefox 93 on Ubuntu with Pulse - shows page title
(deleted), image/png
Details
Since recently, we support PulseAudio as audio backend. One of the nice features of Pulse is that the application can name the streams, i.e. tell the system what is playing. This is useful, because the user can control each stream individually, volume and output, in the volume control manager. I'll attach a screenshot how that looks for SongBird. Same for XMMS2, IIRC. Given that the browser can play a great variety of streams from different sites (YouTube, Spotify, yourfavartist.younameit) and features like WebRTC or new mail notifications, it makes sense to allow the user to see where the sound is coming from. This is particularly useful, * if the user doesn't know which site and tab is making noise right now, or * when he has both video chat via WebRTC and YouTube open. The stream metadata should contain the domain name of the site, and the page title. Ifg the <video>/<audio> element provides meta data - e.g. currently playing song for web radios, see bug 778050 and bug 908902 and media.mozGetMetadata(), this should be shown as well, but in a different field, and sanitized. Related bugs: Bug 837563 - PulseAudio enabled on Linux by default Bug 844818 - PulseAudio in WebRTC Bug 778050 - Song metadata from ogg streams in <audio> Bug 908902 - Song metadata from mp3 streams in <audio>
Assignee: kinetik → nobody
Whiteboard: [good first bug]
> [good first bug] Can you put in some pointers where a dev would start? The pulseaudio code is here [1], but where is this called, i.e. hooked to the page, so that we can get DOM info from the nsIDocShell into this audio output? [1] http://mxr.mozilla.org/comm-central/source/mozilla/media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc
Nope, the code is here [1]. Your link is the input side, for WebRTC/getUserMedia stuff. It goes, from the pulse audio code: - cubeb_pulse.c - AudioStream.cpp - Either MediaStreamGraph.cpp OR MediaDecoderStateMachine.cpp - Either AudioContext (if we came from MediaStreamGraph.cpp), from which you can get a docshell. - OR MediaDecoder and HTMLMediaElement if we are talking about <video> or <audio> (via MediaDecoderStateMachine.cpp), from which you can get a docshell. [1]: http://mxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_pulse.c
Thanks, Paul!
Hi. I've started looking into this. Basically, a proper name must be given to cubeb_stream_init instead of the hardcoded "AudioStream". padenot mentioned over irc using MediaInfo as a vessel for this information. My questions are: - This meta data needs to be added to the MediaInfo. But should it be: a) part of the audio data? b) independent of audio/video? (something like a title string) - Looks like MediaInfo is populated from within the MediaDecoderReaders, but the readers I've checked don't contain any relevant meta-data. What else could be used? The page title comes to mind, but that would not work so well with multiple streams within one page. - The "AudioStream" default value should still be set when title information is not available, right? - Should the MediaInfo trickle down to AudioStream? Or only the title needs to be passed.
Here's an initial patch. Some comments on it: - It doesn't use MediaInfo because that structure is filled with decoded data. Adding stuff to it didn't feel right. - The MediaDecoderStateMachine -> AudioSink -> AudioStream path is set up. But MediaStreamGraph->MediaStream->AudioStream isn't, just the name setter/getter are there. Initially I thought that this is the only way AudioStreams are created, which is not the case. The default way for a "normal" audio stream is via AudioSink. - I hope I got the string operations right.
Comment on attachment 8451368 [details] [diff] [review] set AudioStream name to URI spec Review of attachment 8451368 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioStream.cpp @@ +250,4 @@ > , mBytesPerFrame(0) > , mState(INITIALIZED) > , mNeedsStart(false) > + , mName() We probably don't need that. ::: content/media/MediaDecoderStateMachine.h @@ +349,5 @@ > > + // Get spec from URI > + void GetNameFromURI(nsACString &aName) > + { > + mDecoder->GetResource()->URI()->GetSpec(aName); We really need to think about thread safety here, because you're calling this from the thread that pushes the audio, but URI can only be called from the main thread, so you need to find another way to get the URI to the state machine. Also URI() can return nullptr, so we need to check that. This is probably going to be solved by sending an event containing the URI to the state machine thread. grep for `Dispatch` and `event` in the same directory (content/media) to learn how to do that. Also, a URI spec can be UTF-8, do you know how Pulse handle non-ascii characters? We have lossy conversion function available if needed. Similarly, do we have constraints on some platforms to set the name of a stream (character/encoding limit...) ? ::: content/media/MediaStreamGraph.h @@ +681,5 @@ > bool mMainThreadFinished; > bool mMainThreadDestroyed; > > + // Name of stream source. For now it comes from the URI. > + nsCString mSourceName; Here, you can't get an URI: the MediaStreamGraph is what we use to route, mix and generally handle MediaStreams: MediaRecorder, Web Audio API, WebRTC, getUserMedia. I think it's best to just call it "MediaStreamGraph", as it can be for example four <audio> elements mixed with a microphone input mixed with an inbound WebRTC stream.
> > nsCString mSourceName; > I think it's best to just call it "MediaStreamGraph", as it can be ... Part of the beauty of Pulse is that the app can give metadata and describe the audio stream, which then other tools can display to allow the user to know *what* is playing. This is highly useful when managing the audio. The information where the audio comes from is very important, as Firefox might have many tabs open, and some are playing in the background. I had it that I didn't know which tab started to blow my ears away, and was closing tab after tab. That should never happen. Right now, it just says "AudioStream: AudioStream". In fact, that's the purpose of this bug, to add this data. A static string is useless. I would propose the "source" to say e.g. "Firefox: " + window.title + " <" + pageURL + ">". At the least use window.title. Now, that's what we should put into Pulse stream source label. But I don't mean to say that the MediaStreamGraph member variable must contain that.
(In reply to Ben Bucksch (:BenB) from comment #8) > > > nsCString mSourceName; > > > I think it's best to just call it "MediaStreamGraph", as it can be ... > > Part of the beauty of Pulse is that the app can give metadata and describe > the audio stream, which then other tools can display to allow the user to > know *what* is playing. This is highly useful when managing the audio. > > The information where the audio comes from is very important, as Firefox > might have many tabs open, and some are playing in the background. I had it > that I didn't know which tab started to blow my ears away, and was closing > tab after tab. That should never happen. > > Right now, it just says "AudioStream: AudioStream". In fact, that's the > purpose of this bug, to add this data. A static string is useless. > > I would propose the "source" to say e.g. "Firefox: " + window.title + " <" + > pageURL + ">". At the least use window.title. This patch currently shows: <Firefox Icon> AudioStream: pageURL. But this is only for the "plain" case (sound playing from a page, via AudioSink).
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
Component: Audio/Video: MediaStreamGraph → Audio/Video: cubeb
Changes in the audiopipeline might have effected this and mght have made it moot
Priority: -- → P3
So I took a quick glance at the codebase to see if the patch could still work or not. It seems that they've migrated to cubeb for the audio backend, and files got shuffled around. Source browser: https://dxr.mozilla.org/mozilla-central/source/ https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/audio/ https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.h https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.cpp Firefox uses the cubeb library https://github.com/kinetiknz/cubeb I've also skimmed chrome source too and my findings are here: https://github.com/Zren/plasma-applets/issues/73
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
Keywords: good-first-bug
Whiteboard: [good first bug]

Is this still a relevant bug that needs any work done on it? Currently it looks like Firefox is providing the tab name to Pulseaudio, at least it is on my system, so it certainly seems like we're supporting PulseAudio metadata to me. If it's a stale bug, maybe it should be closed.

...I'll add that if this isn't considered closed and there's extra data we could be providing PulseAudio, I'm extremely interested in working on this bug if I can get some pointers from a more experienced dev on where I should be looking.

Looks sorted now, thanks peeps.

Essentially the same problem still exists with PipeWire, but that should be a new issue I'd guess.

(In reply to Milkii from comment #15)

Looks sorted now, thanks peeps.

Essentially the same problem still exists with PipeWire, but that should be a new issue I'd guess.

I'm running pipewire-pulse right now and getting the tab information, if you're getting issues with it and file a bug though i'd be down to try and help reproduce it :)

It relates to how port names show up in Carla, possibly via the JACK API side of PipeWire. See https://i.imgur.com/XDba5U7.png More likely PW bug I guess, will ask wtay.

Thanks for looking at this bug again!

Indeed, the situation is far better than when the bug was filed: I now see the page title.

The original description states:

  • The stream metadata should contain the domain name of the site, and the page title.
  • If the <video>/<audio> element provides meta data - e.g. currently playing song for web radios, see bug 778050 and bug 908902 and media.mozGetMetadata(), this should be shown as well, but in a different field, and sanitized.

Compared to current status:

  • Page title - the first and most important part - is implemented.
  • The domain name is missing. This would be important in cases where the website doesn't properly or correctly identify itself in the page title.
  • If I play an mp3 file (audio/mp3, without HTML page), it shows the filename = page title.
  • <video>/<audio> metadata (second point above) seems unused. This is particularly interesting for Internet radio stations and other websites which do not change the page title using JS when the song changes.

In all cases, care should be taken to remove all potentially dangerous special characters like \0 before sending this untrusted information to the system. I am not sure whether PulseAudio and all the Pulse applications consider the data to be untrusted in the same way as we consider sites to be potentially hostile. This is particularly important as websites can trigger playing audio at will, and could run exploits this way, if any of the applications in the down-stream chain were not expecting untrusted security-relevant content in the stream title.

Is it possible to get media.role sent to pulseaudio? To know if it is either stream/music or video?

It relates to how port names show up in Carla, possibly via the JACK API side of PipeWire. See https://i.imgur.com/XDba5U7.png More likely PW bug I guess, will ask wtay.

Sorry for ~OT, but @Milkii did you end up opening a bug on PipeWire? I would like to see that issue fixed as well!

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: