AESinkALSA: only use S24NE4MSB with S32 alsa format #13701

Kwiboo · 2018-03-26T21:15:32Z

Description

This PR fixes incorrect signaling of AE_FMT_S24NE4MSB format together with SND_PCM_FORMAT_S24 alsa format. (fixes white noise on Rockchip and HDMI alsa device)

Motivation and Context

Kodi incorrectly signals the AE_FMT_S24NE4MSB format using an alsa device limited to 24 significant bits when SND_PCM_FORMAT_S24 is probed before SND_PCM_FORMAT_S32.

Fix this by only signaling AE_FMT_S24NE4MSB when probing SND_PCM_FORMAT_S32.

This issue can be observed on Rockchip platform when using the HDMI alsa device (hdmi-codec and dw-hdmi-i2s), current workaround have been to remove the signaling of 24 significant bits in kernel (Kwiboo/linux-rockchip@1f19793)

How Has This Been Tested?

Before: AE_FMT_S24NE4MSB is wrongly used for S24

INFO: CAESinkALSA::Initialize - Opened device "hdmi:CARD=HDMI,DEV=0,AES0=0x04,AES1=0x82,AES2=0x00,AES3=0x00"
INFO: CAESinkALSA::InitializeHW - Your hardware does not support AE_FMT_FLOAT, trying other formats
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_FLOAT, fmt=FLOAT_LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE3, fmt=S24_3LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE4, fmt=S24_LE
INFO: CAESinkALSA::InitializeHW - AE_FMT_S24NE4 is valid, fmt=S24_LE fmtBits=32 bits=24
INFO: CAESinkALSA::InitializeHW - Using data format AE_FMT_S24NE4MSB

After: AE_FMT_S24NE4 is used for S24

INFO: CAESinkALSA::InitializeHW - Your hardware does not support AE_FMT_FLOAT, trying other formats
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_FLOAT, fmt=FLOAT_LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE3, fmt=S24_3LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE4, fmt=S24_LE
INFO: CAESinkALSA::InitializeHW - AE_FMT_S24NE4 is valid, fmt=S24_LE fmtBits=32 bits=24
INFO: CAESinkALSA::InitializeHW - Using data format AE_FMT_S24NE4

After: AE_FMT_S24NE4MSB is used for S32 and 24 significant bits (S24 support is removed in kernel driver)

INFO: CAESinkALSA::InitializeHW - Your hardware does not support AE_FMT_FLOAT, trying other formats
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_FLOAT, fmt=FLOAT_LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE3, fmt=S24_3LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE4, fmt=S24_LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S32NE, fmt=S32_LE
INFO: CAESinkALSA::InitializeHW - AE_FMT_S32NE is valid, fmt=S32_LE fmtBits=32 bits=24
INFO: CAESinkALSA::InitializeHW - Using data format AE_FMT_S24NE4MSB

After: AE_FMT_S32NE is used for S32 with 32 significant bits (S24 support is removed in kernel driver)

INFO: CAESinkALSA::InitializeHW - Your hardware does not support AE_FMT_FLOAT, trying other formats
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_FLOAT, fmt=FLOAT_LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE3, fmt=S24_3LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE4, fmt=S24_LE
INFO: CAESinkALSA::InitializeHW - testing AE_FMT_S32NE, fmt=S32_LE
INFO: CAESinkALSA::InitializeHW - AE_FMT_S32NE is valid, fmt=S32_LE fmtBits=32 bits=32
INFO: CAESinkALSA::InitializeHW - Using data format AE_FMT_S32NE

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the Code guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the CONTRIBUTING document
I have added tests to cover my change
All new and existing tests passed

xbmc/cores/AudioEngine/Sinks/AESinkALSA.cpp

@@ -746,9 +746,13 @@ bool CAESinkALSA::InitializeHW(const ALSAConfig &inconfig, ALSAConfig &outconfig
      int bits    = snd_pcm_hw_params_get_sbits(hw_params);
      if (bits != fmtBits)
      {
-        /* if we opened in 32bit and only have 24bits, signal it accordingly */
+        /*


xbmc/cores/AudioEngine/Sinks/AESinkALSA.cpp

        if (fmtBits == 32 && bits == 24)
-          i = AE_FMT_S24NE4MSB;
+          i = (fmt == SND_PCM_FORMAT_S32) ? AE_FMT_S24NE4MSB : AE_FMT_S24NE4;


Kwiboo · 2018-03-27T06:40:01Z

@FernetMenta updated to use if block like you suggested, also removed the comment change since the code now is much easier to follow

Kwiboo · 2018-03-27T06:49:01Z

There could still be an issue with S16 formats, noticed I got the following when I removed both S24 and S32 in kernel driver. S16 reported 0 significant bits.

 INFO: CAESinkALSA::Initialize - Opened device "hdmi:CARD=HDMI,DEV=0,AES0=0x04,AES1=0x82,AES2=0x00,AES3=0x00"
 INFO: CAESinkALSA::InitializeHW - Your hardware does not support AE_FMT_FLOAT, trying other formats
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_FLOAT, fmt=FLOAT_LE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE3, fmt=S24_3LE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE4, fmt=S24_LE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S32NE, fmt=S32_LE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S16NE, fmt=S16_LE
DEBUG: CAESinkALSA::InitializeHW - AE_FMT_S16NE is valid, fmt=S16_LE fmtBits=16 bits=0
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S16LE, fmt=S16_LE
DEBUG: CAESinkALSA::InitializeHW - AE_FMT_S16LE is valid, fmt=S16_LE fmtBits=16 bits=0
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S16BE, fmt=S16_BE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_U8, fmt=U8
ERROR: CAESinkALSA::InitializeHW - Unable to find a suitable output format

It then continues and falls back on "default" alsa device.

xbmc/cores/AudioEngine/Sinks/AESinkALSA.cpp

@@ -747,8 +747,10 @@ bool CAESinkALSA::InitializeHW(const ALSAConfig &inconfig, ALSAConfig &outconfig
      if (bits != fmtBits)
      {
        /* if we opened in 32bit and only have 24bits, signal it accordingly */
-        if (fmtBits == 32 && bits == 24)
+        if (fmt == SND_PCM_FORMAT_S32 && fmtBits == 32 && bits == 24)


Kwiboo · 2018-03-27T20:17:32Z

Updated to remove fmtBits == 32 for both S32 and S24 as they always are 32 bits

Kwiboo · 2018-03-27T22:15:00Z

Added two new commits that fixes two related issues:

restore hw_params when skipping 20 significant bits formats (very few drivers in kernel use this), trying to set S16 format after S24/S32 failed before this change
S16 formats report 0 significant bits for some reason on my device, skip bits check when 0 sbits is reported

S16 will be selected instead of falling back to "default" alsa device

 INFO: CAESinkALSA::Initialize - Opened device "hdmi:CARD=HDMI,DEV=0,AES0=0x04,AES1=0x82,AES2=0x00,AES3=0x00"
 INFO: CAESinkALSA::InitializeHW - Your hardware does not support AE_FMT_FLOAT, trying other formats
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_FLOAT, fmt=FLOAT_LE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE3, fmt=S24_3LE
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S24NE4, fmt=S24_LE
DEBUG: CAESinkALSA::InitializeHW - AE_FMT_S24NE4 is valid, fmt=S24_LE fmtBits=32 bits=20
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S32NE, fmt=S32_LE
DEBUG: CAESinkALSA::InitializeHW - AE_FMT_S32NE is valid, fmt=S32_LE fmtBits=32 bits=20
DEBUG: CAESinkALSA::InitializeHW - testing AE_FMT_S16NE, fmt=S16_LE
DEBUG: CAESinkALSA::InitializeHW - AE_FMT_S16NE is valid, fmt=S16_LE fmtBits=16 bits=0
 INFO: CAESinkALSA::InitializeHW - Using data format AE_FMT_S16NE

xbmc/cores/AudioEngine/Sinks/AESinkALSA.cpp

@@ -752,7 +756,10 @@ bool CAESinkALSA::InitializeHW(const ALSAConfig &inconfig, ALSAConfig &outconfig
        else if (fmt == SND_PCM_FORMAT_S24 && bits == 24)
          i = AE_FMT_S24NE4;
        else
+        {
+          snd_pcm_hw_params_copy(hw_params, hw_params_copy); // restore hw_params


xbmc/cores/AudioEngine/Sinks/AESinkALSA.cpp

@@ -748,7 +748,7 @@ bool CAESinkALSA::InitializeHW(const ALSAConfig &inconfig, ALSAConfig &outconfig

      int fmtBits = CAEUtil::DataFormatToBits(i);
      int bits    = snd_pcm_hw_params_get_sbits(hw_params);
-      if (bits != fmtBits)
+      if (bits && bits != fmtBits)


… bits

Kwiboo · 2018-03-28T17:11:49Z

updated to simplify code flow, now always restore hw_params before testing next alsa format and a short comment was added to indicate that bits check is skipped when bogus sbits is returned

(not yet fully runtime tested)

FernetMenta · 2018-03-28T18:07:50Z

jenkins build this please

Kwiboo · 2018-03-28T19:11:43Z

Thanks

FernetMenta · 2018-03-28T19:28:05Z

likewise

FernetMenta reviewed Mar 27, 2018

View reviewed changes

xbmc/cores/AudioEngine/Sinks/AESinkALSA.cpp Outdated

if (fmtBits == 32 && bits == 24)

i = AE_FMT_S24NE4MSB;

i = (fmt == SND_PCM_FORMAT_S32) ? AE_FMT_S24NE4MSB : AE_FMT_S24NE4;

This comment was marked as spam.

Sign in to view

Kwiboo force-pushed the alsa-fix-msb branch from f47713d to 8733283 Compare March 27, 2018 06:37

FernetMenta reviewed Mar 27, 2018

View reviewed changes

Kwiboo force-pushed the alsa-fix-msb branch from 8733283 to 94af81d Compare March 27, 2018 20:13

FernetMenta reviewed Mar 28, 2018

View reviewed changes

Kwiboo added 2 commits March 28, 2018 18:42

AESinkALSA: only use S24NE4MSB with S32 alsa format

926a99a

AESinkALSA: restore hw_params before testing next alsa format

5feac9a

Kwiboo force-pushed the alsa-fix-msb branch from 4f0d82a to a1e3998 Compare March 28, 2018 16:57

AESinkALSA: skip bits check when alsa-lib reports invalid significant…

3c3ae17

… bits

Kwiboo force-pushed the alsa-fix-msb branch from a1e3998 to 3c3ae17 Compare March 28, 2018 17:03

FernetMenta merged commit ad9770f into xbmc:master Mar 28, 2018

Kwiboo deleted the alsa-fix-msb branch March 28, 2018 19:11

Rechi added Type: Fix non-breaking change which fixes an issue Platform: Linux Component: Audio v18 Leia labels Mar 28, 2018

Rechi added this to the Leia 18.0-alpha2 milestone Mar 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AESinkALSA: only use S24NE4MSB with S32 alsa format #13701

AESinkALSA: only use S24NE4MSB with S32 alsa format #13701

Kwiboo commented Mar 26, 2018 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

Kwiboo commented Mar 27, 2018

Kwiboo commented Mar 27, 2018

This comment was marked as spam.

Kwiboo commented Mar 27, 2018

Kwiboo commented Mar 27, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Kwiboo commented Mar 28, 2018

FernetMenta commented Mar 28, 2018

Kwiboo commented Mar 28, 2018

FernetMenta commented Mar 28, 2018

AESinkALSA: only use S24NE4MSB with S32 alsa format #13701

AESinkALSA: only use S24NE4MSB with S32 alsa format #13701

Conversation

Kwiboo commented Mar 26, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

This comment was marked as spam.

This comment was marked as spam.

Kwiboo commented Mar 27, 2018

Kwiboo commented Mar 27, 2018

This comment was marked as spam.

Kwiboo commented Mar 27, 2018

Kwiboo commented Mar 27, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Kwiboo commented Mar 28, 2018

FernetMenta commented Mar 28, 2018

Kwiboo commented Mar 28, 2018

FernetMenta commented Mar 28, 2018

Kwiboo commented Mar 26, 2018 •

edited

Loading