[AE/PulseAudio] fix for volume, device selection #1868

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
8 participants

s7mx1 commented Nov 30, 2012

first commit is to fix pulsaudio volume step under xbmc. This is caused by volume range difference between AE and PulseAudio and as a result the minimum volume (apart from silent) you will get with XBMC is 10% or -59.33 dB.

second commit is to fix sample file playback constant 100% volume, aka skin/menu sound. The fix brings menu sound volume to the same level as everything else.

third commit is to fix xbmc gui audio devices (including passthrough) are being ignored. If user set it then system should use it otherwise what the point having it in the first place?

s7mx1 commented Nov 30, 2012

To clarify: AE's volume range is [-60,0]dB and PulseAudio's volume range is (-inf,0]dB.

Member

jmarshallnz commented Nov 30, 2012

Doesn't pulse use a 60dB range as well. At least according to the sources it does:

http://pulseaudio.sourcearchive.com/documentation/0.9.14-1/volume_8c-source.html

Or is USER_DECIBEL_RANGE defined as something else?

Either way, a separate function for doing the computation is required, along with an explanation that pulse takes a range from 0 (mute) to PA_VOLUME_NORM which is linear on the dB scale.

s7mx1 commented Nov 30, 2012

The latest file is here

http://freedesktop.org/software/pulseaudio/doxygen/volume_8h_source.html

Looks like its been removed in the latest header file. I am running pulseaudio 2.1 and if I set the sink volume to 0% I do get the db value to be -inf.

s7mx1 commented Nov 30, 2012

The (hardware) volume range from 0 to 1 is fine. The problem lies in

CApplication::SetHardwareVolume

where hardware volume is converted to db using CAEUtil::PercentToGain then converted to liner value using CAEUtil::GainToScale(dB). 60dB are hardcoded in these two functions.

Member

jmarshallnz commented Nov 30, 2012

Yes - the UI volume in XBMC is a percentage (i.e. linear on the -60...0 dB scale). The volume in AE is a scaler multiplier on the linear scale.

Thus, you'd want to convert the AE volume back to a percentage and then set that directly using pa_sw_volume_from_linear which is basically what you're doing, though you're doing it directly rather than relying on pulseaudio to do it for you.

As long as it's documented well and in a separate function (so you can change it in one spot) it'll be fine.

s7mx1 commented Dec 1, 2012

Will do tomorrow. Its midnight here.

@s7mx1 s7mx1 [AE/Pulseaudio] add CAEUtil::PercentToPulseVolume to convert hardware…
… volume [0,1] to pulseaudio volume [0,PA_VOLUME_NORM]. This fixes volume range mismatch between AE [-60,0]dB and pulseaudio (-inf,0]dB. This should work with any version of Puleaudio as long as PA_VOLUME_NORM is correctly defined.
e9346f2

s7mx1 commented Dec 1, 2012

Had to revert the 2 patches and did a rebase to clean it up. Will add back the other 2 patches shortly. The above patch added CAEUtil::PercentToPulseVolume to do the conversion which should work for any pulseaudio version that has PA_VOLUME_NORM defined.

s7mx1 commented Dec 20, 2012

@bobo1on1 @topfs2

Please review the code.

@arnova arnova and 2 others commented on an outdated diff Dec 20, 2012

xbmc/cores/AudioEngine/Engines/PulseAE/PulseAE.cpp
@@ -409,4 +410,19 @@ void CPulseAE::SetMute(const bool enabled)
m_muted = enabled;
}
+/*
+ Return audio device name set within XBMC GUI. If passthrough is set to true
+ audio passthrough device name will be returned.
+*/
+const char* CPulseAE::GetAudioDevice(bool passthrough)
+{
+ std::string m_outputDevice;
+ switch(passthrough)
+ {
+ case 1 : m_outputDevice = g_guiSettings.GetString("audiooutput.passthroughdevice"); break;
+ default: m_outputDevice = g_guiSettings.GetString("audiooutput.audiodevice");
+ }
+ return m_outputDevice.c_str();
@arnova

arnova Dec 20, 2012

Member

You might as well use "if (.. == 1)... else" here instead of the case

@Jalle19

Jalle19 Dec 26, 2012

Member

I don't know what style you guys normally use, but wouldn't

std::string deviceName = passthrough ? "audiooutput.passthroughdevice" : "audiooutput.audiodevice";
m_outputDevice = g_guiSettings.GetString(deviceName);

be way more readable?

@bobo1on1

bobo1on1 Dec 26, 2012

Member

We prefer job security code style.

CurlyMoo referenced this pull request in xbianonpi/xbian Dec 22, 2012

Closed

Both HDMI and Analog output on Raspberry Pi XBMC #172

s7mx1 commented Dec 27, 2012

@arnova

I have updated CPulseAE::GetAudioDevice to use if-else instead of switch.

@bobo1on1

Are you ok with these patches?

@CurlyMoo

This is to fix AE/PA bugs and has nothing to do with omxplayer at all. Ask @huceke nicely or code it yourself if you want AE on raspberry pi.

s7mx1 commented Dec 27, 2012

Did a rebase

@arnova arnova commented on an outdated diff Dec 27, 2012

xbmc/cores/AudioEngine/Engines/PulseAE/PulseAE.cpp
@@ -409,4 +410,18 @@ void CPulseAE::SetMute(const bool enabled)
m_muted = enabled;
}
+/*
+ Return audio device name set within XBMC GUI. If passthrough is set to true
+ audio passthrough device name will be returned.
+*/
+const char* CPulseAE::GetAudioDevice(bool passthrough)
+{
+ std::string m_outputDevice;
+ if (passthrough == 1)
@arnova

arnova Dec 27, 2012

Member

drop the == 1, it's a bool after all. I think some compilers may even warn about this.

s7mx1 commented Dec 27, 2012

@arnova
Fixed.

s7mx1 commented Feb 11, 2013

@fritsch

Could you have a look at this pull request?

You use this as a std::string the whole time, what do you really need here, const char* or std::string?

Yeah there is no real Sink right now, so the sound has to know the engine. This should not be like this, if there would be a Sink that knows what it should play and is properly initialized...

Stream and Sound should just play, no matter they know which device is currently used. Now you make a lot of work at points where it should not be. We have the PulseAE already, that could manage this for us.

It is okay to include the stuff from above as you have to know some things about Audio Setup and so on - but device selection should be done either more up in Engine or way more down in the Sink directly. We can talk on irc if you want.

Owner

s7mx1 commented on 521da3d Feb 11, 2013

What's your irc details?

just fritsch on freenode.

Member

fritsch commented Feb 11, 2013

As talked via irc:

  • What happens when user forces the passthrough device and menu sounds are played?
  • What happens if user pauses movie and menu sounds have to be mixed in?

I think we need more logic somewhere, as best in a separate PA Sink, that just handles all that stuff for us.

Owner

MartijnKaijser commented Jul 24, 2013

small nudge on status

Owner

MartijnKaijser commented Oct 17, 2013

@FernetMenta / @fritsch
PR still needed or will you pick relevant parts from it?

FernetMenta was assigned Oct 17, 2013

Member

fritsch commented Oct 17, 2013

I already asked some questions above and came to a conclusion.

fritsch closed this Oct 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment