CR-Client: Support multi-channel playback on Linux using ALSA

Daniel Yek wrote:
> 
> Modified by: dyekreal.com
> Date: 10/24/2006
> Project: Helix Player
> 
> Synopsis: Currently, we hear 2 channels only when
playing 5.1 channels 
> content
>   with Helix and RealPlayer.
>   This change enables multi-channels playback on ALSA.
> 
> Overview:
> (1)
>   Many member functions implementation in Helix Client
Core's ALSA audio
>   device code, audlinux_alsa.cpp, share the same member
variable,
>   m_wLastError.
> 
>   When an error occurred in a member function, the
ending _CloseAudio()
>   call can wipe out the existing error condition.
> 
>   The code looked like this:
> 
>   m_wLastError = RA_AOE_BADFORMAT;
>   _CloseAudio();  // This function always set:
"m_wLastError = 
> RA_AOE_NOERR;"
>   return m_wLastError;
> 
>   So, we always returned RA_AOE_NOERR in
_CheckFormat(), even when the 
> system
>   can't play the format.
> 
> (2)
>   The implementation was catered for 2-channel playback
only.
>   Playing back 5.1 channels content caused ALSA's
"default" PCM device to
>   invoke its ("plug" and) "route"
plugin and shrink the channels to 2 
> channels,
>   discarding surround sound content.
> 
>   The right ALSA PCM device to use to play 5.1 content
is the 
> "plug:surround51" PCM,
>   which only takes 6-channel input.
> 
>   If the system isn't set up to play 5.1 content,
snd_pcm_open() this PCM
>   would fail and Helix client would then fallback to
2-channel and use the
>   "default" PCM.
> 
>   Note that on some mis-configured system,
"plug:surround51" was defined 
> even though
>   the system isn't 5.1 capable. This is system
configuration problem.
>   On such system, ALSA would invoke the
"plug" plugin, which uses 
> "route" plugin to
>   discard surround sound content and playback in
2-channel mode.
>   On these systems, we really want to have Helix client
"preserve" all 
> channels by
>   down-mixing it to 2 channels.

So, do we properly handle mis-configured systems? If not, we
should. At
least we should have a pref that will let users force
2-channel playback,
if we don't already.

> 
> (3)
>   ALSA now always configures the sound card to 48000Hz.
>   So, Helix's ALSA audio device _CheckFormat() code now
forces Helix client
>   to use 48000Hz.

Is this the case for *all* ALSA systems out in the wild? If
not, we need
to handle that.


I am only reviewing the core parts below. In the future you
should
break up the CR so that at least the core parts are
seperate. If
someone sees me respond to this CR they may assume that I
looked at all of it and not read it. They you also don't
have to
cross post to many lists (helix-client-dev is never used for
CRs,
audio-dev would have been fine for just the core parts).

-    virtual HX_RESULT _OpenAudio() = 0;
+    virtual HX_RESULT _OpenAudio( const HXAudioFormat*
pFormat ) = 0;

I am not sure I like changing this in audUnix.h. If you do,
you need
to change it everywhere, not just ALSA and OSS:

D:cygwinhomegwrighthelixaudiodevicepubplatformopenw
aveaudopwave.h(205):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audhpux.h(100):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audirix.h(100):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audlinux_alsa.h(99):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audlinux_esound.h(118):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audlinux_oss.h(110):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audSolaris.h(102):    virtual HX_RESULT _OpenAudio();
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audUnix.h(221):    virtual HX_RESULT _OpenAudio() = 0;
D:cygwinhomegwrighthelixaudiodevicepubplatformunix
audusound.h(100):    virtual HX_RESULT _OpenAudio();

I think it would be better, since they all inherit from
audUnix, that you
just store the format in a member var (maybe a new one in
audUnix) and
let the child classes use it. So, in _OpenAudio they already
have the
format in, for example, m_pAudioFormat.



+
+    if (m_bCheckFormat)
+    {
+        // Print ALSA PCM device name while doing checking
format.
+        HXLOGL4 (HXLOG_ADEV, "Opening ALSA PCM device,
%s, to check format!", szDevice);
+    }
+    else
+    {
+        // Print ALSA PCM device name outside of
_CheckFormat() function (that is, it is actually being
used).
+        HXLOGL4(HXLOG_ADEV, "Opening ALSA PCM device,
%s, for actual use!", szDevice);
+        printf("Opening ALSA PCM device %sn",
szDevice);
+    }

I think those should be L2 log statements. It would be
helpful to have
those when trying to debug a player out in the field (you
only get L1
and L2 in release builds).

-
+        // Output to stdout too because the code is still
quite new.
+        printf("Device Configured:n");
+        printf("         Sample Rate: %un", 
(unsigned int) m_unSampleRate);
+        printf("        Sample Width: %un", 
(unsigned int) m_uSampFrameSize);
+        printf("        Num channels: %un", 
(unsigned int) m_unNumChannels);
+        printf("          Block size: %dn", 
m_wBlockSize);
+        printf("  Device buffer size: %lun",
(unsigned long int) m_ulDeviceBufferSize);
+        printf("   Supports HW Pause: %dn", 
m_bHasHardwarePauseAndResume);
+        printf("     Start threshold: %dn", 
(int) start_threshold);
+        printf("n");
+        fflush(stdout);

only do the above in _DEBUG builds....


+    if(state == SND_PCM_STATE_XRUN)
+    {
+        HXLOGL1 ( HXLOG_ADEV, "XRUN in
GetBytesActuallyPlayedUsingDelay()");
+    }
...
      if (err < 0)
      {
-        HXLOGL1 ( HXLOG_ADEV, "snd_pcm_status:
%s", snd_strerror(err));
+        HXLOGL1 ( HXLOG_ADEV, "snd_pcm_delay:
%s", snd_strerror(err));
+    }
+    else if (frame_delay < 0)
+    {
+        HXLOGL1 ( HXLOG_ADEV, "XRUN! frame_delay:
%d", frame_delay);
+        nBytesPlayed = m_ulTotalWritten;
+        retVal = HXR_OK; // Is this really OK?
      }


Since those are L1 logs, make sure they only fire during
important
error conditions. L1s should fire very rarely or just at
startup.

-//    HXLOGL4 ( HXLOG_ADEV, "nBytesPlayed: %llu,
m_ulTotalWritten: %llun", nBytesPlayed,
m_ulTotalWritten);
+    HXLOGL2 ( HXLOG_ADEV,
"GetBytesActuallyPlayedUsingDelay()  nBytesPlayed:
%llu,   m_ulTotalWritten: %llu n", nBytesPlayed,
m_ulTotalWritten);


This looks like it should *NOT* be a L2, L1 and L2 are in
all release builds.
They should not fire all the time. Also, don't leave code in
that is
commented out, just remove it unless it is really needed to
understand
the code.

+                // Add AlsaVaryingSampleRate into
Preference.
+                IHXBuffer* pBuffer = new CHXBuffer;
+                pBuffer->AddRef();
+                pBuffer->SetSize(2);
+                SafeSprintf((char*)
pBuffer->GetBuffer(),2,"%d", 0); /* Flawfinder:
ignore */
+               
z_pIHXPrefs->WritePref("AlsaVaryingSampleRate",
pBuffer);
+                pBuffer->Release();
+            }

this seems like overkill. I think you can just do:

   WritePrefUINT32( m_pContext,
"AlsaVaryingSampleRate", 0 );




Not really part of this CR, but what happens if a user
selects ALSA as
the output system and they do not have it installed?

--greg.

_______________________________________________
Player-dev mailing list
Player-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/play
er-dev