Skip to content
This repository

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

Closed
wants to merge 3 commits into from

8 participants

s7mx1 jmarshallnz Peter Frühberger Martijn Kaijser arnova Sam Stenvall bobo1on1 Rainer Hochecker
s7mx1

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

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

jmarshallnz
Owner

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

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

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.

jmarshallnz
Owner

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

Will do tomorrow. Its midnight here.

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

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

@bobo1on1 @topfs2

Please review the code.

xbmc/cores/AudioEngine/Engines/PulseAE/PulseAE.cpp
... ...
@@ -409,4 +410,19 @@ void CPulseAE::SetMute(const bool enabled)
409 410
   m_muted = enabled;
410 411
 }
411 412
 
  413
+/*
  414
+  Return audio device name set within XBMC GUI. If passthrough is set to true
  415
+  audio passthrough device name will be returned.
  416
+*/
  417
+const char* CPulseAE::GetAudioDevice(bool passthrough)
  418
+{
  419
+  std::string m_outputDevice;
  420
+  switch(passthrough)
  421
+  {
  422
+    case 1 : m_outputDevice = g_guiSettings.GetString("audiooutput.passthroughdevice"); break;
  423
+    default: m_outputDevice = g_guiSettings.GetString("audiooutput.audiodevice");
  424
+  }
  425
+  return m_outputDevice.c_str();
3
arnova Collaborator
arnova added a note

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

Sam Stenvall
Jalle19 added a note

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 Collaborator

We prefer job security code style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
CurlyMoo CurlyMoo referenced this pull request in xbianonpi/xbian
Closed

Both HDMI and Analog output on Raspberry Pi XBMC #172

s7mx1

@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

Did a rebase

xbmc/cores/AudioEngine/Engines/PulseAE/PulseAE.cpp
... ...
@@ -409,4 +410,18 @@ void CPulseAE::SetMute(const bool enabled)
409 410
   m_muted = enabled;
410 411
 }
411 412
 
  413
+/*
  414
+  Return audio device name set within XBMC GUI. If passthrough is set to true
  415
+  audio passthrough device name will be returned.
  416
+*/
  417
+const char* CPulseAE::GetAudioDevice(bool passthrough)
  418
+{
  419
+  std::string m_outputDevice;
  420
+  if (passthrough == 1)
1
arnova Collaborator
arnova added a note

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

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

@arnova
Fixed.

s7mx1

@fritsch

Could you have a look at this pull request?

Peter Frühberger

m_something are class fields.

Peter Frühberger

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

Peter Frühberger

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...

Peter Frühberger

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.

Collaborator

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.

s7mx1

What's your irc details?

Collaborator

just fritsch on freenode.

Peter Frühberger
Collaborator

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.

Martijn Kaijser
Owner

small nudge on status

Martijn Kaijser

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

Peter Frühberger
Collaborator

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

Peter Frühberger fritsch closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

Dec 01, 2012
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 [AE/Pulseaudio] set sample(skin menu) playback volume to the current …
…XBMC system volume level
44eda77
Dec 27, 2012
s7mx1 [AE/Pulseaudio] use xbmc gui audio device for skin/menu sound playbac…
…k and stream audio playback
521da3d
This page is out of date. Refresh to see the latest.
19  xbmc/cores/AudioEngine/Engines/PulseAE/PulseAE.cpp
@@ -30,6 +30,7 @@
30 30
 #include <pulse/pulseaudio.h>
31 31
 #include <pulse/simple.h>
32 32
 #include "guilib/LocalizeStrings.h"
  33
+#include "settings/GUISettings.h"
33 34
 
34 35
 /* Static helpers */
35 36
 static const char *ContextStateToString(pa_context_state s)
@@ -353,7 +354,7 @@ static void SinkInfo(pa_context *c, const pa_sink_info *i, int eol, void *userda
353 354
     {
354 355
       CStdString desc, sink;
355 356
       desc.Format("%s (PulseAudio)", i->description);
356  
-      sink.Format("pulse:%s@default", i->name);
  357
+      sink.Format("%s", i->name);
357 358
       sinkStruct->list->push_back(AEDevice(desc, sink));
358 359
       CLog::Log(LOGDEBUG, "PulseAudio: Found %s with devicestring %s", desc.c_str(), sink.c_str());
359 360
     }
@@ -375,7 +376,7 @@ void CPulseAE::EnumerateOutputDevices(AEDeviceList &devices, bool passthrough)
375 376
   sinkStruct.list = &devices;
376 377
   CStdString def;
377 378
   def.Format("%s (PulseAudio)",g_localizeStrings.Get(409).c_str());
378  
-  devices.push_back(AEDevice(def, "pulse:default@default"));
  379
+  devices.push_back(AEDevice(def, "default"));
379 380
   WaitForOperation(pa_context_get_sink_info_list(m_Context,
380 381
                    SinkInfo, &sinkStruct), m_MainLoop, "EnumerateAudioSinks");
381 382
 
@@ -409,4 +410,18 @@ void CPulseAE::SetMute(const bool enabled)
409 410
   m_muted = enabled;
410 411
 }
411 412
 
  413
+/*
  414
+  Return audio device name set within XBMC GUI. If passthrough is set to true
  415
+  audio passthrough device name will be returned.
  416
+*/
  417
+const char* CPulseAE::GetAudioDevice(bool passthrough)
  418
+{
  419
+  std::string m_outputDevice;
  420
+  if (passthrough)
  421
+    m_outputDevice = g_guiSettings.GetString("audiooutput.passthroughdevice");
  422
+  else
  423
+    m_outputDevice = g_guiSettings.GetString("audiooutput.audiodevice");
  424
+  return m_outputDevice.c_str();
  425
+}
  426
+
412 427
 #endif
1  xbmc/cores/AudioEngine/Engines/PulseAE/PulseAE.h
@@ -69,6 +69,7 @@ class CPulseAE : public IAE
69 69
   virtual void SetMute(const bool enabled);
70 70
   virtual bool IsMuted() { return m_muted; }
71 71
   virtual void SetSoundMode(const int mode) {}
  72
+  static const char* GetAudioDevice(bool passthrough);
72 73
 #if PA_CHECK_VERSION(1,0,0)
73 74
   virtual bool SupportsRaw() { return true; }
74 75
 #endif
12  xbmc/cores/AudioEngine/Engines/PulseAE/PulseAESound.cpp
@@ -22,7 +22,9 @@
22 22
 #ifdef HAS_PULSEAUDIO
23 23
 
24 24
 #include "PulseAESound.h"
  25
+#include "PulseAE.h"
25 26
 #include "AEFactory.h"
  27
+#include "Utils/AEUtil.h"
26 28
 #include "utils/log.h"
27 29
 #include "MathUtils.h"
28 30
 #include "StringUtils.h"
@@ -81,7 +83,7 @@ bool CPulseAESound::Initialize()
81 83
 
82 84
   m_maxVolume     = CAEFactory::GetEngine()->GetVolume();
83 85
   m_volume        = 1.0f;
84  
-  pa_volume_t paVolume = pa_sw_volume_from_linear((double)(m_volume * m_maxVolume));
  86
+  pa_volume_t paVolume = CAEUtil::PercentToPulseVolume((double)(m_volume * m_maxVolume));
85 87
   pa_cvolume_set(&m_chVolume, m_sampleSpec.channels, paVolume);
86 88
 
87 89
   pa_threaded_mainloop_lock(m_mainLoop);
@@ -135,7 +137,13 @@ void CPulseAESound::Play()
135 137
   /* we only keep the most recent operation as it is the only one needed for IsPlaying to function */
136 138
   if (m_op)
137 139
     pa_operation_unref(m_op);
138  
-  m_op = pa_context_play_sample(m_context, m_pulseName.c_str(), NULL, PA_VOLUME_INVALID, NULL, NULL);
  140
+  m_maxVolume     = CAEFactory::GetEngine()->GetVolume();
  141
+  pa_volume_t  paVolume = CAEUtil::PercentToPulseVolume((double)(m_volume * m_maxVolume));
  142
+  std::string m_outputDevice = CPulseAE::GetAudioDevice(false);
  143
+  if (m_outputDevice == "default")
  144
+    m_op = pa_context_play_sample(m_context, m_pulseName.c_str(), NULL, paVolume, NULL, NULL);
  145
+  else
  146
+    m_op = pa_context_play_sample(m_context, m_pulseName.c_str(), m_outputDevice.c_str(), paVolume, NULL, NULL);   
139 147
   pa_threaded_mainloop_unlock(m_mainLoop);
140 148
 }
141 149
 
20  xbmc/cores/AudioEngine/Engines/PulseAE/PulseAEStream.cpp
@@ -22,8 +22,10 @@
22 22
 #ifdef HAS_PULSEAUDIO
23 23
 
24 24
 #include "PulseAEStream.h"
  25
+#include "PulseAE.h"
25 26
 #include "AEFactory.h"
26 27
 #include "Utils/AEUtil.h"
  28
+#include "Utils/AEUtil.h"
27 29
 #include "utils/log.h"
28 30
 #include "utils/MathUtils.h"
29 31
 #include "threads/SingleLock.h"
@@ -63,6 +65,8 @@ CPulseAEStream::CPulseAEStream(pa_context *context, pa_threaded_mainloop *mainLo
63 65
   m_channelLayout = channelLayout;
64 66
   m_options = options;
65 67
 
  68
+  bool m_passthrough = false;
  69
+
66 70
   m_DrainOperation = NULL;
67 71
   m_slave = NULL;
68 72
 
@@ -138,7 +142,7 @@ CPulseAEStream::CPulseAEStream(pa_context *context, pa_threaded_mainloop *mainLo
138 142
 
139 143
   m_MaxVolume     = CAEFactory::GetEngine()->GetVolume();
140 144
   m_Volume        = 1.0f;
141  
-  pa_volume_t paVolume = pa_sw_volume_from_linear((double)(m_Volume * m_MaxVolume));
  145
+  pa_volume_t paVolume = CAEUtil::PercentToPulseVolume((double)(m_Volume * m_MaxVolume));
142 146
   pa_cvolume_set(&m_ChVolume, m_SampleSpec.channels, paVolume);
143 147
 
144 148
 #if PA_CHECK_VERSION(1,0,0)
@@ -155,6 +159,8 @@ CPulseAEStream::CPulseAEStream(pa_context *context, pa_threaded_mainloop *mainLo
155 159
   pa_format_info_set_channels     (info[0], m_SampleSpec.channels);
156 160
   pa_format_info_set_sample_format(info[0], m_SampleSpec.format);
157 161
   m_Stream = pa_stream_new_extended(m_Context, "audio stream", info, 1, NULL);
  162
+  if (!info[0]->encoding == PA_ENCODING_PCM)
  163
+    m_passthrough = true;
158 164
   pa_format_info_free(info[0]);
159 165
 #else
160 166
   m_Stream = pa_stream_new(m_Context, "audio stream", &m_SampleSpec, &map);
@@ -177,7 +183,14 @@ CPulseAEStream::CPulseAEStream(pa_context *context, pa_threaded_mainloop *mainLo
177 183
   if (options && AESTREAM_FORCE_RESAMPLE)
178 184
     flags |= PA_STREAM_VARIABLE_RATE;
179 185
 
180  
-  if (pa_stream_connect_playback(m_Stream, NULL, NULL, (pa_stream_flags)flags, &m_ChVolume, NULL) < 0)
  186
+  int pa_state;
  187
+  std::string m_outputDevice = CPulseAE::GetAudioDevice(m_passthrough);
  188
+  if (m_outputDevice == "default")
  189
+    pa_state = pa_stream_connect_playback(m_Stream, NULL, NULL, (pa_stream_flags)flags, &m_ChVolume, NULL);
  190
+  else
  191
+    pa_state = pa_stream_connect_playback(m_Stream, m_outputDevice.c_str(), NULL, (pa_stream_flags)flags, &m_ChVolume, NULL);
  192
+
  193
+  if (pa_state < 0)
181 194
   {
182 195
     CLog::Log(LOGERROR, "PulseAudio: Failed to connect stream to output");
183 196
     pa_threaded_mainloop_unlock(m_MainLoop);
@@ -208,6 +221,7 @@ CPulseAEStream::CPulseAEStream(pa_context *context, pa_threaded_mainloop *mainLo
208 221
   m_Initialized = true;
209 222
 
210 223
   CLog::Log(LOGINFO, "PulseAEStream::Initialized");
  224
+  CLog::Log(LOGINFO, "  Sink Output   : %s", m_outputDevice.c_str());
211 225
   CLog::Log(LOGINFO, "  Sample Rate   : %d", m_sampleRate);
212 226
   CLog::Log(LOGINFO, "  Sample Format : %s", CAEUtil::DataFormatToStr(m_format));
213 227
   CLog::Log(LOGINFO, "  Channel Count : %d", m_channelLayout.Count());
@@ -419,7 +433,7 @@ void CPulseAEStream::SetVolume(float volume)
419 433
   if (volume > 0.f)
420 434
   {
421 435
     m_Volume = volume;
422  
-    pa_volume_t paVolume = pa_sw_volume_from_linear((double)(m_Volume * m_MaxVolume));
  436
+    pa_volume_t paVolume = CAEUtil::PercentToPulseVolume((double)(m_Volume * m_MaxVolume));
423 437
 
424 438
     pa_cvolume_set(&m_ChVolume, m_SampleSpec.channels, paVolume);
425 439
   } 
24  xbmc/cores/AudioEngine/Utils/AEUtil.h
@@ -24,6 +24,10 @@
24 24
 #include "PlatformDefs.h"
25 25
 #include <math.h>
26 26
 
  27
+#if defined(HAS_PULSEAUDIO)
  28
+#include <pulse/pulseaudio.h>
  29
+#endif
  30
+
27 31
 #ifdef TARGET_WINDOWS
28 32
 #if _M_IX86_FP>0 && !defined(__SSE__)
29 33
 #define __SSE__
@@ -81,6 +85,26 @@ class CAEUtil
81 85
   {
82 86
     return pow(10.0f, dB/20);
83 87
   }
  88
+  
  89
+  #if defined(HAS_PULSEAUDIO)
  90
+  /*
  91
+     Hardware volume [0,1] will be converted to dB using PercentToGain() and then converted to scale by 
  92
+     GainToScale() before reaching PulseAudio. However the underlying math assumes 60dB volume range which
  93
+     contradicts (-inf,0] volume range used by pulseaudio 2.0 and above. To fix this we will reverse the
  94
+     process of GainToScale() and PercentToGain() to get hardware volume then using our own linear converter to
  95
+     get valid pulseaudio volume [0,PA_VOLUME_NORM].
  96
+  */
  97
+  static inline const int PercentToPulseVolume(const float value)
  98
+  {
  99
+    float hardware_volume = 0.f;
  100
+    // This is to reverse "pow(10.0f, dB/20)" in GainToScale() and "(value - 1)*db_range" in PercentToGain()
  101
+    if ( value > 0.f )
  102
+      hardware_volume = (double)( (20 * log10(value)) / 60 + 1 );
  103
+    // This is to simulate pa_sw_volume_from_linear() which convert a linear hardware volume rage [0,1] 
  104
+    // to pulseaudio volume rage [0, PA_VOLUME_NORM]
  105
+    return (int)( hardware_volume * PA_VOLUME_NORM );
  106
+  }
  107
+  #endif
84 108
 
85 109
   #ifdef __SSE__
86 110
   static void SSEMulArray     (float *data, const float mul, uint32_t count);
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.