From c8479278e63fcb5850dbae7fe2f0a648453da234 Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Fri, 29 Dec 2023 05:15:22 +0000 Subject: [PATCH 1/6] Random fixups while trying to understand and simplify startup and cleanup In particular, we should be able to drop mkchromecast.pid and simplify or drop mkchromecast.tmp. --- bin/mkchromecast | 3 +++ mkchromecast/stream_infra.py | 21 +++++++++++++-------- mkchromecast/utils.py | 34 +++++++++++++++++----------------- mkchromecast/video.py | 1 + start_tray.py | 2 ++ 5 files changed, 36 insertions(+), 25 deletions(-) diff --git a/bin/mkchromecast b/bin/mkchromecast index c01deadf1..a8d01c9d2 100755 --- a/bin/mkchromecast +++ b/bin/mkchromecast @@ -194,6 +194,9 @@ class CastProcess(object): def terminate_app(self): """Terminate the app (kill app)""" + if self.mkcc.debug: + print(f'terminate_app running in pid {os.getpid()}') + self.cc.stop_cast() if self.mkcc.platform == 'Darwin': inputint() diff --git a/mkchromecast/stream_infra.py b/mkchromecast/stream_infra.py index 827afd2ca..f19f65d18 100644 --- a/mkchromecast/stream_infra.py +++ b/mkchromecast/stream_infra.py @@ -262,19 +262,19 @@ def start(self): @staticmethod def _monitor_loop(platform: str): - f = open("/tmp/mkchromecast.pid", "rb") - pidnumber = int(pickle.load(f)) - print(colors.options("PID of main process:") + " " + str(pidnumber)) + with open("/tmp/mkchromecast.pid", "rb") as pid_file: + main_pid = int(pickle.load(pid_file)) + print(colors.options("PID of main process:") + f" {main_pid}") - localpid = os.getpid() - print(colors.options("PID of streaming process:") + " " + str(localpid)) + local_pid = os.getpid() + print(colors.options("PID of streaming process:") + f" {os.getpid()}") - while psutil.pid_exists(localpid) is True: + while psutil.pid_exists(local_pid): try: time.sleep(0.5) # With this I ensure that if the main app fails, everything # will get back to normal - if psutil.pid_exists(pidnumber) is False: + if not psutil.pid_exists(main_pid): if platform == "Darwin": inputint() outputint() @@ -282,10 +282,15 @@ def _monitor_loop(platform: str): from mkchromecast.pulseaudio import remove_sink remove_sink() - parent = psutil.Process(localpid) + parent = psutil.Process(local_pid) + # TODO(xsdg): This is unlikely to finish, given that this + # code itself is running in one of the child processes. We + # should instead signal the parent to terminate, and have it + # handle child cleanup on its own. for child in parent.children(recursive=True): child.kill() parent.kill() + except KeyboardInterrupt: print("Ctrl-c was requested") sys.exit(0) diff --git a/mkchromecast/utils.py b/mkchromecast/utils.py index b113994a1..97ceb3a00 100644 --- a/mkchromecast/utils.py +++ b/mkchromecast/utils.py @@ -146,21 +146,22 @@ def terminate(): return -def del_tmp(): +def del_tmp(debug: bool = False) -> None: """Delete files created in /tmp/""" delete_me = ["/tmp/mkchromecast.tmp", "/tmp/mkchromecast.pid"] - print(colors.important("Cleaning up /tmp/...")) + if debug: + print(colors.important("Cleaning up /tmp/...")) for f in delete_me: - if os.path.exists(f) is True: + if os.path.exists(f): os.remove(f) - print(colors.success("[Done]")) - return + if debug: + print(colors.success("[Done]")) -def is_installed(name, path, debug): +def is_installed(name, path, debug) -> bool: PATH = path iterate = PATH.split(":") for item in iterate: @@ -171,7 +172,7 @@ def is_installed(name, path, debug): if debug is True: print("Program %s found in %s." % (name, verifyif)) return True - return + return False def check_url(url): @@ -183,22 +184,21 @@ def check_url(url): return False -def writePidFile(): +def writePidFile() -> None: + pid_filename = "/tmp/mkchromecast.pid" # This is to verify that pickle tmp file exists - if os.path.exists("/tmp/mkchromecast.pid") is True: - os.remove("/tmp/mkchromecast.pid") + if os.path.exists(pid_filename): + os.remove(pid_filename) + pid = str(os.getpid()) - f = open("/tmp/mkchromecast.pid", "wb") - pickle.dump(pid, f) - f.close() - return + with open(pid_filename, "wb") as pid_file: + pickle.dump(pid, pid_file) -def checkmktmp(): +def checkmktmp() -> None: # This is to verify that pickle tmp file exists - if os.path.exists("/tmp/mkchromecast.tmp") is True: + if os.path.exists("/tmp/mkchromecast.tmp"): os.remove("/tmp/mkchromecast.tmp") - return def check_file_info(name, what=None): diff --git a/mkchromecast/video.py b/mkchromecast/video.py index 721478816..22745df43 100644 --- a/mkchromecast/video.py +++ b/mkchromecast/video.py @@ -97,6 +97,7 @@ def main(): try: subprocess.Popen(webcast) except: + # TODO(xsdg): Capture a specific exception here. print( colors.warning( "Nodejs is not installed in your system. " diff --git a/start_tray.py b/start_tray.py index f3687da9e..8117928ee 100644 --- a/start_tray.py +++ b/start_tray.py @@ -4,6 +4,8 @@ from mkchromecast.utils import checkmktmp, writePidFile import mkchromecast.systray +# TODO(xsdg): This should go through mkchromecast and shouldn't be a separate +# entrypoint. checkmktmp() writePidFile() mkchromecast.systray.main() From 99a16760542a9a8eb054d73d6ae1d477c7fd66fa Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Fri, 5 Jan 2024 22:52:31 +0000 Subject: [PATCH 2/6] Clarifies the standard codepath within mkchromecast, and fixes up some related issues - Doesn't run `atexit` from multiple places - Avoids redundant usage of `atexit` and `signal` - Renames `show_control` to `block_until_exit`, which clarifies that it's actually essentially the main loop of the parent process - Avoids useless recursive call from the 'a' handler - Shares the `KeyboardInterrupt` exception handler between the controls-enabled and controls-disabled codepaths --- bin/mkchromecast | 91 ++++++++++++++++++++----------------------- mkchromecast/utils.py | 10 +---- 2 files changed, 43 insertions(+), 58 deletions(-) diff --git a/bin/mkchromecast b/bin/mkchromecast index a8d01c9d2..ead079c8f 100755 --- a/bin/mkchromecast +++ b/bin/mkchromecast @@ -40,7 +40,6 @@ def maybe_execute_single_action(mkcc: mkchromecast.Mkchromecast): print("mkchromecast " + "v" + colors.success(__version__)) sys.exit(0) - # TODO(xsdg): Stop using conditional imports all over the place. class CastProcess(object): """Class to manage cast process""" @@ -56,6 +55,8 @@ class CastProcess(object): checkmktmp() writePidFile() + atexit.register(self.terminate_app) + self.check_connection() if self.mkcc.tray is False and self.mkcc.videoarg is False: if self.mkcc.platform == 'Linux': @@ -81,14 +82,14 @@ class CastProcess(object): self.cc.initialize_cast() self.get_devices(self.mkcc.select_device) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() elif self.mkcc.youtube_url is None and self.mkcc.source_url is not None: self.start_backend(self.mkcc.backend) self.cc.initialize_cast() self.get_devices(self.mkcc.select_device) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() # When casting youtube url, we do it through the audio module elif self.mkcc.youtube_url is not None and self.mkcc.videoarg is False: @@ -97,7 +98,7 @@ class CastProcess(object): self.cc.initialize_cast() self.get_devices(self.mkcc.select_device) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() def audio_macOS(self): """This method manages all related to casting audio in macOS""" @@ -111,21 +112,21 @@ class CastProcess(object): outputdev() print(colors.success('[Done]')) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() elif self.mkcc.youtube_url is None and self.mkcc.source_url is not None: self.start_backend(self.mkcc.backend) self.cc.initialize_cast() self.get_devices(self.mkcc.select_device) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() print('Switching to BlackHole...') inputdev() outputdev() print(colors.success('[Done]')) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() # When casting youtube url, we do it through the audio module elif self.mkcc.youtube_url is not None and self.mkcc.videoarg is False: @@ -134,7 +135,7 @@ class CastProcess(object): self.cc.initialize_cast() self.get_devices(self.mkcc.select_device) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() def cast_video(self): """This method launches video casting""" @@ -151,7 +152,7 @@ class CastProcess(object): self.cc.initialize_cast() self.get_devices(self.mkcc.select_device) self.cc.play_cast() - self.show_control(self.mkcc.control) + self.block_until_exit() def get_devices(self, select_device: bool, write_to_pickle: bool = True): """Get chromecast name, and let user select one from a list if @@ -203,9 +204,9 @@ class CastProcess(object): outputint() elif self.mkcc.platform == 'Linux': remove_sink() - terminate() + terminate() # Does not return. - def controls_msg(self): + def print_controls_msg(self): """Messages shown when controls is True""" print('') print(colors.important('Controls:')) @@ -221,34 +222,31 @@ class CastProcess(object): print(colors.options('Quit the Application:')+' q or Ctrl-C') print('') - def show_control(self, control): + def block_until_exit(self) -> None: """Method to show controls""" - if control is True: - from mkchromecast.getch import getch + try: + if self.mkcc.control: + from mkchromecast.getch import getch - self.controls_msg() + self.print_controls_msg() - # We capture keys - try: while(True): key = getch() if(key == 'u'): self.cc.volume_up() if self.mkcc.backend == 'ffmpeg': if self.mkcc.debug is True: - self.controls_msg() + self.print_controls_msg() elif(key == 'd'): self.cc.volume_down() if self.mkcc.backend == 'ffmpeg': if self.mkcc.debug is True: - self.controls_msg() + self.print_controls_msg() elif (key == 'a'): print(self.cc.available_devices) self.get_devices(self.mkcc.select_device, write_to_pickle=False) self.cc.play_cast() - # TODO(xsdg): Why is this recursing? - self.show_control(control) elif(key == 'p'): if self.mkcc.videoarg is True: print('Pausing Casting Process...') @@ -256,9 +254,7 @@ class CastProcess(object): self.backend_handler(action, self.mkcc.backend) if self.mkcc.backend == 'ffmpeg': if self.mkcc.debug is True: - self.controls_msg() - else: - pass + self.print_controls_msg() elif(key == 'r'): if self.mkcc.videoarg is True: print('Resuming Casting Process...') @@ -266,33 +262,30 @@ class CastProcess(object): self.backend_handler(action, self.mkcc.backend) if self.mkcc.backend == 'ffmpeg': if self.mkcc.debug is True: - self.controls_msg() - else: - pass - elif(key == 'q'): - print(colors.error('Quitting application...')) - self.terminate_app() - elif(key == '\x03'): + self.print_controls_msg() + elif(key in {'q', '\x03'}): + # "q" or ^D raise KeyboardInterrupt - # TODO(xsdg): Move atexit to run() method. - atexit.register(self.terminate_app()) - except KeyboardInterrupt: - self.terminate_app() - else: - if self.mkcc.platform == 'Linux' and self.mkcc.adevice is None: - print(colors.warning('Remember to open pavucontrol and select ' - 'the mkchromecast sink.')) - print('') - print(colors.error('Ctrl-C to kill the Application at any Time')) - print('') - # TODO(xsdg): atexit is incorrect here. The signal handlers should - # directly call terminate_app. - signal.signal(signal.SIGINT, - lambda *_: atexit.register(self.terminate_app())) - signal.signal(signal.SIGTERM, - lambda *_: atexit.register(self.terminate_app())) - signal.pause() + else: + if self.mkcc.platform == 'Linux' and self.mkcc.adevice is None: + print(colors.warning('Remember to open pavucontrol and select ' + 'the mkchromecast sink.')) + print('') + print(colors.error('Ctrl-C to kill the Application at any Time')) + print('') + # TODO(xsdg): atexit is incorrect here. The signal handlers should + # directly call terminate_app. + #signal.signal(signal.SIGINT, + # lambda *_: atexit.register(self.terminate_app())) + #signal.signal(signal.SIGTERM, + # lambda *_: atexit.register(self.terminate_app())) + signal.pause() + + except KeyboardInterrupt: + print() + print(colors.error('Quitting application...')) + self.terminate_app() def backend_handler(self, action, backend): """Methods to handle pause and resume state of backends""" diff --git a/mkchromecast/utils.py b/mkchromecast/utils.py index 97ceb3a00..99db7078e 100644 --- a/mkchromecast/utils.py +++ b/mkchromecast/utils.py @@ -14,13 +14,6 @@ from mkchromecast import messages -""" -To call them: - from mkchromecast.terminate import name - name() -""" - - def quantize_sample_rate(has_source_url: bool, codec: str, sample_rate: int, @@ -136,14 +129,13 @@ def clamp_bitrate(codec: str, bitrate: Optional[int]) -> int: return bitrate -def terminate(): +def terminate() -> None: del_tmp() parent_pid = os.getpid() parent = psutil.Process(parent_pid) for child in parent.children(recursive=True): child.kill() parent.kill() - return def del_tmp(debug: bool = False) -> None: From efce4f326938d0ef828b1840dd7c9473206da604 Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Sat, 6 Jan 2024 00:21:00 +0000 Subject: [PATCH 3/6] Fixes a bug that caused args to be parsed multiple times (instead of once per interpreter process, as designed) --- mkchromecast/__init__.py | 43 ++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/mkchromecast/__init__.py b/mkchromecast/__init__.py index c564bab5f..bdc21fb89 100644 --- a/mkchromecast/__init__.py +++ b/mkchromecast/__init__.py @@ -19,10 +19,12 @@ class Mkchromecast: def __init__(self, args = None): # TODO(xsdg): Require arg parsing to be done outside of this class. + first_parse: bool = False if not args: if not self._parsed_args: - self._parsed_args = _arg_parsing.Parser.parse_args() - args = self._parsed_args + Mkchromecast._parsed_args = _arg_parsing.Parser.parse_args() + first_parse = True + args = Mkchromecast._parsed_args self.args = args self.debug: bool = args.debug @@ -202,23 +204,26 @@ def __init__(self, args = None): # Diagnostic messages - self._debug(f"ALSA device name: {self.adevice}") - self._debug(f"Google Cast name: {self.device_name}") - self._debug(f"backend: {self.backend}") - - # TODO(xsdg): These were just printed warnings in the original, but - # should they be errors? - if self.mtype and not self.videoarg: - print(colors.warning( - "The media type argument is only supported for video.")) - - if self.loop and self.videoarg: - print(colors.warning( - "The loop and video arguments aren't compatible.")) - - if self.command and not self.videoarg: - print(colors.warning( - "The --command option only works for video.")) + if first_parse: + self._debug(f"Parsed args in process {os.getpid()}") + + self._debug(f"ALSA device name: {self.adevice}") + self._debug(f"Google Cast name: {self.device_name}") + self._debug(f"backend: {self.backend}") + + # TODO(xsdg): These were just printed warnings in the original, but + # should they be errors? + if self.mtype and not self.videoarg: + print(colors.warning( + "The media type argument is only supported for video.")) + + if self.loop and self.videoarg: + print(colors.warning( + "The loop and video arguments aren't compatible.")) + + if self.command and not self.videoarg: + print(colors.warning( + "The --command option only works for video.")) def _validate_input_file(self) -> None: if not self.input_file: From 64c01a9c2547d07f47d186f8e16cb15b2e8ef33a Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Sat, 6 Jan 2024 00:21:36 +0000 Subject: [PATCH 4/6] Groups action arguments and makes them mutually exclusive. This will avoid the need for stacks of conditions, such as `if self.mkcc.youtube_url is None and self.mkcc.source_url is None:` --- mkchromecast/_arg_parsing.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/mkchromecast/_arg_parsing.py b/mkchromecast/_arg_parsing.py index 41a2fb51a..ccd654e6e 100644 --- a/mkchromecast/_arg_parsing.py +++ b/mkchromecast/_arg_parsing.py @@ -57,6 +57,14 @@ def raise_arg_type_error(): formatter_class=argparse.RawTextHelpFormatter, ) +# All of the "do this action" arguments. +_ActionGroupContainer = Parser.add_argument_group( + title='Actions', + description=('Optional actions that mkchromecast can perform. These are ' + 'mutually-exclusive.'), +) +_ActionGroup = _ActionGroupContainer.add_mutually_exclusive_group() + Parser.add_argument( "--alsa-device", type=str, @@ -176,7 +184,7 @@ def raise_arg_type_error(): """, ) -Parser.add_argument( +_ActionGroup.add_argument( "-d", "--discover", action="store_true", @@ -241,7 +249,7 @@ def raise_arg_type_error(): """, ) -Parser.add_argument( +_ActionGroup.add_argument( "-i", "--input-file", type=str, @@ -314,7 +322,7 @@ def raise_arg_type_error(): ) -Parser.add_argument( +_ActionGroup.add_argument( "-r", "--reset", action="store_true", @@ -391,7 +399,7 @@ def raise_arg_type_error(): """, ) -Parser.add_argument( +_ActionGroup.add_argument( "--screencast", action="store_true", default=False, @@ -433,7 +441,7 @@ def raise_arg_type_error(): """, ) -Parser.add_argument( +_ActionGroup.add_argument( "--source-url", type=str, default=None, @@ -473,7 +481,7 @@ def raise_arg_type_error(): """, ) -Parser.add_argument( +_ActionGroup.add_argument( "-t", "--tray", action="store_true", @@ -498,7 +506,7 @@ def raise_arg_type_error(): help="Do not use. Argument dropped.", ) -Parser.add_argument( +_ActionGroup.add_argument( "-v", "--version", action="store_true", @@ -542,7 +550,7 @@ def raise_arg_type_error(): help="Do not use. Renamed to --control.", ) -Parser.add_argument( +_ActionGroup.add_argument( "-y", "--youtube", type=str, From 7dc611b7e0ad3d3980e77d91ae2b5f8b815f0303 Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Sat, 6 Jan 2024 02:53:28 +0000 Subject: [PATCH 5/6] Fixes newlines in bin/mkchromecast --- bin/mkchromecast | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bin/mkchromecast b/bin/mkchromecast index ead079c8f..2801a8f25 100755 --- a/bin/mkchromecast +++ b/bin/mkchromecast @@ -22,6 +22,7 @@ from mkchromecast.pulseaudio import create_sink, get_sink_list, remove_sink from mkchromecast.utils import terminate, checkmktmp, writePidFile from mkchromecast.messages import print_available_devices + def maybe_execute_single_action(mkcc: mkchromecast.Mkchromecast): """Potentially executes a one-off action, followed by exiting.""" @@ -40,6 +41,7 @@ def maybe_execute_single_action(mkcc: mkchromecast.Mkchromecast): print("mkchromecast " + "v" + colors.success(__version__)) sys.exit(0) + # TODO(xsdg): Stop using conditional imports all over the place. class CastProcess(object): """Class to manage cast process""" From d18f8a69372ddd35a72b3eb544bfbde164555c30 Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Sat, 6 Jan 2024 02:56:15 +0000 Subject: [PATCH 6/6] Drops some unneeded commented-out code. --- bin/mkchromecast | 6 ------ 1 file changed, 6 deletions(-) diff --git a/bin/mkchromecast b/bin/mkchromecast index 2801a8f25..7e2a7758e 100755 --- a/bin/mkchromecast +++ b/bin/mkchromecast @@ -276,12 +276,6 @@ class CastProcess(object): print('') print(colors.error('Ctrl-C to kill the Application at any Time')) print('') - # TODO(xsdg): atexit is incorrect here. The signal handlers should - # directly call terminate_app. - #signal.signal(signal.SIGINT, - # lambda *_: atexit.register(self.terminate_app())) - #signal.signal(signal.SIGTERM, - # lambda *_: atexit.register(self.terminate_app())) signal.pause() except KeyboardInterrupt: