Merge ~stanley31/checkbox-support:0909_modify_audio_settings into checkbox-support:master

[StanleyHuang]

A fixed for lp:1940391

I have validated the audio_settings script could parse the volume from mono/stereo audio interface.

Test this script on a system with input (mono) audio and output (stereo) audio.

detail audio information:
{'name': 'bluez_source.14_3F_A6_62_7C_02.headset_head_unit', 'muted': 'no', 'volumes': {'mono': 100}, 'max_volume': 100}
{'name': 'alsa_output.pci-0000_00_1f.3.analog-stereo', 'muted': 'no', 'volumes': {'front-left': 64, 'front-right': 40}, 'max_volume': 64}

I have ran the store_audio_settings function directly, please see following contents:
default_source: bluez_source.14_3F_A6_62_7C_02.headset_head_unit
source_muted: no
source_volume: 100%
default_sink: alsa_output.pci-0000_00_1f.3.analog-stereo
sink_muted: no
sink_volume: 64%

[Maciej Kisielewski (kissiel)]

I found one problem with the regex string, see below.

Nitpicking: There are at least three ways that string interpolation is used in this code (overall audio_settings.py, not just your patch):
+
.format
""%()

And I think it's pretty yucky. I'm not asking anyone to do a small revolution and fix all that, but I would prefer not to cultivate bad habits.

review: Needs Fixing

[StanleyHuang (stanley31)]

@Maciej,

May I have your suggestion for the way to format string?
option 1: use .format
option 2: use f"" string

If the script would only run under Python 3.6 up, I think the option 2 is a better solution due to it makes the string more readable.

Thanks.

[StanleyHuang (stanley31)]

I think the ".format" might be a option due to we need to replace string for some pattern variables.

[StanleyHuang (stanley31)]

Two commits for this MR.

first one is to change the way to parse audio volumes.
second one is to unified the format string method.

And it has been tested on my laptop with stereo and mono audio devices.
the stereo logs:
>>> print(get_audio_settings("source"))
{'name': 'alsa_input.pci-0000_00_1f.3.analog-stereo', 'muted': 'no', 'volumes': {'front-left': 26, 'front-right': 26}, 'max_volume': 26}
>>> print(get_audio_settings("sink"))
{'name': 'alsa_output.pci-0000_00_1f.3.hdmi-stereo-extra1', 'muted': 'no', 'volumes': {'front-left': 88, 'front-right': 88}, 'max_volume': 88}

the mono logs:
>>> print(get_audio_settings("source"))
{'name': 'bluez_source.74_5C_4B_D4_EB_94.headset_head_unit', 'muted': 'no', 'volumes': {'mono': 59}, 'max_volume': 59}
>>> print(get_audio_settings("sink"))
{'name': 'bluez_sink.74_5C_4B_D4_EB_94.headset_head_unit', 'muted': 'no', 'volumes': {'mono': 69}, 'max_volume': 69}

[Maciej Kisielewski (kissiel)]

Great stuff! Thank you for all the patches. As for which string interpolation option, I think .format() is great. One thing to note, though, is for logging calls the %s should be used, as this prevents formatting from happening unless the logging call is on the right level (it's evaluated lazily). You can read more here: https://stackoverflow.com/questions/34619790/pylint-message-logging-format-interpolation

I'm +1 on landing this.

review: Approve

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

"10.38.105.54"
"10.38.105.108"
"10.38.105.197"
[xenial] [14:06:01] starting container
Device project added to xenial-testing
"10.38.105.54"
[xenial] [14:07:16] provisioning container
[xenial] [14:09:26] Starting tests...
[xenial] Found a test script: ./requirements/container-tests-checkbox-support
[bionic] [14:09:52] starting container
Device project added to bionic-testing
[xenial] [14:10:03] container-tests-checkbox-support: FAIL
[xenial] output:
[xenial] [14:10:03] Fixing file permissions in source directory
[xenial] [14:10:04] Destroying container
[focal] [14:10:28] starting container
Device project added to focal-testing
"10.38.105.115"
[bionic] [14:10:41] provisioning container
"10.38.105.137"
[focal] [14:10:59] provisioning container
[bionic] [14:11:09] Starting tests...
[bionic] Found a test script: ./requirements/container-tests-checkbox-support
[focal] [14:11:29] Starting tests...
[focal] Found a test script: ./requirements/container-tests-checkbox-support
[bionic] [14:11:48] container-tests-checkbox-support: FAIL
[bionic] output:
[bionic] [14:11:48] Fixing file permissions in source directory
[bionic] [14:11:48] Destroying container
[focal] [14:12:03] container-tests-checkbox-support: FAIL
[focal] output:
[focal] [14:12:03] Fixing file permissions in source directory
[focal] [14:12:03] Destroying container

review: Needs Fixing

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

"10.38.105.54"
"10.38.105.108"
"10.38.105.197"
[xenial] [14:40:43] starting container
[bionic] [14:40:45] starting container
[focal] [14:40:52] starting container
Device project added to xenial-testing
Device project added to bionic-testing
"10.38.105.54"
[xenial] [14:41:02] provisioning container
Device project added to focal-testing
"10.38.105.141"
[bionic] [14:41:06] provisioning container
"10.38.105.142"
[focal] [14:41:13] provisioning container
[bionic] [14:41:44] Starting tests...
[bionic] Found a test script: ./requirements/container-tests-checkbox-support
[focal] [14:41:51] Starting tests...
[focal] Found a test script: ./requirements/container-tests-checkbox-support
[bionic] [14:42:17] container-tests-checkbox-support: FAIL
[bionic] output:
[bionic] [14:42:17] Fixing file permissions in source directory
[bionic] [14:42:18] Destroying container
[xenial] [14:42:18] Starting tests...
[xenial] Found a test script: ./requirements/container-tests-checkbox-support
[focal] [14:42:21] container-tests-checkbox-support: FAIL
[focal] output:
[focal] [14:42:21] Fixing file permissions in source directory
[focal] [14:42:22] Destroying container
[xenial] [14:42:50] container-tests-checkbox-support: FAIL
[xenial] output:
[xenial] [14:42:50] Fixing file permissions in source directory
[xenial] [14:42:50] Destroying container

review: Needs Fixing

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

"10.38.105.54"
"10.38.105.108"
"10.38.105.197"
[xenial] [17:35:38] starting container
[focal] [17:35:44] starting container
Device project added to xenial-testing
Device project added to focal-testing
"10.38.105.54"
[xenial] [17:35:56] provisioning container
"10.38.105.151"
[focal] [17:36:06] provisioning container
[bionic] [17:36:07] starting container
Device project added to bionic-testing
"10.38.105.154"
[bionic] [17:36:26] provisioning container
[focal] [17:36:38] Starting tests...
[focal] Found a test script: ./requirements/container-tests-checkbox-support
[bionic] [17:36:55] Starting tests...
[bionic] Found a test script: ./requirements/container-tests-checkbox-support
[focal] [17:37:08] container-tests-checkbox-support: FAIL
[focal] output: https://paste.ubuntu.com/p/8Mf4pRdRnj/
[focal] [17:37:11] Fixing file permissions in source directory
[focal] [17:37:11] Destroying container
[xenial] [17:37:20] Starting tests...
[xenial] Found a test script: ./requirements/container-tests-checkbox-support
[bionic] [17:37:28] container-tests-checkbox-support: FAIL
[bionic] output: https://paste.ubuntu.com/p/xDMQrgJyJN/
[bionic] [17:37:30] Fixing file permissions in source directory
[bionic] [17:37:31] Destroying container
[xenial] [17:37:52] container-tests-checkbox-support: FAIL
[xenial] output: https://paste.ubuntu.com/p/Sy353V7jcJ/
[xenial] [17:37:54] Fixing file permissions in source directory
[xenial] [17:37:54] Destroying container

review: Needs Fixing

[Jonathan Cave (jocave)]

@Stanley the unit tests need updating for this script. See the pastebins above for failure

review: Needs Fixing

[StanleyHuang (stanley31)]

attached test results of the unit test:

test_volume_regex_trusty (test_audio_settings.RegexTest)
Testing pactl 4.0 output ... ok
test_volume_regex_xenial (test_audio_settings.RegexTest)
Testing pactl 8.0 output ... ok
test_desktop_bionic_x13 (test_audio_settings.SetProfileTest)
Bionic system with a Intel UHD Graphics chipset, it's DMIC system. ... ok
test_desktop_precise_radeon_available (test_audio_settings.SetProfileTest)
Home-made system running Precise with a Radeon card. ... ok
test_desktop_precise_radeon_not_available (test_audio_settings.SetProfileTest)
Home-made system running Precise with a Radeon card. ... ok
test_desktop_precise_xps1340 (test_audio_settings.SetProfileTest)
Precise system with a Nvidia chipset. ... ok
test_desktop_raring_t430s_available (test_audio_settings.SetProfileTest)
Raring system with a Mini-DisplayPort. ... ok
test_desktop_raring_t430s_not_available (test_audio_settings.SetProfileTest)
Raring system with a Mini-DisplayPort. ... ok

----------------------------------------------------------------------
Ran 8 tests in 7.784s

OK

[Jonathan Cave (jocave)]

Hmmm I'm wondering why they are passing for you because I'm seeing:

======================================================================
ERROR: test_volume_regex_trusty (checkbox_support.scripts.tests.test_audio_settings.RegexTest)
Testing pactl 4.0 output
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/host/checkbox-project/checkbox-support/checkbox_support/scripts/tests/test_audio_settings.py", line 179, in test_volume_regex_trusty
    volume_regex = re.compile(volume_pattern % "0", re.DOTALL)
TypeError: not all arguments converted during string formatting

======================================================================
ERROR: test_volume_regex_xenial (checkbox_support.scripts.tests.test_audio_settings.RegexTest)
Testing pactl 8.0 output
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/host/checkbox-project/checkbox-support/checkbox_support/scripts/tests/test_audio_settings.py", line 187, in test_volume_regex_xenial
    volume_regex = re.compile(volume_pattern % "front-left", re.DOTALL)
TypeError: not all arguments converted during string formatting

[StanleyHuang (stanley31)]

Interesting, I guess it related to the python version.
I am going to take a look how does the audio_settings scripts works in trusty and xenial (with default python environment.

> Hmmm I'm wondering why they are passing for you because I'm seeing:
>
> ======================================================================
> ERROR: test_volume_regex_trusty
> (checkbox_support.scripts.tests.test_audio_settings.RegexTest)
> Testing pactl 4.0 output
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/ubuntu/host/checkbox-project/checkbox-
> support/checkbox_support/scripts/tests/test_audio_settings.py", line 179, in
> test_volume_regex_trusty
> volume_regex = re.compile(volume_pattern % "0", re.DOTALL)
> TypeError: not all arguments converted during string formatting
>
> ======================================================================
> ERROR: test_volume_regex_xenial
> (checkbox_support.scripts.tests.test_audio_settings.RegexTest)
> Testing pactl 8.0 output
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/ubuntu/host/checkbox-project/checkbox-
> support/checkbox_support/scripts/tests/test_audio_settings.py", line 187, in
> test_volume_regex_xenial
> volume_regex = re.compile(volume_pattern % "front-left", re.DOTALL)
> TypeError: not all arguments converted during string formatting

[StanleyHuang (stanley31)]

I have modified the way to format string, and run the unittest on focal and impish.
@jocave, please give it a try on your test environment. Thanks.

[Jonathan Cave (jocave)]

Yep, this works for me now. Attempting to land...

review: Approve

diff --git a/checkbox_support/scripts/audio_settings.py b/checkbox_support/scripts/audio_settings.py
index 6c00743..2cf6a3f 100644
--- a/checkbox_support/scripts/audio_settings.py
+++ b/checkbox_support/scripts/audio_settings.py
@@ -36,13 +36,16 @@ from checkbox_support.snap_utils.system import in_classic_snap
 TYPES = ("source", "sink")
 DIRECTIONS = {"source": "input", "sink": "output"}
 
+# use %s string format to compatible with other python version
 default_pattern = "(?<=Default %s: ).*"
 index_regex = re.compile("(?<=Sink Input #)[0-9]*")
 muted_regex = re.compile("(?<=Mute: ).*")
-volume_regex = re.compile("Volume: (?:0|front-left):[\s\/0-9]*\s([0-9]*)")
 name_regex = re.compile("(?<=Name:).*")
+channel_map_regex = re.compile("(?<=Channel Map: ).*")
 
+# use %s string format to compatible with other python version
 entry_pattern = "Name: %s.*?(?=Properties)"
+volume_pattern = r"Volume: .*(?:%s):[\w\/0-9 ]* ([0-9]*)%%"
 
 
 def unlocalized_env(reset={"LANG": "POSIX.UTF-8"}):
@@ -144,8 +147,8 @@ def set_profile_hdmi():
     try:
         check_call(["pactl", "set-card-profile", card, profile])
     except CalledProcessError as error:
-        logging.error("Failed setting audio output to:%s: %s" %
-                      (profile, error))
+        logging.error("Failed setting audio output to:{}: {}".format(
+                        profile, error))
 
 
 def get_current_profiles_settings(profiles_file):
@@ -156,8 +159,8 @@ def get_current_profiles_settings(profiles_file):
     config = configparser.ConfigParser()
 
     for match in re.finditer(
-        "(?P<card_id>Card #\d+)\n\tName:\s+(?P<card_name>.*?)\n.*?"
-        "Active\sProfile:\s+(?P<profile>.*?)\n", pactl_list, re.M | re.S
+        r"(?P<card_id>Card #\d+)\n\tName:\s+(?P<card_name>.*?)\n.*?"
+        r"Active\sProfile:\s+(?P<profile>.*?)\n", pactl_list, re.M | re.S
     ):
         config[match.group('card_id')] = {
             'name': match.group('card_name'),
@@ -168,8 +171,8 @@ def get_current_profiles_settings(profiles_file):
         with open(profiles_file, 'w') as active_profiles:
             config.write(active_profiles)
     except IOError:
-        logging.error("Failed to save active profiles information: %s" %
-                      sys.exc_info()[1])
+        logging.error("Failed to save active profiles information: {}".format(
+                        sys.exc_info()[1]))
 
 
 def restore_profiles_settings(profiles_file):
@@ -184,9 +187,9 @@ def restore_profiles_settings(profiles_file):
             check_call(["pactl", "set-card-profile", config[card]['name'],
                        config[card]['profile']])
         except CalledProcessError as error:
-            logging.error("Failed setting card <%s> profile to <%s>: %s" %
-                          (config[card]['name'],
-                           config[card]['profile'], error))
+            logging.error(
+                "Failed setting card <{}> profile to <{}>: {}".format(
+                    config[card]['name'], config[card]['profile'], error))
 
 
 def move_sinks(name):
@@ -201,49 +204,68 @@ def move_sinks(name):
                 check_call(["pactl", "move-sink-input", input_index, name],
                            stdout=DEVNULL)
         except CalledProcessError:
-            logging.error("Failed to move input %d to sink %d" %
-                          (input_index, name))
+            logging.error("Failed to move input {} to sink {}".format(
+                            input_index, name))
             sys.exit(1)
 
 
+def get_audio_settings(type, name="default"):
+    if name == "default":
+        pactl_status = check_output(["pactl", "info"],
+                                universal_newlines=True,
+                                env=unlocalized_env())
+        default_regex = re.compile(default_pattern % type.title())
+        name = default_regex.search(pactl_status).group()
+
+    pactl_list = check_output(["pactl", "list", "{}s".format(type)],
+                              universal_newlines=True,
+                              env=unlocalized_env())
+    entry_regex = re.compile(entry_pattern % name, re.DOTALL)
+    entry = entry_regex.search(pactl_list).group()
+
+    muted = muted_regex.search(entry).group()
+
+    volumes = {}
+    max_volume = 0
+    channels = channel_map_regex.search(entry).group()
+    for channel in channels.split(","):
+        volume_regex = re.compile(volume_pattern % channel, re.DOTALL)
+        _volume = int(volume_regex.search(entry).group(1).strip())
+        volumes.update({channel: _volume})
+        max_volume = max(_volume, max_volume)
+
+    return {
+        "name": name,
+        "muted": muted,
+        "volumes": volumes,
+        "max_volume": max_volume
+    }
+
+
 def store_audio_settings(file):
     logging.info("[ Saving audio settings ]".center(80, '='))
     try:
         settings_file = open(file, 'w')
     except IOError:
-        logging.error("Failed to save settings: %s" % sys.exc_info()[1])
+        logging.error("Failed to save settings: {}".format(sys.exc_info()[1]))
         sys.exit(1)
 
     for type in TYPES:
-        pactl_status = check_output(["pactl", "info"],
-                                    universal_newlines=True,
-                                    env=unlocalized_env())
-        default_regex = re.compile(default_pattern % type.title())
-        default = default_regex.search(pactl_status).group()
-
-        print("default_%s: %s" % (type, default), file=settings_file)
-
-        pactl_list = check_output(["pactl", "list", type + 's'],
-                                  universal_newlines=True,
-                                  env=unlocalized_env())
-
-        entry_regex = re.compile(entry_pattern % default, re.DOTALL)
-        entry = entry_regex.search(pactl_list).group()
-
-        muted = muted_regex.search(entry)
-        print("%s_muted: %s" % (type, muted.group().strip()),
+        audio_settings = get_audio_settings(type)
+        print("default_{}: {}".format(type, audio_settings["name"]),
               file=settings_file)
-
-        volume = int(volume_regex.search(entry).group(1).strip())
-
-        print("%s_volume: %s%%" % (type, str(volume)),
+        print("{}_muted: {}".format(type, audio_settings["muted"].strip()),
               file=settings_file)
+        print("{}_volume: {}%".format(
+                type, str(audio_settings["max_volume"])),
+              file=settings_file)
+
     settings_file.close()
 
 
 def set_audio_settings(device, mute, volume):
     for type in TYPES:
-        pactl_entries = check_output(["pactl", "list", type + 's'],
+        pactl_entries = check_output(["pactl", "list", "{}s".format(type)],
                                      universal_newlines=True,
                                      env=unlocalized_env())
 
@@ -257,10 +279,12 @@ def set_audio_settings(device, mute, volume):
                     logging.info("[ Fallback sink ]".center(80, '='))
                     logging.info("Name: {}".format(name))
                     with open(os.devnull, 'wb') as DEVNULL:
-                        check_call(["pactl", "set-default-%s" % type, name],
+                        check_call(["pactl",
+                                    "set-default-{}".format(type),
+                                    name],
                                    stdout=DEVNULL)
                 except CalledProcessError:
-                    logging.error("Failed to set default %s" % type)
+                    logging.error("Failed to set default {}".format(type))
                     sys.exit(1)
 
                 if type == "sink":
@@ -268,16 +292,18 @@ def set_audio_settings(device, mute, volume):
 
                 try:
                     check_call(["pactl",
-                                "set-%s-mute" % type, name, str(int(mute))])
+                                "set-{}-mute".format(type),
+                                name,
+                                str(int(mute))])
                 except:
-                    logging.error("Failed to set mute for %s" % name)
+                    logging.error("Failed to set mute for {}".format(name))
                     sys.exit(1)
 
                 try:
-                    check_call(["pactl", "set-%s-volume" % type,
-                               name, str(volume) + '%'])
+                    check_call(["pactl", "set-{}-volume".format(type),
+                               name, "{}%".format(str(volume))])
                 except:
-                    logging.error("Failed to set volume for %s" % name)
+                    logging.error("Failed to set volume for {}".format(name))
                     sys.exit(1)
 
 
@@ -287,8 +313,8 @@ def restore_audio_settings(file):
         with open(file) as f:
             settings_file = f.read().split()
     except IOError:
-        logging.error("Unable to open existing settings file: %s" %
-                      sys.exc_info()[1])
+        logging.error("Unable to open existing settings file: {}".format(
+                            sys.exc_info()[1]))
         return 1
 
     for type in TYPES:
@@ -297,10 +323,11 @@ def restore_audio_settings(file):
         # is incorrect, so we just abort.
         try:
             name = settings_file[
-                settings_file.index("default_%s:" % type) + 1]
-            muted = settings_file[settings_file.index("%s_muted:" % type) + 1]
+                settings_file.index("default_{}:".format(type)) + 1]
+            muted = settings_file[
+                settings_file.index("{}_muted:".format(type)) + 1]
             volume = settings_file[
-                settings_file.index("%s_volume:" % type) + 1]
+                settings_file.index("{}_volume:".format(type)) + 1]
         except ValueError:
             logging.error("Unable to restore settings because settings "
                           "file is invalid")
@@ -308,25 +335,25 @@ def restore_audio_settings(file):
 
         try:
             with open(os.devnull, 'wb') as DEVNULL:
-                check_call(["pactl", "set-default-%s" % type, name],
+                check_call(["pactl", "set-default-{}".format(type), name],
                            stdout=DEVNULL)
         except CalledProcessError:
-            logging.error("Failed to restore default %s" % name)
+            logging.error("Failed to restore default {}".format(name))
             return 1
 
         if type == "sink":
             move_sinks(name)
 
         try:
-            check_call(["pactl", "set-%s-mute" % type, name, muted])
+            check_call(["pactl", "set-{}-mute".format(type), name, muted])
         except:
-            logging.error("Failed to restore mute for %s" % name)
+            logging.error("Failed to restore mute for {}".format(name))
             return 1
 
         try:
-            check_call(["pactl", "set-%s-volume" % type, name, volume])
+            check_call(["pactl", "set-{}-volume".format(type), name, volume])
         except:
-            logging.error("Failed to restore volume for %s" % name)
+            logging.error("Failed to restore volume for {}".format(name))
             return 1
 
 
@@ -359,7 +386,7 @@ def main():
             logging.error("No file specified to store audio settings!")
             return 1
         settings_file = args.file
-        profiles_file = args.file + ".profiles"
+        profiles_file = "{}.profiles".format(args.file)
 
     if args.verbose:
         logging.basicConfig(format='%(levelname)s:%(message)s',
@@ -383,7 +410,7 @@ def main():
             set_profile_hdmi()
         set_audio_settings(args.device, args.mute, args.volume)
     else:
-        logging.error(args.action + "is not a valid action")
+        logging.error("{} is not a valid action".format(args.action))
         return 1
 
     return 0
diff --git a/checkbox_support/scripts/tests/test_audio_settings.py b/checkbox_support/scripts/tests/test_audio_settings.py
index 9f81f97..6a6b0ba 100644
--- a/checkbox_support/scripts/tests/test_audio_settings.py
+++ b/checkbox_support/scripts/tests/test_audio_settings.py
@@ -22,9 +22,10 @@ from __future__ import print_function
 from __future__ import unicode_literals
 
 import os
+import re
 import unittest
 
-from checkbox_support.scripts.audio_settings import _guess_hdmi_profile, volume_regex
+from checkbox_support.scripts.audio_settings import _guess_hdmi_profile, volume_pattern
 from checkbox_support.parsers.tests.test_pactl import PactlDataMixIn
 
 
@@ -171,10 +172,11 @@ class SetProfileTest(unittest.TestCase, PactlDataMixIn):
             ('0', 'Hdmi2'))
 
 class RegexTest(unittest.TestCase):
-    
+
     def test_volume_regex_trusty(self):
         """Testing pactl 4.0 output"""
         pactl_volume = "    Volume: 0:  47% 1:  47%"
+        volume_regex = re.compile(volume_pattern % "0", re.DOTALL)
         volume = int(volume_regex.search(pactl_volume).group(1).strip())
         self.assertEqual(volume, 47)
 
@@ -182,5 +184,6 @@ class RegexTest(unittest.TestCase):
         """Testing pactl 8.0 output"""
         # See lp:1595380 for more info
         pactl_volume = "    Volume: front-left: 65536 / 100% / 0.00 dB,   front-right: 65536 / 100% / 0.00 dB"
+        volume_regex = re.compile(volume_pattern % "front-left", re.DOTALL)
         volume = int(volume_regex.search(pactl_volume).group(1).strip())
         self.assertEqual(volume, 100)