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.

Sign up for GitHub

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

fix: exact-order-hls #1374

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

fix: exact-order-hls #1374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
450a936
force exact command-line ordering for HLS by breaking grouping
SteveR-PMP Mar 14, 2024
5bc2ba4
lint
SteveR-PMP Mar 15, 2024
ab2736a
lint
SteveR-PMP Mar 15, 2024
6861ad6
lint
SteveR-PMP Mar 15, 2024
a19869f
Merge remote-tracking branch 'pmp/VR-4186' into exact-order-hls
SteveR-PMP Mar 18, 2024
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
83 changes: 46 additions & 37 deletions packager/hls/base/master_playlist.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ struct Variant {
uint64_t avg_audio_bitrate = 0;
};

// This structure is used to store the playlist and its tags.
struct MediaTagslist {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something about the name MediaTagslist is too visually close to MediaPlaylist to the point where I had to read the code closely several times to make sure I was reading it correctly. Perhaps we can find a better name.

const MediaPlaylist* playlist;
std::string group_id;
bool is_default;
bool is_autoselect;
};

uint64_t GetMaximumMaxBitrate(const std::list<const MediaPlaylist*> playlists) {
uint64_t max = 0;
for (const auto& playlist : playlists) {
Expand Down Expand Up @@ -364,11 +372,12 @@ void BuildMediaTag(const MediaPlaylist& playlist,
out->append("\n");
}

void BuildMediaTags(
std::list<std::pair<std::string, std::list<const MediaPlaylist*>>>& groups,
std::list<MediaTagslist> BuildMediaTags(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit confusing, before this change BuildMediaTags called BuildMediaTag within, but now it no longer does that. We probably need to rename this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternatively keep this function as is, giving it a list of of MediaTagslist or whatever we rename that to, and instead build the right struct (with the group name, etc) in the main loop as suggested in another comment

const std::map<std::string, std::list<const MediaPlaylist*>>& groups,
const std::string& default_language,
const std::string& base_url,
std::string* out) {
const std::string& base_url) {
std::list<MediaTagslist> mediaTag_playlist;

for (const auto& group : groups) {
const std::string& group_id = group.first;
const auto& playlists = group.second;
Expand Down Expand Up @@ -412,22 +421,25 @@ void BuildMediaTags(
is_autoselect = true;
}

BuildMediaTag(*playlist, group_id, is_default, is_autoselect, base_url,
out);
MediaTagslist mediaTagslist;
mediaTagslist.playlist = playlist;
mediaTagslist.group_id = group_id;
mediaTagslist.is_default = is_default;
mediaTagslist.is_autoselect = is_autoselect;
mediaTag_playlist.push_back(mediaTagslist);
}
}

return mediaTag_playlist;
}

bool ListOrderFn(const MediaPlaylist*& a, const MediaPlaylist*& b) {
bool PlaylistOrderFn(const MediaPlaylist*& a, const MediaPlaylist*& b) {
return a->GetMediaInfo().index() < b->GetMediaInfo().index();
}

bool GroupOrderFn(std::pair<std::string, std::list<const MediaPlaylist*>>& a,
std::pair<std::string, std::list<const MediaPlaylist*>>& b) {
a.second.sort(ListOrderFn);
b.second.sort(ListOrderFn);
return a.second.front()->GetMediaInfo().index() <
b.second.front()->GetMediaInfo().index();
bool TagslistOrderFn(const MediaTagslist& a, const MediaTagslist& b) {
return a.playlist->GetMediaInfo().index() <
b.playlist->GetMediaInfo().index();
}

void AppendPlaylists(const std::string& default_audio_language,
Expand Down Expand Up @@ -465,38 +477,35 @@ void AppendPlaylists(const std::string& default_audio_language,
}
}

// convert the std::map to std::list and reorder it if indexes were provided
std::list<std::pair<std::string, std::list<const MediaPlaylist*>>>
audio_groups_list(audio_playlist_groups.begin(),
audio_playlist_groups.end());
std::list<std::pair<std::string, std::list<const MediaPlaylist*>>>
subtitle_groups_list(subtitle_playlist_groups.begin(),
subtitle_playlist_groups.end());
if (has_index) {
audio_groups_list.sort(GroupOrderFn);
for (const auto& group : audio_groups_list) {
std::list<const MediaPlaylist*> group_playlists = group.second;
group_playlists.sort(ListOrderFn);
}
subtitle_groups_list.sort(GroupOrderFn);
for (const auto& group : subtitle_groups_list) {
std::list<const MediaPlaylist*> group_playlists = group.second;
group_playlists.sort(ListOrderFn);
}
video_playlists.sort(ListOrderFn);
iframe_playlists.sort(ListOrderFn);
video_playlists.sort(PlaylistOrderFn);
iframe_playlists.sort(PlaylistOrderFn);
}

if (!audio_playlist_groups.empty()) {
content->append("\n");
BuildMediaTags(audio_groups_list, default_audio_language, base_url,
content);
std::list<MediaTagslist> audio_playlists =
Copy link
Contributor

Choose a reason for hiding this comment

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

above we group playlists into group using audio_playlist_groups[GetGroupId(*playlist)].push_back(playlist); so that the groups can be sorted, but now we're no longer doing that, instead flattening the group and sorting that. We should probably insert the right structs into a list in that first for (const MediaPlaylist* playlist : playlists) { loop then we can sort each list, and no longer keep things grouped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting removing std::map<std::string, std::list<const MediaPlaylist*>> audio_playlist_groups and replacing it with a new std::list<MediaTagslist> audio_playlists? The audio_playlist_groups is still used by BuildVariants(), so that and BuildMediaTags() would need to be refactored.

I initially started down that route but felt that the code in BuildMediaTags() was easier to follow when grouped first. I could re-address this if you think otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SteveR-PMP ah yes, BuildVariants() still operates on groups. It still feels that using std::list<MediaTagslist> audio_playlists, refactoring BuildMediaTags() to use this (and call BuildMediaTag() within as before) and deferring any necessary grouping to BuildVariants() would be cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to do this refactor now @cosmin ? me or @SteveR-PMP haven't really given this a try yet and test again the combinations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vish91 I do think this needs a refactoring, one way or another.

BuildMediaTags(audio_playlist_groups, default_audio_language, base_url);
if (has_index) {
audio_playlists.sort(TagslistOrderFn);
}
for (const auto& pl : audio_playlists) {
BuildMediaTag(*pl.playlist, pl.group_id, pl.is_default, pl.is_autoselect,
base_url, content);
}
}

if (!subtitle_playlist_groups.empty()) {
content->append("\n");
BuildMediaTags(subtitle_groups_list, default_text_language, base_url,
content);
std::list<MediaTagslist> subtitle_playlists = BuildMediaTags(
subtitle_playlist_groups, default_text_language, base_url);
if (has_index) {
subtitle_playlists.sort(TagslistOrderFn);
}
for (const auto& pl : subtitle_playlists) {
BuildMediaTag(*pl.playlist, pl.group_id, pl.is_default, pl.is_autoselect,
base_url, content);
}
}

std::list<Variant> variants =
Expand All @@ -522,7 +531,7 @@ void AppendPlaylists(const std::string& default_audio_language,
if (!audio_playlist_groups.empty() && video_playlists.empty() &&
subtitle_playlist_groups.empty()) {
content->append("\n");
for (const auto& playlist_group : audio_groups_list) {
for (const auto& playlist_group : audio_playlist_groups) {
Variant variant;
// Populate |audio_group_id|, which will be propagated to "AUDIO" field.
// Leaving other fields, e.g. xxx_audio_bitrate in |Variant|, as
Expand Down
3 changes: 2 additions & 1 deletion packager/media/formats/mp2t/es_parser_teletext.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ bool EsParserTeletext::ParseDataBlock(const int64_t pts,
}

void EsParserTeletext::UpdateCharset() {
memcpy(current_charset_, TELETEXT_CHARSET_G0_LATIN, sizeof(TELETEXT_CHARSET_G0_LATIN));
memcpy(current_charset_, TELETEXT_CHARSET_G0_LATIN,
sizeof(TELETEXT_CHARSET_G0_LATIN));
if (charset_code_ > 7) {
return;
}
Expand Down
Loading