fix: exact-order-hls #1374

SteveR-PMP · 2024-03-18T18:39:36Z

HLS manifest was still grouping by audio codecs first in the manifest before forcing the command line order.

vish91 · 2024-03-18T23:32:23Z

@joeyparrish @cosmin if you can help review this please. We found a bug with the input command based ordering feature where it was working correctly for DASH but not for HLS because of this grouping logic baked into packager only in HLS. so this rework from @SteveR-PMP should help fix that and make that feature a true command input order for both HLS and DASH.

SteveR-PMP · 2024-03-22T21:34:34Z

as an example, the following command line:

packager \ in=audio.mp4,stream=audio,hls_group_id=ec3,output=audio2.mp4 \ in=audio.mp4,stream=audio,hls_group_id=aac,output=audio3.mp4 \ in=audio.mp4,stream=audio,hls_group_id=ec3,output=audio4.mp4 \ in=audio.mp4,stream=audio,hls_group_id=aac,output=audio5.mp4 \ in=video.mp4,stream=video,output=video2.mp4 \ --hls_master_playlist_output master.m3u8

would previously generate a HLS manifest that grouped by hls_group_id first:

#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_0.m3u8",GROUP-ID="ec3",NAME="stream_0",DEFAULT=NO,AUTOSELECT=YES,CHANNELS="2"
#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_2.m3u8",GROUP-ID="ec3",NAME="stream_2",DEFAULT=NO,CHANNELS="2"
#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_1.m3u8",GROUP-ID="aac",NAME="stream_1",DEFAULT=NO,AUTOSELECT=YES,CHANNELS="2"
#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_3.m3u8",GROUP-ID="aac",NAME="stream_3",DEFAULT=NO,CHANNELS="2"

#EXT-X-STREAM-INF:BANDWIDTH=2354148,AVERAGE-BANDWIDTH=880371,CODECS="avc1.64001e,mp4a.40.2",RESOLUTION=720x300,FRAME-RATE=24.000,AUDIO="aac",CLOSED-CAPTIONS=NONE
stream_4.m3u8

#EXT-X-STREAM-INF:BANDWIDTH=2354148,AVERAGE-BANDWIDTH=880371,CODECS="avc1.64001e,mp4a.40.2",RESOLUTION=720x300,FRAME-RATE=24.000,AUDIO="ec3",CLOSED-CAPTIONS=NONE
stream_4.m3u8

and now it will force the exact command-line order:

#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_0.m3u8",GROUP-ID="ec3",NAME="stream_0",DEFAULT=NO,AUTOSELECT=YES,CHANNELS="2"
#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_1.m3u8",GROUP-ID="aac",NAME="stream_1",DEFAULT=NO,AUTOSELECT=YES,CHANNELS="2"
#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_2.m3u8",GROUP-ID="ec3",NAME="stream_2",DEFAULT=NO,CHANNELS="2"
#EXT-X-MEDIA:TYPE=AUDIO,URI="stream_3.m3u8",GROUP-ID="aac",NAME="stream_3",DEFAULT=NO,CHANNELS="2"

#EXT-X-STREAM-INF:BANDWIDTH=2354148,AVERAGE-BANDWIDTH=880371,CODECS="avc1.64001e,mp4a.40.2",RESOLUTION=720x300,FRAME-RATE=24.000,AUDIO="aac",CLOSED-CAPTIONS=NONE
stream_4.m3u8

#EXT-X-STREAM-INF:BANDWIDTH=2354148,AVERAGE-BANDWIDTH=880371,CODECS="avc1.64001e,mp4a.40.2",RESOLUTION=720x300,FRAME-RATE=24.000,AUDIO="ec3",CLOSED-CAPTIONS=NONE
stream_4.m3u8

cosmin · 2024-03-23T05:10:48Z

And the groups no longer being grouped together is what we want? On the one hand I guess it's not really strict command line ordering if we preserve the groups being grouped, on the other hand it's not clear where that would matter, as long as we respect the ordering within each group.

cosmin · 2024-03-23T05:12:56Z

packager/hls/base/master_playlist.cc

@@ -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 {


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.

cosmin · 2024-03-23T05:13:44Z

packager/hls/base/master_playlist.cc

@@ -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(


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.

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

cosmin · 2024-03-23T05:23:41Z

packager/hls/base/master_playlist.cc

  }

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


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.

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.

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

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.

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

SteveR-PMP · 2024-03-25T20:15:13Z

And the groups no longer being grouped together is what we want? On the one hand I guess it's not really strict command line ordering if we preserve the groups being grouped, on the other hand it's not clear where that would matter, as long as we respect the ordering within each group.

@cosmin
This is important because:

It helps address some problems for certain players where if we do not follow command line order a non-native primary language or an AD audio track shows up first based on current grouping and leads to playback audio track selection issues.
It should match the functionality of DASH and it avoids the confusion as to why is doesn't match the command line exactly.

If someone still wants it grouped by audio-groups first, they could manually enforce that on the command line.

SteveR-PMP added 5 commits March 14, 2024 16:28

force exact command-line ordering for HLS by breaking grouping

450a936

lint

5bc2ba4

lint

ab2736a

lint

6861ad6

Merge remote-tracking branch 'pmp/VR-4186' into exact-order-hls

a19869f

cosmin reviewed Mar 23, 2024

View reviewed changes

cosmin marked this pull request as draft May 1, 2024 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: exact-order-hls #1374

fix: exact-order-hls #1374

SteveR-PMP commented Mar 18, 2024

vish91 commented Mar 18, 2024

SteveR-PMP commented Mar 22, 2024 •

edited

cosmin commented Mar 23, 2024

cosmin Mar 23, 2024

cosmin Mar 23, 2024

cosmin Mar 23, 2024

cosmin Mar 23, 2024

SteveR-PMP Mar 25, 2024

cosmin Apr 19, 2024

vish91 May 10, 2024

cosmin May 12, 2024

SteveR-PMP commented Mar 25, 2024 •

edited

fix: exact-order-hls #1374

Are you sure you want to change the base?

fix: exact-order-hls #1374

Conversation

SteveR-PMP commented Mar 18, 2024

vish91 commented Mar 18, 2024

SteveR-PMP commented Mar 22, 2024 • edited

as an example, the following command line:

would previously generate a HLS manifest that grouped by hls_group_id first:

and now it will force the exact command-line order:

cosmin commented Mar 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveR-PMP commented Mar 25, 2024 • edited

SteveR-PMP commented Mar 22, 2024 •

edited

SteveR-PMP commented Mar 25, 2024 •

edited