Support PulseAudio metadata

NEW
Unassigned

Status

()

P4
enhancement
5 years ago
a year ago

People

(Reporter: BenB, Unassigned)

Tracking

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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>
(Reporter)

Comment 1

5 years ago
Created attachment 8367777 [details]
SongBird via PulseAudio showing current song and artist in PulseAudio volume manager
Assignee: kinetik → nobody
Whiteboard: [good first bug]
(Reporter)

Comment 2

5 years ago
> [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
(Reporter)

Comment 4

5 years ago
Thanks, Paul!

Comment 5

4 years ago
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.

Comment 6

4 years ago
Created attachment 8451368 [details] [diff] [review]
set AudioStream name to URI spec

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.
(Reporter)

Comment 8

4 years ago
> > 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.

Comment 9

4 years ago
(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

Comment 11

2 years ago
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
You need to log in before you can comment on or make changes to this bug.