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

Multi period for HLS #83

Merged
merged 71 commits into from
Sep 3, 2021
Merged

Conversation

mariocynicys
Copy link
Member

@mariocynicys mariocynicys commented Jul 20, 2021

This PR adds the support for multi-period concatenation for HLS, using #EXT-X-DISCONTINUITY between periods.

The new file m3u8_concater.py has the classes used to parse the m3u8 files and reduce multiple files of them into one concatenated file.

HLSConcater is just an API that is exposed to the PeriodConcatNode for abstraction.

MasterPlaylist is a class representing a master hls playlist, the parsing part happens in the __init__ method, creates a MediaPlaylist object for each media(video,text,audio) playlist found in this master playlist, the stream attributes for each playlist is passed to the MediaPlaylist object and stored in it.

MediaPlaylist is a class representing an hls media playlist, the playlist gets parsed in the __init__ method, the biggest part of the playlist is saved mostly unchanged in the self.content variable, but the media segment paths get updated to point to the new relative path of the media segment.

The playlist concatenation methods are the static methods in the MediaPlaylist class (concat_sub, concat_aud, concat_vid, concat_aud_only(to handle the audio-only stream cases)).

No period concatenation is performed if an inconsistent media in periods is detected(e.g. one period has video and one does not).

Things that need addressing:
1- When Shaka Player gets an HLS master playlist containing unsupported and supported audio codecs, it breaks, this happens with:

  • audio-only playlists with and without concatenation,
  • video+audio playlists that have been concatenated, but does't happen for the un-concatenated playlists with the same set of codecs, this may have to do with the discontinuities inserted between periods.

2- When Shaka Packager receives an audio-only period to package, it normally creates a stream variant(stream-inf) for each media(audio) playlist generated, but when this audio-only input has one subtitles playlist along with the audio, Shaka-Packager doesn't create the stream-inf playlists, for the current implementation, this will create an error in the HLS concater, we can escape this period as a solution if it has no stream-infs, periods with no stream-inf we can't know its bitrates and codecs and isn't playable in Shaka Player.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

I still need to read m3u8_concater.py, but I haven't been able to make time yet. Wanted to go ahead with feedback on the other files.

Comment on lines 73 to 74
"\tperiods with other periods that have video that is for the concatenation\n"
"\tto be performed successfully.\n")
Copy link
Member

Choose a reason for hiding this comment

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

English nit: This sentence is too long. Please end this sentence after "other periods that have video." If you want to say that this is necessary for concatenation, make that a second sentence.

Comment on lines 61 to 63
# Overwrite the start and the stop methods.
setattr(self, 'start', lambda: None)
setattr(self, 'stop', lambda _=None: None)
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 overwriting these methods. How about you set a flag like self._fatal_error = True and then throw a RuntimeError at the top of _thread_single_pass if it's set?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, sure,
Can we not error when we see the flag?, i think of supporting multiple inputs even if it's not going to be concatenated.
Can we just set the status to finished in the _thread_single_pass.

Copy link
Member

Choose a reason for hiding this comment

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

We don't intend to support it in the player, so we don't intend to support that scenario here, either. I think it would be best to guide people toward more reasonable, widely-supported structures in media, until there's a compelling use-case for us to broaden support. We haven't heard one yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

What i meant is that the user can use one streamer instance to process multiple inputs, instead of instantiating multiple instances of streamer, for LIVE for example, since there is no concatenation support, i just didn't append the concater node here, so the user can push multiple live streams from the same streamer instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh i think i miss understood you, ok let's raise an error till someone requests not to.

"""Concatenates multiple HLS playlists using #EXT-X-DISCONTINUITY."""

# Initialize the HLS concater with a sample Master HLS playlist and
# the output direcotry of the concatenated playlists.
Copy link
Member

Choose a reason for hiding this comment

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

typo: directory


# Initialize the HLS concater with a sample Master HLS playlist and
# the output direcotry of the concatenated playlists.
hls_concater = HLSConcater(os.path.join(self._packager_nodes[0].output_dir,
Copy link
Member

Choose a reason for hiding this comment

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

It took me a minute to read and understand this join. Can you make it easier to read by assigning this path to a local variable with an explanatory name? Perhaps something like first_playlist_path?

self._output_dir)

for packager_node in self._packager_nodes:
hls_concater.add(os.path.join(packager_node.output_dir,
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Please assign the path to a local var for clarity.

@@ -142,13 +142,13 @@ def start(self) -> None:

args += [output_stream.ipc_pipe.write_end()]

env = {}
stderr = None
Copy link
Member Author

Choose a reason for hiding this comment

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

@joeyparrish The multiple nodes form ffmpeg overwrite TranscoderNode.log every time they try to write to it.
Here i removed the FFreport and redirected the stderr instead to a file that is open for appending.
This has a downside though, there will be no ffmpeg logs in the terminal for the sake of saving them to a file, only when debug_logs is set.

Copy link
Member

Choose a reason for hiding this comment

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

See my comment on the PackagerNode logs. Let's go back to using FFREPORT, so we get separate log verbosity for stderr (just progress) and log file (more verbose). But let's also change the file name to indicate the period.

@@ -112,7 +112,7 @@ def start(self) -> None:
# Log by writing all Packager output to a file. Unlike the logging
# system in ffmpeg, this will stop any Packager output from getting to
# the screen.
stdout = open('PackagerNode.log', 'w')
stdout = open('PackagerNode.log', 'a')
Copy link
Member

Choose a reason for hiding this comment

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

The log files could have a number added, based on the period, such as PackagerNode-1.log. That would be a nice solution to the problem of overwriting them, I think.

@@ -142,13 +142,13 @@ def start(self) -> None:

args += [output_stream.ipc_pipe.write_end()]

env = {}
stderr = None
Copy link
Member

Choose a reason for hiding this comment

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

See my comment on the PackagerNode logs. Let's go back to using FFREPORT, so we get separate log verbosity for stderr (just progress) and log file (more verbose). But let's also change the file name to indicate the period.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Just 700 lines left to review... :-)

# Save common master playlist header, this will call
# MediaPlaylist.extract_header() to save the common
# media playlist header as well.
MasterPlaylist.extract_headers(sample_master_playlist_path)
Copy link
Member

Choose a reason for hiding this comment

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

It bothers me that these are stored statically on the class. It means that an application using the Python API can't create multiple independent controllers doing concatenation if the headers would need to be different.

What if, instead, the headers (both master & media headers) are stored on the HLSConcater instance, and passed to .write()?


def write(self, dir_name: str) -> None:
file_path = os.path.join(dir_name, _unquote(self.stream_info['URI']))
with open(file_path, 'w') as media_playlist:
Copy link
Member

Choose a reason for hiding this comment

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

There are other places where "media_playlist" is a MediaPlaylist instance. Here, it's a file instance. That makes it confusing when searching the code for media_playlist.write(), because they mean two different things in these two contexts. Could you please rename this file instance to something else?

self._output_location,
packager_node))

def concat(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why concat() and write() are separate steps in this class, or why the resulting master playlist is saved to a member variable. These should be combined, I think, and the master playlist should be a local variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can do this.

var_playlists.append(media_playlist)
else:
# TODO: We need a case for CLOSED-CAPTIONS(CC).
raise RuntimeError("TYPE={} is not regonized.".format(stream_type))
Copy link
Member

Choose a reason for hiding this comment

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

typo: recognized

master_hls.playlists.extend(
MediaPlaylist.concat_sub(all_txt_playlists, durations))

if all(not MediaPlaylist.inf_is_vid(inf_pl) for inf_pl in all_var_playlists):
Copy link
Member

Choose a reason for hiding this comment

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

I'm having trouble reading this. What's "inf" short for? Variable names make a big difference in readability and maintainability.

Since "inf_pl" is an element from the list "all_var_playlists", could this fairly be called "var_playlist" instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry about that, i just renamed all the infs to vars today but overlooked this one.
I was calling a playlist that is a stream variant(#EXT-X-STREAM-INF) an inf playlist.

durations))
else:
master_hls.playlists.extend(
MediaPlaylist.concat_aud(all_aud_playlists))
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you need the duration for audio?

Copy link
Member Author

@mariocynicys mariocynicys Aug 16, 2021

Choose a reason for hiding this comment

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

For text streams we use the durations to substitute for missing text streamer(in the worst case) in some periods, but ideally we don't need the durations anywhere else other than with stream variants, because the durations are used to calculated the average bitrate of the concatenated playlist.

Comment on lines 724 to 726
outstream
.get_single_seg_file()
.write_end(): outstream
Copy link
Member

Choose a reason for hiding this comment

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

Will these fit on one line? I think I would find it easier to read that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

This does.

Comment on lines 732 to 735
outstream
.get_media_seg_file()
.write_end()
.replace('$Number$', '1'): outstream
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Does this fit on one line?

Copy link
Member Author

Choose a reason for hiding this comment

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

But this does not, can we make an exception for this :), ends at col.87

else:
# If the no playlist were found for this period,
# Create a time gap filled with dummy data for the period's duration.
dummy = ',\ndata:text/plain,NO {} SUBTITLES\n'.format(lang.upper())
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 haven't actually tested whether this renders on the screen normally or not, i have a problem with subtitles in HLS, their streams are being dropped in chrome and firefox too. Will try to fix it and report back.

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 should be an empty WEBVTT file:

data:text/vtt;charset=utf-8,WEBVTT%0A%0A

The WEBVTT header, followed by two newlines, with the correct MIME type and character set.

If you want, you could add an actual comment in the WEBVTT:

data:text/vtt;charset=utf-8,WEBVTT%0A%0ANOTE%20No%20{}%20subtitles

Or put a comment into the HLS playlist above the URI instead.

It's weird, though, and maybe hacky, to put the command and newline before the URI as part of this string. That would more logically belong to the EXTINF string.

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 it's better to leave it a blank string as you stated. Thanks!

It's weird, though, and maybe hacky, to put the comma and newline before the URI as part of this string. That would more logically belong to the EXTINF string.

Was fighting with line lengths T_T

aud_playlist_options = [codec_lang_division[codec][lang][0] for
lang in langs
if len(codec_lang_division[codec][lang])]
sub_lang = MediaPlaylist._fit_missing_lang(aud_playlist_options,
Copy link
Member Author

Choose a reason for hiding this comment

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

This substitution is not as big as the one you do in the player(based on channel count, bitrate, sample rate, etc..).
It is only based on the language only for simplicity, so if some language has no playlists at all with any channel layout, we query _fit_missing_lang() with the all the media playlists we have for that period and a language to fit upon, the method will return a best fit based on the language only.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. You should be able to count on similar channels for the same reason as similar resolutions in video: everything went through the same pipeline config.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Still reviewing concatenation of audio and video, but I've read and commented on the rest now.

self.content += ',BYTERANGE=' + attribs['BYTERANGE']
self.content += '\n'
elif line.startswith(MediaPlaylist.HEADER_TAGS + ('#EXT-X-ENDLIST',)):
# Escape header and end-list tags.
Copy link
Member

Choose a reason for hiding this comment

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

By "escape", do you mean "skip"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

# Escape header and end-list tags.
pass
elif not line.startswith('#EXT'):
# Escape comments.
Copy link
Member

Choose a reason for hiding this comment

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

Same 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.

Yup

"""Get the audio and video codecs and other relavent stream features
from the matching OutputStream in the `streams_map`, this will be used
in the codec matching process in the concat_xxx() methods, but the codecs
that will be written in the final concatenateded master playlist will be
Copy link
Member

Choose a reason for hiding this comment

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

typo: concatenated

output_stream: Optional[OutputStream] = None
lines = self.content.split('\n')
for i in range(len(lines)):
# Don't use #EXT-X-MAP to get the codec, because HLS specs says
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 think EXT-X-MAP would ever contain codec information anyway. What did you mean by this comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

def get_init_seg_file(self) -> Pipe:
    INIT_SEGMENT = {
      MediaType.AUDIO: 'audio_{language}_{channels}c_{bitrate}_{codec}_init.{format}',
      MediaType.VIDEO: 'video_{resolution_name}_{bitrate}_{codec}_init.{format}',
      MediaType.TEXT: 'text_{language}_init.{format}',
    }
    path_templ = INIT_SEGMENT[self.type].format(**self.features)
    return Pipe.create_file_pipe(path_templ, mode='w')

  def get_media_seg_file(self) -> Pipe:
    MEDIA_SEGMENT = {
      MediaType.AUDIO: 'audio_{language}_{channels}c_{bitrate}_{codec}_$Number$.{format}',
      MediaType.VIDEO: 'video_{resolution_name}_{bitrate}_{codec}_$Number$.{format}',
      MediaType.TEXT: 'text_{language}_$Number$.{format}',
    }
    path_templ = MEDIA_SEGMENT[self.type].format(**self.features)
    return Pipe.create_file_pipe(path_templ, mode='w')

  def get_single_seg_file(self) -> Pipe:
    SINGLE_SEGMENT = {
      MediaType.AUDIO: 'audio_{language}_{channels}c_{bitrate}_{codec}.{format}',
      MediaType.VIDEO: 'video_{resolution_name}_{bitrate}_{codec}.{format}',
      MediaType.TEXT: 'text_{language}.{format}',
    }
    path_templ = SINGLE_SEGMENT[self.type].format(**self.features)
    return Pipe.create_file_pipe(path_templ, mode='w')
#EXTM3U
#EXT-X-VERSION:6
## Generated with https://github.com/google/shaka-packager version c1f64e5-release
#EXT-X-TARGETDURATION:4
#EXT-X-PLAYLIST-TYPE:VOD
#EXT-X-MAP:URI="video_480p_1M_h264_init.mp4"
#EXTINF:3.000,
video_480p_1M_h264_1.mp4
#EXT-X-ENDLIST

We use the streams_map to get the matching output stream object, but when creating the streams map we only added the mapping from the methods get_single_seg_file() and get_media_seg_file(), in the segment_per_file case, we could have added get_init_seg_file() instead and checked the URI in the #EXT-X-MAP tag, this would yield to the same result, but i was skeptic about that in some cases packager might not output an #EXT-X-MAP tag since it's optional according to the HLS specs. Kept the comment as a reminder.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I see. How about this, which would make more sense to me in a comment:

# Don't use the URIs from any tag to try to extract codec information.
# We should not rely on the exact structure of file names for this.
# Use stream_maps instead.


output_stream: Optional[OutputStream] = None
lines = self.content.split('\n')
for i in range(len(lines)):
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're seeking to the first URI line in the file after EXTINF. Is that right?

This loop is so long, I think I could use a comment just before it, explaining what it's for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.
Ok, will do.

"""Returns a substitution language for a missing language by considering
the languages of the given variants.

Returns the argument `language` back only if no variants were given
Copy link
Member

Choose a reason for hiding this comment

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

What is this argument? A default? A goal/target? It's difficult to tell from the arguments, the name, or the docstring.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the argument passed to the method, it's like, if you don't provide me with variants i can choose form to replace the missing language, i will just return this missing language back to you.

This will only happen for when no text input in any language what so ever is present for some period, this means we will fill this period with the dummy subtitles for the period's duration to keep the subtitles of the next period(if there is) in sync.
This method will return the same missing language back and that will indicate that we failed to find a substitution for this language.

It's some fancy way to handle this though, i can just change it in the concat_sub method itself.

# the best fit is not the same as the base of the original language
# OR the candidate is a regional variant).
if language_base == candidate_base:
if language_base != best_fit_base or candidate_is_reg:
Copy link
Member

Choose a reason for hiding this comment

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

So any regional variant is better than ... anything else? I'm not sure I understand this clause.

Copy link
Member Author

Choose a reason for hiding this comment

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

(regional variant > base-only variant) das ist gut.

(regional variant > another regional variant with the same base) they should be equal, but the stability isn't a concern, the user should have provided stitchable subtitles in the first place >:O.

I wouldn't say it's not stable though, it is like stable from the end, so always the regional variant at the end wins the slot.

stream_name = 'stream_' + str(MediaPlaylist.current_stream_index)
MediaPlaylist.current_stream_index += 1

return _quote(stream_name), _quote(stream_name + '.m3u8')
Copy link
Member

Choose a reason for hiding this comment

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

Again I'm surprised by the tuple of quoted strings. How about a dictionary indicating what they are for? If you return a dict with attributes to add to a tag, then the quoted string values would make more sense. And you could still easily use .update() with the return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be absolutely great.

else:
# If the no playlist were found for this period,
# Create a time gap filled with dummy data for the period's duration.
dummy = ',\ndata:text/plain,NO {} SUBTITLES\n'.format(lang.upper())
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 should be an empty WEBVTT file:

data:text/vtt;charset=utf-8,WEBVTT%0A%0A

The WEBVTT header, followed by two newlines, with the correct MIME type and character set.

If you want, you could add an actual comment in the WEBVTT:

data:text/vtt;charset=utf-8,WEBVTT%0A%0ANOTE%20No%20{}%20subtitles

Or put a comment into the HLS playlist above the URI instead.

It's weird, though, and maybe hacky, to put the command and newline before the URI as part of this string. That would more logically belong to the EXTINF string.

dummy = ',\ndata:text/plain,NO {} SUBTITLES\n'.format(lang.upper())
# Break the period's duration into target duration count and remains,
# because the target duration might be a prime number, this might
# cause an accumulated rounding error.
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, BTW.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean the error?
Python generates a very long float number when doing a normal float division, i think accumulating these fractions to one #EXTINF tag might reduce the error to some extent.

Copy link
Member

Choose a reason for hiding this comment

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

I mean some small accumulated rounding error here is fine. I think you're doing the right thing, and I think it's not worth the effort to do more until we have evidence that it's necessary.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

That's everything! Thanks for being patient with me. I know it has taken me all day (my day, well into your night) to get this review completed.

# Initialize a mapping between video codecs and a list of resolutions available.
codec_division: Dict[VideoCodec, List[MediaPlaylist]] = {}
for codec in codecs:
codec_division[codec] = []
Copy link
Member

Choose a reason for hiding this comment

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

No need to change this in this PR, but just as a tip, you might consider if collections.defaultdict might be a useful tool for this pattern.

codec_division[codec].sort(key=lambda pl: pl.resolution)
for i, resolution in enumerate(sorted(resolutions)):
division[codec][resolution].append(
# Append the ith resolution if found, else, append
Copy link
Member

Choose a reason for hiding this comment

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

So you're not comparing the actual resolutions? I'm thinking about this abstractly here at first, with something like:

[360p, 720p, 1080p] + [720p, 1080p, 4k]

Going by index, you'd end up with something misaligned like:

[360p + 720p, 720p + 1080p, 1080p + 4k]

Instead of something like:

[360p + 720p, 720p + 720p, 1080p + 1080p, 1080p + 4k]

At least in Player, we have to contend with these scenarios. But I guess here, we don't, since the bottom rungs of the output from Streamer will always align since they came from the same pipeline config, and we only cut off the top rungs based on the input res. Is that accurate?

Could you explain this in the comment a little? (If I understood correctly, add to the comment that the lower resolutions will always align because the outputs shared a pipeline config, so this indexing is always safe.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, i am using this perk of the Streamer to make the matching easier, actually i had to do it similar to what you do in the Player with the audio channel matching prior to the 'channel_layout as an input feature' PR. I was starting with a dictionary full on Nones for every slot and start filling it appropriately with the channel count it should get for every period.

media_playlist.write(dir_name)
# We don't write the URI in the attributes of a stream
# variant playlist. Pop out the URI.
uri = _unquote(media_playlist.stream_info.pop('URI'))
Copy link
Member

Choose a reason for hiding this comment

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

This is really cool. I had no idea you could "pop" from a dictionary in Python.

for codec in codecs:
for lang in langs:
# If this language for this codec in this period has no media playlists
# with a for any channel layout, this means that the language itself
Copy link
Member

Choose a reason for hiding this comment

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

typo: maybe remove "with a"? Guessing at your original intent.

aud_playlist_options = [codec_lang_division[codec][lang][0] for
lang in langs
if len(codec_lang_division[codec][lang])]
sub_lang = MediaPlaylist._fit_missing_lang(aud_playlist_options,
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. You should be able to count on similar channels for the same reason as similar resolutions in video: everything went through the same pipeline config.

durations: List[float]) -> List['MediaPlaylist']:
"""Concatenates audio only periods with other audio only periods."""

# Pair audio streams with their equivalent stream variants
Copy link
Member

Choose a reason for hiding this comment

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

I honestly think it feels wrong that Packager spits out a "variant" that pairs with an audio group of itself, but I haven't spent the time digging through the spec to decide if it's required by the spec. And Packager isn't the only one, and Player already has to handle it. In any case, you've done well with this detail.

output_stream: Optional[OutputStream] = None
lines = self.content.split('\n')
for i in range(len(lines)):
# Don't use #EXT-X-MAP to get the codec, because HLS specs says
Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I see. How about this, which would make more sense to me in a comment:

# Don't use the URIs from any tag to try to extract codec information.
# We should not rely on the exact structure of file names for this.
# Use stream_maps instead.

return str(band), str(math.ceil(avg_band/sum(durations)))
return {
'BANDWIDTH': str(band),
'AVERAGE-BANDWIDTH': str(math.ceil(avg_band/sum(durations)))
Copy link
Member

Choose a reason for hiding this comment

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

Wow. I'm really surprised to check the spec and find that these values must be integers. Nice attention to detail!

I probably would have just written out a float, which would have worked fine in a JS player and may have broken some other player implementation.

dummy = ',\ndata:text/plain,NO {} SUBTITLES\n'.format(lang.upper())
# Break the period's duration into target duration count and remains,
# because the target duration might be a prime number, this might
# cause an accumulated rounding error.
Copy link
Member

Choose a reason for hiding this comment

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

I mean some small accumulated rounding error here is fine. I think you're doing the right thing, and I think it's not worth the effort to do more until we have evidence that it's necessary.

@joeyparrish
Copy link
Member

Just back from vacation. Will try to take another pass at review today if at all possible.

if remains:
concat_txt_playlist.content += '#EXTINF:' + str(remains) + ',\n'
concat_txt_playlist.content += dummy
# If the no playlist were found for this period, we create at time gap
Copy link
Member

Choose a reason for hiding this comment

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

typo: at => a

@joeyparrish joeyparrish merged commit a8ec6fc into shaka-project:master Sep 3, 2021
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants