-
Notifications
You must be signed in to change notification settings - Fork 62
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
Multi period for DASH #78
Conversation
adding period concatenation for dash, no tests are done yet.
@joeyparrish |
run_end_to_end_tests.py
Outdated
@@ -189,7 +189,7 @@ def stop(): | |||
resp = createCrossOriginResponse() | |||
if controller is not None: | |||
# Check status to see if one of the processes exited. | |||
if controller.check_status() == node_base.ProcessStatus.Errored: | |||
if controller.check_status() == node_base.ProcessStatus.ERRORED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of change would be great as its own change. It would be small and easy to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of change would be great as its own change. It would be small and easy to review.
I agree completely. Please send these non-functional or cosmetic changes separately.
streamer/bitrate_configuration.py
Outdated
@@ -368,7 +368,8 @@ class BitrateConfig(configuration.Base): | |||
""" | |||
|
|||
video_resolutions = configuration.Field( | |||
Dict[str, VideoResolution], default=DEFAULT_VIDEO_RESOLUTIONS).cast() | |||
Dict[str, VideoResolution], | |||
default=DEFAULT_VIDEO_RESOLUTIONS).cast() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid whitespace-only changes, especially in larger changes like these. If you think this is better, I'd suggest having all these style changes as their own change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for that.
streamer/cloud_node.py
Outdated
|
||
Additional output from gsutil: | ||
{}""".format(bucket_url, status.stderr) | ||
message = "Unable to write to cloud storage URL: {}\n\nPlease double-check that the URL is correct, that you are signed into the\nGoogle Cloud SDK or Amazon AWS CLI, and that you have access to the\ndestination bucket.\n\nAdditional output from gsutil:\n {}".format(bucket_url, status.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert. Limit lines to 80 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
streamer/configuration.py
Outdated
# NOTE: ignore[override] is needed to suppress the following the mypy | ||
# error: "This violates the Liskov substitution principle, | ||
# See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides" | ||
def __eq__(self, other: 'RuntimeMap') -> bool: # type: ignore[override] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is correct. We could compare with any type, so it should stay Any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that Any
must have a _sortable_properties
method for it to be comparable with self: RuntimeMap
, so that Any
is a RuntimeMap
.
These magical methods are used to compare objects of type VideoResolution
& AudioChannelLayout
which are RuntimeMap
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is fine, but not obviously related to multiperiod support. We'd prefer this kind of unrelated cleanup separately, because it makes reviews easier and faster when a PR is focused on one thing.
streamer/controller_node.py
Outdated
if isinstance(node, TranscoderNode): | ||
status = self._nodes[i+1].check_status() | ||
elif isinstance(node, PackagerNode): | ||
status = self._nodes[i-1].check_status() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This required ordering is fragile and confusing. If you want to get a specific node or its status, I'd suggest storing the nodes in separate fields instead of in a list. Something like self._packager_node
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no more the case that there is only one transcoder and packager node, i think putting each type in a separate list will be less confusing, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is necessary. stop() is only called on the controller when we're stopping everything. There are only two cases for this:
- check_status() on controller is Finished, meaning everything is finished, so the pairs don't matter because all status are the same
- check_status() on controller is Running or Errored, meaning something failed or we shut down early, so the pairs don't matter because we want to force-kill anything still running
streamer/periodconcat_node.py
Outdated
|
||
# If all the time containers were zero, write zero seconds | ||
if self.dur == 'PT': | ||
self.dur += '0S' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, you can have 0 in these fields, so you can just have every value. Second, since we target Python3, you can use format strings for this.
return f'P{self.PY}Y{self.PM}M{self.PW}W{self.PD}DT{self.TH}H{self.TM}M{self.TS}S'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i just wanted the output to look cleaner :).
i looked up for f strings and they where supported for >= python3.6, is that ok?
streamer/periodconcat_node.py
Outdated
self.dur += '0S' | ||
|
||
def parse(self, duration: str) -> None: | ||
"""Parses duration string into different containers.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing can be done with regexes. See:
streamer/periodconcat_node.py
Outdated
duration: Optional[str] =None): | ||
"""If given a duration, it uses it to set the time containers.""" | ||
|
||
self.PY: int = Y # Years |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest full names for these. e.g. years
, months
.
streamer/periodconcat_node.py
Outdated
"""A helper class represeting the iso8601:2004 duration format.""" | ||
|
||
def __init__(self, Y=0, PM=0, W=0, D=0, H=0, TM=0, S=0, | ||
duration: Optional[str] =None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having both the string and number versions, I would suggest a static method to parse from string.
@staticmethod
def parse(value: str) -> ISO8601:
...
return ISO8601(...)
streamer/periodconcat_node.py
Outdated
from streamer.packager_node import PackagerNode | ||
from streamer.input_configuration import Input | ||
from streamer.pipeline_configuration import PipelineConfig, ManifestFormat, StreamingMode | ||
from typing import List, Tuple, Optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imports go in sections, each alphabatized. Sections go: __future__
, system, third-party, ours. So it should be:
import os
from typing import List, Tuple, Optional
from xml.etree import ElementTree
from streamer import __version__
...
streamer/cloud_node.py
Outdated
|
||
Additional output from gsutil: | ||
{}""".format(bucket_url, status.stderr) | ||
message = ("Unable to write to cloud storage URL: {}\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheModMaker
Would that be ok?, i am trying to keep the indentation because it causes visual problems in vscode and i think other text editors as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that be ok?, i am trying to keep the indentation because it causes visual problems in vscode and i think other text editors as well.
Yes, this okay. But please make non-functional or cosmetic-only changes in a separate PR.
In fact, I don't see any of the changes in this file as being related to multi-period. (At least, not obviously to me.)
You've renamed a member variable, reformatted this error message, and changed the way we read from a file while waiting to upload.
I'm not opposed to all of those changes, but they are unrelated and make the review of this PR more difficult.
config_files/input_multiperiod.yaml
Outdated
|
||
multiperiod_inputs_list: | ||
|
||
# List of inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help the reader understand the config at a glance, would you please add to this comment to say that this list of inputs is the first period? And on the second list below, add a comment saying that these inputs are for the second period?
run_end_to_end_tests.py
Outdated
@@ -189,7 +189,7 @@ def stop(): | |||
resp = createCrossOriginResponse() | |||
if controller is not None: | |||
# Check status to see if one of the processes exited. | |||
if controller.check_status() == node_base.ProcessStatus.Errored: | |||
if controller.check_status() == node_base.ProcessStatus.ERRORED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of change would be great as its own change. It would be small and easy to review.
I agree completely. Please send these non-functional or cosmetic changes separately.
streamer/cloud_node.py
Outdated
|
||
Additional output from gsutil: | ||
{}""".format(bucket_url, status.stderr) | ||
message = ("Unable to write to cloud storage URL: {}\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that be ok?, i am trying to keep the indentation because it causes visual problems in vscode and i think other text editors as well.
Yes, this okay. But please make non-functional or cosmetic-only changes in a separate PR.
In fact, I don't see any of the changes in this file as being related to multi-period. (At least, not obviously to me.)
You've renamed a member variable, reformatted this error message, and changed the way we read from a file while waiting to upload.
I'm not opposed to all of those changes, but they are unrelated and make the review of this PR more difficult.
streamer/cloud_node.py
Outdated
with open(manifest_path, 'rb') as f: | ||
contents = f.read() | ||
|
||
while (not contents and | ||
self.check_status() == ProcessStatus.Running): | ||
time.sleep(0.1) | ||
|
||
with open(manifest_path, 'rb') as f: | ||
while (self.check_status() == ProcessStatus.RUNNING): | ||
f.seek(0) | ||
contents = f.read() | ||
if contents: | ||
break | ||
time.sleep(0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, we would close and reopen the file. Now, you have it keeping the file handle open while polling for content. There is a subtle difference between these which may cause issues, depending on what Shaka Packager does with the files it's writing. It's possible to write them in a way that breaks with your changes. To demonstrate, run these at roughly the same time, in this order:
App 1
import os
import time
print('Writing!')
f = open('foo.mpd', 'w') # Creates an inode on disk that maps to this FS name
f.write('old data')
f.close()
print('Sleeping!')
time.sleep(10) # Let app 2 open the file now
print('Deleting and re-writing!')
os.unlink('foo.mpd') # Unlinks the old inode from the FS
f = open('foo.mpd', 'w') # Creates a new inode on disk mapping to the same FS name
f.write('new data')
f.close()
print('Done!')
App 2
import time
print('Sleeping!')
time.sleep(3) # Let app 1 write the file
print('Reading!')
f = open('foo.mpd', 'r') # Open the original inode
data = f.read()
while data != 'new data':
# This loop will never terminate, because you're re-reading
# the same inode that app 1 unlinked from the file system.
# The inode persists on disk until all handles are closed.
# foo.mpd, if opened again as a new handle, would reference
# a different inode and you would see the new data.
print('Trying again!')
time.sleep(1)
f.seek(0)
data = f.read()
print('Done!')
Finally, you wouldn't know this, but we're working with another contributor on a plan to remove cloud_node completely. We're replacing it with a new HTTP upload capability in Shaka Packager, combined with an authentication proxy, such that Packager can write files directly to cloud storage via HTTP without going through the filesystem, and without polling here. So changes to this class are not going to be relevant for much longer.
streamer/configuration.py
Outdated
@@ -223,7 +232,7 @@ def get_subtypes( | |||
|
|||
underlying = Field.get_underlying_type(type) | |||
if underlying is dict: | |||
return cast(Tuple[Optional[Type], Optional[Type]], args) | |||
return (args[0], args[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mypy 0.812 on my workstation as well.
But I found the difference. return args
is an error, because args is Tuple[Any, ...]
, and the return value must be a 2-tuple. So returning those two specific args seems to work without the cast.
I dug a little deeper and found that args[0]
and args[1]
are seen as Any
, and a Tuple[Any, Any]
seems to be accepted for a return type of Tuple[Optional[Type], Optional[Type]]
, presumably converting Any
to Optional[Type]
.
So that explains how it's working... but I don't see why you've made this change at all. How does it relate to the point of the PR?
streamer/periodconcat_node.py
Outdated
for node in all_nodes: | ||
if isinstance(node, TranscoderNode): | ||
self._transcoder_nodes.append(node) | ||
self._period_inputs_list.append(node._inputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's poor form to access a private member of another class. Also, I don't see this being used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought i might need it for the HLS, but i will remove it for now.
streamer/periodconcat_node.py
Outdated
"PeriodConcatNode: 'PackagerNode#{} Errored, Concatenation is stopped.'".format(i) | ||
) | ||
|
||
if(ManifestFormat.DASH in self._pipeline_config.manifest_format): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: Don't use parens unless you need to span two lines
streamer/periodconcat_node.py
Outdated
"""Concatenates multiple single-period DASH manifests into one multi-period DASH manifest.""" | ||
|
||
class ISO8601: | ||
"""A helper class represeting the iso8601:2004 duration format.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: representing
streamer/periodconcat_node.py
Outdated
# We will add the time of each period on this container to set that time later | ||
# as 'mediaPresentationDuration' for the MPD tag for VOD. | ||
# For LIVE we will use this object to determine the start time of the next period. | ||
total_time: ISO8601 = ISO8601() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do this. If you move the mediaPresentationDuration of each mpd into the Period element's duration attribute, you can omit mediaPresentationDuration from the combined mpd element. Players must be able to handle this, so you can skip all the duration parsing above.
streamer/periodconcat_node.py
Outdated
# We will start the concatenation once all the PackagerNode(s) are finished. | ||
for node in all_nodes: | ||
if isinstance(node, TranscoderNode): | ||
self._transcoder_nodes.append(node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also appears to be unused
streamer/periodconcat_node.py
Outdated
Otherwise it will raise an AssertionError.""" | ||
full_path = '' | ||
for tag in args: | ||
full_path+='{'+dash_namespace+'}'+tag+'/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: add spaces around operators
Also, a more pythonic suggestion for this:
full_path = '/'.join([ '{' + dash_namespace + '} + tag for tag in args ])
child_elem = elem.find(full_path)
Finally, are you sure you need the explicit namespace in this helper at all? It might be more resilient not to rely on a hard-coded namespace with "2011" in the name, in case a newer version of the DASH spec changes this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay , I rather be reading it from the sample dash file generated by packager.
streamer/periodconcat_node.py
Outdated
full_path = '' | ||
for tag in args: | ||
full_path+='{'+dash_namespace+'}'+tag+'/' | ||
child_elem = elem.find(full_path[:-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: remove double space after equals
streamer/periodconcat_node.py
Outdated
period.attrib['duration'] = mpd.attrib['mediaPresentationDuration'] | ||
total_time += ISO8601.parse(period.attrib['duration']) | ||
else: | ||
# For LIVE, set the start attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this only runs when all packagers have completed, I'm not convinced it can work for live at all. I'm okay with restricting it to VOD for now.
I don't think Shaka Streamer's config file approach is well-suited to a truly open-ended multi-period live stream, because a streaming service will generally want to add periods dynamically over time, rather than configuring a fixed number of them upfront.
streamer/periodconcat_node.py
Outdated
'AdaptationSet', | ||
'Representation', | ||
'SegmentTemplate'): | ||
path_to_segement = os.path.relpath(packager_node._output_dir, self._output_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you are updating the segment URLs to point into the subdirectories. You can simplify this greatly by adding BaseURL to the start of each new Period element instead. BaseURLs are resolved across the whole XML hierarchy, so you can turn this:
<Period>
<AdaptationSet>
<Representation>
<BaseURL>foo.mp4</BaseURL>
Into this:
<Period>
<BaseURL>subdir/</BaseURL>
<AdaptationSet>
<Representation>
<BaseURL>foo.mp4</BaseURL>
Now you don't need to know or modify all the places in an MPD where a URL could show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's way neater, thanks.
streamer/periodconcat_node.py
Outdated
def __init__(self, | ||
pipeline_config: PipelineConfig, | ||
all_nodes: List[NodeBase], | ||
outpud_dir: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: output_dir
streamer/periodconcat_node.py
Outdated
for i, packager_node in enumerate(self._packager_nodes): | ||
|
||
dash_file_path = os.path.join( | ||
packager_node._output_dir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make output_dir public on PackagerNode (remove the leading underscore)
streamer/controller_node.py
Outdated
for node in self._nodes: | ||
status: ProcessStatus = node.check_status() | ||
if isinstance(node, (TranscoderNode, PackagerNode, PeriodConcatNode)): | ||
values.add(status.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a suggestion:
- Pass all packager nodes to both PeriodConcatNode and CloudNode constructors.
- Add a static method to NodeBase to query the combined status of a list of nodes, based on the
max()
comprehension. - Make ControllerNode use the new static method here to compute status of all nodes.
- PeriodConcatNode can use the new static method to wait for all PackagerNodes to have a Finished status, then combine manifests/playlists, then shut down.
- If CloudNode sees Finished across PackagerNodes, it can make one additional upload pass, then shut down.
streamer/periodconcat_node.py
Outdated
|
||
def __init__(self, | ||
pipeline_config: PipelineConfig, | ||
all_nodes: List[NodeBase], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of all_nodes, could this just be packager_nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great otherwise. Thanks!
streamer/input_configuration.py
Outdated
|
||
# Because these fields are not marked as required at the class level | ||
# , we need to check ourselves that one of them is provided. | ||
if dictionary.get('inputs') is None and dictionary.get('multiperiod_inputs_list') is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses "is None", so empty lists will not be matched. This means this condition would allow:
inputs: []
I would prefer to treat this as missing fields, so let's drop use "is None" here and just use truthiness.
Combined with the above suggestion, we would require exactly one field, and that the exactly one field must be non-empty.
We should add tests for these various cases, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did want such malformed field error or other types of errors to be caught in the Base
class constructor instead, that would provide more descriptive error messages about the cause of the error.
But just found out that empty lists between all other types indeed pass the Base
class error checking.
Can we add some check in Base._check_and_convert_type
that the list passed by the user must not be empty ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding this error for the length of the user-inputted list and ran the tests, some resolution
fields were given as an empty list, after putting some dummy resolutions inside them, the tests passed.
What do you think about this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. We should check for empty lists generally. Possibly empty strings, too. But let's put that those in a separate PR.
I'm thinking of a property on Field that indicates whether empty is acceptable, defaulting to False.
tests/tests.js
Outdated
|
||
await startStreamer(inputConfigDict, pipelineConfigDict); | ||
await player.load(manifestUrl); | ||
const duration = player.seekRange().end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joeyparrish should i be using this to determine the time??
also is there any other way to check for this, like some function that tells the number of periods in the manifest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way to check the number of periods in Shaka v3. In Shaka v2, we kept periods separate, but now we combine them in the player when we parse DASH.
tests/tests.js
Outdated
}); | ||
|
||
it('fails when neither "inputs" nor "multiperiod_inputs_list" is given', async() =>{ | ||
const inputConfig = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: add a semicolon at the end of this statement
tests/tests.js
Outdated
inputConfig.multiperiod_inputs_list = [ | ||
getBasicInputConfig(), | ||
getBasicInputConfig(), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: add a semicolon at the end of this statement
tests/tests.js
Outdated
@@ -316,6 +318,38 @@ function errorTests() { | |||
field_name: 'content_id', | |||
})); | |||
}); | |||
|
|||
it('fails when both "inputs" and "multiperiod_inputs_list" are given', async() =>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: add a space between =>
and {
tests/tests.js
Outdated
})); | ||
}); | ||
|
||
it('fails when neither "inputs" nor "multiperiod_inputs_list" is given', async() =>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: add a space between =>
and {
tests/tests.js
Outdated
|
||
await startStreamer(inputConfigDict, pipelineConfigDict); | ||
await player.load(manifestUrl); | ||
const duration = player.seekRange().end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way to check the number of periods in Shaka v3. In Shaka v2, we kept periods separate, but now we combine them in the player when we parse DASH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! Just a few nits, mostly style.
tests/tests.js
Outdated
|
||
await startStreamer(inputConfigDict, pipelineConfigDict); | ||
await player.load(manifestUrl); | ||
const duration = player.getManifest().presentationTimeline.getDuration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can use video.duration
.
tests/tests.js
Outdated
|
||
// Since we processed only 0:01s, the total duration shoud be 2s. | ||
// Be more tolerant with float comparison, (D > 1.9 * length) instead of (D == 2 * length). | ||
expect(duration).toBeGreaterThan(1.9 * 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should drop the * 1
part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! Congrats!
This (manually) reverts the non-refactoring parts of commit e6152fd. For the v0.4.0 release, we will remove the multi-period DASH feature. It will then be restored and released together with multi-period HLS in v0.5.0. Change-Id: I450ca7b78d93f781d85a9561a6b747e4389d0126
This reverts commit ea9f1f2, restoring the multi-period DASH feature in anticipation of v0.5.0. Change-Id: Ib43fd6d8d3dc1a025ffd40bfbe0834951ddce169
Changes:
ConflictingFields
error raised wheninputs
andmultiperiod_inputs_list
fields are present at the same time in theinput_config.yaml
.MissingRequiredExclusiveFields
error raised when neitherinputs
field normultiperiod_inputs_list
field is present.InputConfig.__init__()
pre-checks for the presence or absence of these fields mentioned above and raises error accordingly.SinglePeriod
class added.controller._append_nodes_for_inputs_list()
performs as whatcontroller.start()
was performing,controller.start()
now callscontroller._append_nodes_for_inputs_list()
once or multiple time (based on whether the input is multiperiod or not) to append more Transcoder and Packager nodes. #change the name of this method.ProcessStatus
Running
is assigned to a greater value thanFinished
.periodconcat_node.py
added, it contains a new ThreadedNode that is appended after all the Transcoder and Packager nodes here, it checks whether all the Packager nodes have finished or not every while (3s
) in thePeriodConcatNode._thread_single_pass()
, if all of them are finished, it starts the period concatenation. For DASH,xml
module is used to parse the.mpd
files and extract the periods and duration information, for HLS we might usem3u8
.