Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(feat): Windows support #85

Merged
merged 24 commits into from
Aug 4, 2021
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
49f70fb
Windows support using an artificial FIFO.
mariocynicys Jul 23, 2021
c9d275e
invoking both processes as shell because it doesn't work on windows f…
mariocynicys Jul 23, 2021
1a79cfb
tests tested from windows
mariocynicys Jul 23, 2021
3d2c926
adding pywin32api to the prerequisites list
mariocynicys Jul 29, 2021
87f9559
to be tested on windows
mariocynicys Jul 30, 2021
a8cac17
moving the windows logic to the OutputStream class, also splitting pi…
mariocynicys Jul 30, 2021
22d907f
minor edits1
mariocynicys Jul 30, 2021
222471c
minor edits2
mariocynicys Jul 30, 2021
8fff654
minor edits3
mariocynicys Jul 30, 2021
0b875c1
amend to this
mariocynicys Aug 1, 2021
eac2d66
replace the ipc pipe with a file pipe when transcoding is skipped
mariocynicys Aug 2, 2021
0a8905d
style edit
mariocynicys Aug 2, 2021
5e52c41
dropping shell=True
mariocynicys Aug 2, 2021
30987f4
style edits
mariocynicys Aug 2, 2021
c5a1201
style edits
mariocynicys Aug 2, 2021
78b3210
irrelevant comment
mariocynicys Aug 2, 2021
f4c6619
to be tested on windows
mariocynicys Aug 3, 2021
9708518
add pywin32 as an installtion requisite for Windows
mariocynicys Aug 3, 2021
d52d65d
Merge branch 'windows-new' into conflicts
mariocynicys Aug 4, 2021
1f5f0b4
removing the {dir} placeholder from the output file templates
mariocynicys Aug 4, 2021
f42bbc0
a stream must be software acc to apply these args to it
mariocynicys Aug 4, 2021
6917446
print the runtime error content
mariocynicys Aug 4, 2021
e1e285f
Merge pull request #3 from meryacine/conflicts
mariocynicys Aug 4, 2021
d30dc73
renaming a var
mariocynicys Aug 4, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ shaka_streamer.egg-info/
.idea/
venv/
dev/
.vscode/
.vscode/
packager.exe
20 changes: 11 additions & 9 deletions run_end_to_end_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ def hlsStreamsReady(master_playlist_path):
return False

for playlist_path in playlist_list:
if playlist_path == master_playlist_path:
# Use os.path.samefile method instead of the == operator because
# this might be a windows machine.
if os.path.samefile(playlist_path, master_playlist_path):
# Skip the master playlist
continue

Expand Down Expand Up @@ -269,7 +271,8 @@ def main():
return 1

# Install test dependencies.
subprocess.check_call(['npm', 'install'])
npm_command = ' '.join(['npm', 'install'])
subprocess.check_call(npm_command, shell=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use shell=True? If "npm" is in your PATH, then the old way should work.

Quoting the Python docs:

On Windows with shell=True, the COMSPEC environment variable specifies the default shell. The only time you need to specify shell=True on Windows is when the command you wish to execute is built into the shell (e.g. dir or copy). You do not need shell=True to run a batch file or console-based executable.

In general, I believe it's best practice to avoid shell=True when possible. Invoking a shell and passing all arguments as a single string can be problematic when the arguments contain spaces or other special characters. And if the arguments include user input, there is a risk of malicious inputs being used to run other commands (similar to an SQL injection attack). See also Python docs on "security considerations" for shell=True.

In this case, there's obviously no risk with shell=True, but I'd prefer to stick to shell=False whenever possible.

This reminds me, though, that we had a similar issue long ago in Shaka Player's build scripts (also written in Python). I think for windows, "npm" resolves to "npm.cmd" rather than "npm.exe", but subprocess won't try ".cmd" automatically like the Windows shell will. I believe we ended up with something similar to:

npm_exe = 'npm.cmd' if windows else 'npm'
subprocess.check_call([npm_exe, 'install'])

I found a stack overflow talking about the same thing: https://stackoverflow.com/a/50045443

It appears that the same technique is not required for "node", which is actually "node.exe" on Windows.

Please use something like that for now. I'm going to experiment with monkey-patching subprocess for Windows to find .cmd. It seems like someone should have done that by now, but I can't find it, so I'll write it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thaaaanks, It didn't make any sense to me why subprocess couldn't find it but i can invoke it from my cmd. thaaanks 😃


# Fetch streams used in tests.
if not os.path.exists(TEST_DIR):
Expand All @@ -289,24 +292,23 @@ def main():

for i in range(trials):
# Start up karma.
karma_args = [
karma_command = ' '.join([
'node',
'node_modules/karma/bin/karma',
'start',
'tests/karma.conf.js',
# DRM currently is not compatible with headless, so it's run in Chrome.
# Linux: If you want to run tests as "headless", wrap it with "xvfb-run -a".
'--browsers', 'Chrome',
'--single-run',
]
])

if args.reporters:
converted_string = ','.join(args.reporters)
karma_args += [
'--reporters',
converted_string,
]
karma_command += ' --reporters {}'.format(converted_string)

# If the exit code was not 0, the tests in karma failed or crashed.
if subprocess.call(karma_args) != 0:
if subprocess.call(karma_command, shell=True) != 0:
fails += 1

print('\n\nNumber of failures:', fails, '\nNumber of trials:', trials)
Expand Down
9 changes: 6 additions & 3 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,18 @@
# limitations under the License.

import base64
import os
import setuptools

import streamer

with open('README.md', 'r') as f:
long_description = f.read()

install_prerequisites = ['PyYAML']
if os.name == 'nt':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will only run on the system where we build the package, which will usually be Linux. Checking os.name here won't work, because no matter what platform we install it on later, this part is only run once when we build the package.

Instead, to express a platform-specific dependency, I just found out there is a syntax for that:

'pypiwin32 ; platform_system=="Windows"'

See https://stackoverflow.com/a/42946495

install_prerequisites.append('pypiwin32')

setuptools.setup(
name='shaka-streamer',
version=streamer.__version__,
Expand All @@ -29,9 +34,7 @@
long_description_content_type='text/markdown',
url='https://github.com/google/shaka-streamer',
packages=setuptools.find_packages(),
install_requires=[
'PyYAML',
],
install_requires=install_prerequisites,
scripts=['shaka-streamer'],
classifiers=[
'Programming Language :: Python :: 3',
Expand Down
36 changes: 22 additions & 14 deletions streamer/controller_node.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from streamer.pipeline_configuration import PipelineConfig, StreamingMode
from streamer.transcoder_node import TranscoderNode
from streamer.periodconcat_node import PeriodConcatNode
from streamer.winfifo import WinFIFO


class ControllerNode(object):
Expand Down Expand Up @@ -79,21 +80,23 @@ def _create_pipe(self, suffix = '') -> str:
Returns:
The path to the named pipe, as a string.
"""

# TODO(#8): mkfifo only works on Unix. We would need a special case for a
# Windows port some day.

if not hasattr(os, 'mkfifo'):
raise RuntimeError('Platform not supported due to lack of mkfifo')

# Since the tempfile module creates actual files, use uuid to generate a
# filename, then call mkfifo to create the named pipe.
unique_name = str(uuid.uuid4()) + suffix
path = os.path.join(self._temp_dir, unique_name)

readable_by_owner_only = 0o600 # Unix permission bits
os.mkfifo(path, mode=readable_by_owner_only)


# For POSIX systems.
if os.name == 'posix':
path = os.path.join(self._temp_dir, unique_name)
readable_by_owner_only = 0o600 # Unix permission bits
os.mkfifo(path, mode=readable_by_owner_only) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "type: ignore"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy complains on windows for this.


# New Technology, aka WindowsNT.
elif os.name == 'nt':
path = '-nt-shaka-' + unique_name
WinFIFO(path).start()
else:
raise RuntimeError('Platform not supported.')

return path

def start(self, output_dir: str,
Expand Down Expand Up @@ -209,9 +212,14 @@ def _append_nodes_for_inputs_list(self, inputs: List[Input],
# read from that pipe for this input.
if input.input_type == InputType.EXTERNAL_COMMAND:
command_output = self._create_pipe()
writer_command_output = command_output
reader_command_output = command_output
if os.name == 'nt':
writer_command_output = WinFIFO.WRITER_PREFIX + command_output
reader_command_output = WinFIFO.READER_PREFIX + command_output
self._nodes.append(ExternalCommandNode(
input.name, command_output))
input.set_pipe(command_output)
input.name, writer_command_output))
input.set_pipe(reader_command_output)

if input.media_type == MediaType.AUDIO:
for audio_codec in self._pipeline_config.audio_codecs:
Expand Down
12 changes: 11 additions & 1 deletion streamer/output_stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@

"""Contains information about each output stream."""

import os

from streamer.bitrate_configuration import AudioCodec, AudioChannelLayout, VideoCodec, VideoResolution
from streamer.input_configuration import Input, MediaType
from streamer.winfifo import WinFIFO
from typing import Dict, Optional, Union


Expand All @@ -29,7 +32,14 @@ def __init__(self,
codec: Union[AudioCodec, VideoCodec, None]) -> None:
self.type: MediaType = type
# If "pipe" is None, then this will not be transcoded.
self.pipe: Optional[str] = pipe
self.read_pipe: Optional[str] = pipe
self.writ_pipe: Optional[str] = pipe
# On posix systems, the read and write pipes will be the same.
# But on Windows, we will have different read and write pipe names.
if os.name == 'nt' and self.read_pipe and self.writ_pipe:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the os.name == 'nt' checks and path manipulations being spread around. This could be encapsulated into a class (called something like "Pipe"), and there could be "reader_path" and "writer_path" attributes on that class. For os.name == 'posix', they would be paths to FIFOs, and identical. For os.name == 'nt', they would be different, and would be built on your "winfifo" module. _create_pipe() would return one of these objects, and everywhere we used to use paths for pipes, we now could use a Pipe object.

Today, the end of the writer pipeline (PackagerNode) writes to a regular file. But it might be useful to avoid hard-coding knowledge of the termination of a pipeline into the nodes. For example, rather than PackagerNode taking Pipe objects for inputs and string paths for outputs, it could take a Pipe for both. The output Pipe for PackagerNode would be a variant where the writer_path was a plain file path.

Does this make sense?

And now that I'm reading through the WinFIFO class, I think it's not far off from what I'm suggesting. You could extend that class to cover the posix case, rename it, and pass it to the nodes in place of string paths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i understood that correctly, you mean to put a pipe before and after each node in the pipe line, like this.
image

So we would make a new file pipe.py and let all the platform diversion logic be in there, and also the pipe path naming(could be a file).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need only the middle and the rightmost pipe in the blueprint up above, as the inputs will represent leftmost pipe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have invested some time working on removing the 'file' terminology from the PackagerNode and replacing it with an output pipe instead, but i am afraid that we might not be able to do that, since the output file is sometimes two and not one, and is a template for the initialization segment and other segments of the media file.
We might though be able to embed these knowledge in the OutputStream class instead and let it deal with the pipe creation and file templates too.

# Prefix the pipe names on Windows platforms.
self.read_pipe = WinFIFO.READER_PREFIX + self.read_pipe
self.writ_pipe = WinFIFO.WRITER_PREFIX + self.writ_pipe
self.input: Input = input
self.codec: Union[AudioCodec, VideoCodec, None] = codec
self._features: Dict[str, str] = {}
Expand Down
3 changes: 2 additions & 1 deletion streamer/packager_node.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,11 @@ def start(self) -> None:
stdout=stdout)

def _setup_stream(self, stream: OutputStream) -> str:

dict = {
# If pipe is None, this wasn't transcoded, so we take the input path
# directly.
'in': stream.pipe or stream.input.name,
'in': stream.read_pipe or stream.input.name,
'stream': stream.type.value,
}

Expand Down
5 changes: 2 additions & 3 deletions streamer/transcoder_node.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def start(self) -> None:
if output_stream.input != input:
# Skip outputs that don't match this exact input object.
continue
if output_stream.pipe is None:
if output_stream.writ_pipe is None:
# This input won't be transcoded. This is common for VTT text input.
continue

Expand All @@ -140,8 +140,7 @@ def start(self) -> None:
assert(isinstance(output_stream, TextOutputStream))
args += self._encode_text(output_stream, input)

# The output pipe.
args += [output_stream.pipe]
args += [output_stream.writ_pipe]

env = {}
if self._pipeline_config.debug_logs:
Expand Down
80 changes: 80 additions & 0 deletions streamer/winfifo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# Copyright 2019 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from threading import Thread
import win32pipe, win32file, pywintypes # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why type: ignore here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy complains on linux for this, also on windows, as there are no library type hints for these modules.


class WinFIFO(Thread):
"""A threaded class that serves as a FIFO pipe that transfers data from a
writer to a reader process on Windows.

It is a replacement for os.mkfifo on POSIX systems."""

READER_PREFIX = r'\\.\pipe\R'
WRITER_PREFIX = r'\\.\pipe\W'

def __init__(self, pipe_name: str, buf_size = 64 * 1024):
"""Initializes a thread and creates two named pipes on the system."""

super().__init__(daemon=True)
self.pipe_name = pipe_name
self.BUF_SIZE = buf_size

# The read pipe is connected to a writer process.
self.read_side = win32pipe.CreateNamedPipe(
WinFIFO.WRITER_PREFIX + self.pipe_name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: indent 4 spaces when you put arguments on subsequent lines

win32pipe.PIPE_ACCESS_INBOUND,
win32pipe.PIPE_WAIT | win32pipe.PIPE_TYPE_BYTE | win32pipe.PIPE_READMODE_BYTE,
1,
self.BUF_SIZE,
self.BUF_SIZE,
0,
None)

# The write pipe is connected to a reader process.
self.writ_side = win32pipe.CreateNamedPipe(
WinFIFO.READER_PREFIX + self.pipe_name,
win32pipe.PIPE_ACCESS_OUTBOUND,
win32pipe.PIPE_WAIT | win32pipe.PIPE_TYPE_BYTE | win32pipe.PIPE_READMODE_BYTE,
1,
self.BUF_SIZE,
self.BUF_SIZE,
0,
None)

def run(self):
"""This method serves as a server that connects a writer client
to a reader client.

This methods will run as a thread."""

try:
# Connect to both ends of the pipe before starting the transfer.
# This funciton is blocking. If no process is connected yet, it will wait
# indefinitely.
win32pipe.ConnectNamedPipe(self.read_side)
win32pipe.ConnectNamedPipe(self.writ_side)
while True:
# Writer -> read_side -> writ_side -> Reader
_, data = win32file.ReadFile(self.read_side, self.BUF_SIZE)
win32file.WriteFile(self.writ_side, data)
except Exception as ex:
# Remove the pipes from the system.
win32file.CloseHandle(self.read_side)
win32file.CloseHandle(self.writ_side)
# If the error was due to one of the processes shutting down, just exit normally.
if isinstance(ex, pywintypes.error) and (ex.args[0] == 109 or ex.args[0] == 232):
return 0
# Otherwise, raise that error.
raise ex