feat: order streams in manifest based on command-line order #1329

SteveR-PMP · 2024-01-18T03:41:42Z

This will force the muxer to order streams in the order given on the command-line.

Closes #560
Closes #1280
Closes #1313

packager/app/manifest_flags.cc

joeyparrish

Looks like it fails to build on Windows. PTAL at the CI logs, one of which I copied here. Also, apologies that the Windows build logs are so hard to sift through.

packager/hls/base/master_playlist.cc

joeyparrish · 2024-02-09T18:04:57Z

@SteveR-PMP, if you are okay with the two edits I made, and if the builds and tests all pass, I will merge this for release in v3.0.0.

vish91 · 2024-02-09T19:35:29Z

@joeyparrish @SteveR-PMP one comment would be to change hls_params and other references initializing bool to true so that's default when the cli argument is not passed ?
Also documentation change to explicitly note that this is the default behavior and set it to false to get old random ordering behavior

vish91 · 2024-02-09T19:36:03Z

packager/app/manifest_flags.cc

+ABSL_FLAG(bool,
+          force_cl_index,
+          true,
+          "Force the muxer to order streams in the order given "


update comment

I'm thinking a mention of the old behavior should be in the release notes and not in this flag description.

vish91 · 2024-02-09T19:36:17Z

include/packager/hls_params.h

@@ -63,6 +63,9 @@ struct HlsParams {
  /// Custom EXT-X-MEDIA-SEQUENCE value to allow continuous media playback
  /// across packager restarts. See #691 for details.
  uint32_t media_sequence_number = 0;
+  /// Force the muxer to order streams in the order given
+  /// on the command-line
+  bool force_cl_index = false;


set to true by default

yes, these bool values should also be set to 'true'.

vish91 · 2024-02-09T19:36:29Z

include/packager/mpd_params.h

@@ -102,6 +102,7 @@ struct MpdParams {
  /// and is greatly influnced by the player.
  /// This parameter is required by DASH-IF Low Latency standards.
  double target_latency_seconds = 1;
+  bool force_cl_index = false;


set to true by default

vish91 · 2024-02-09T19:36:37Z

include/packager/packager.h

@@ -58,6 +58,9 @@ struct PackagingParams {
  /// Only use a single thread to generate output.  This is useful in tests to
  /// avoid non-deterministic outputs.
  bool single_threaded = false;
+  /// Force the command-line order in the output manifest(s)
+  bool force_cl_index = false;


set to true by default

joeyparrish · 2024-02-10T17:57:03Z

Looks like test outputs may need to be updated.

* added 'cl_index' to sort by command-line order * first commit include sorting group in addition to sorting streams in a group * correct sorting of HLS and DASH * change parameter orders for HlsNotifyMuxerListener * adding unit/functional tests * fix for using force_cl_index with trickplay and allow_codec_switching

* fix sorting audio groups also in HLS

* fixing unit tests for forced AdaptationSet IDs

* making sure AdaptationSets are sequential * fixing unit tests for forced AdaptationSet IDs

Fixes the shadowing of the parameter `playlists`

…f true

cosmin · 2024-02-13T17:39:39Z

There may also be some bugs here, I'm looking at the test output and seeing some cases where adaptation set ID ends up being 4294967295 which might mean there are some code paths that are not setting this properly. I'm digging into it.

joeyparrish · 2024-02-13T17:48:00Z

Thank you, @cosmin. I would welcome any changes you want to push to the fork for this PR to correct these issues before we merge it.

cosmin

The current approach doesn't seem sound. The cli_index ends up overwriting explicitly supplied IDs which is why tests are breaking. This would not work for library users. Command line concerns should remain limited to the command line code and not leak into the implementation of adaptation sets.

cosmin · 2024-02-13T17:59:31Z

I'm taking a look to see if there's a way to do this more cleanly.

cosmin · 2024-02-13T23:27:41Z

@joeyparrish take a look at the changes I pushed and see if it still looks good to you

SteveR-PMP · 2024-02-14T01:01:02Z

@cosmin The changes look good to me, except can we stay with the variable name of 'cl_index' since it is associated with the command-line option 'force_cl_index'? 'index' seems too generic of a name.

cosmin · 2024-02-14T01:04:48Z

@SteveR-PMP it should be generic, the underlying code may not be called from a command line and yet an index can be provided. The flag (on by default) is whether the command line interface will set the order, and nothing outside of the packager_main ought to look at the force_cl_index flag.

joeyparrish

LGTM % ~~test~~ build failures, e.g.:

/home/runner/work/shaka-packager/shaka-packager/packager/media/event/muxer_listener_factory.cc: In function ‘std::__cxx11::list<std::unique_ptr<shaka::media::MuxerListener> > shaka::media::{anonymous}::CreateHlsListenersInternal(const shaka::media::MuxerListenerFactory::StreamData&, int, shaka::hls::HlsNotifier*)’:
/home/runner/work/shaka-packager/shaka-packager/packager/media/event/muxer_listener_factory.cc:77:68: error: ‘const struct shaka::media::MuxerListenerFactory::StreamData’ has no member named ‘index’
   77 |                                  characteristics, notifier, stream.index));
      |                                                                    ^~~~~
/home/runner/work/shaka-packager/shaka-packager/packager/media/event/muxer_listener_factory.cc:81:54: error: ‘const struct shaka::media::MuxerListenerFactory::StreamData’ has no member named ‘index’
   81 |         std::vector<std::string>(), notifier, stream.index));

cosmin · 2024-02-14T15:39:55Z

It builds locally, this usually means there's some missing include of probably optional somewhere, I'll track it down.

@cosmin

Addressed by @cosmin

joeyparrish requested changes Feb 8, 2024

View reviewed changes

packager/app/manifest_flags.cc Outdated Show resolved Hide resolved

joeyparrish changed the title ~~feat: streams in manifest order base on command line~~ feat: order streams in manifest based on command-line order Feb 8, 2024

joeyparrish requested changes Feb 8, 2024

View reviewed changes

packager/hls/base/master_playlist.cc Outdated Show resolved Hide resolved

This was referenced Feb 8, 2024

feat: Preserve order of variants in HLS master playlist #1313

Closed

feat: Implement stream ordering in HLS playlist based on input order. #1280

Closed

chore(main): release 3.0.0 #1312

Merged

joeyparrish approved these changes Feb 9, 2024

View reviewed changes

vish91 reviewed Feb 9, 2024

View reviewed changes

joeyparrish approved these changes Feb 10, 2024

View reviewed changes

SteveR-PMP and others added 8 commits February 13, 2024 09:17

fix: sorting audio groups also in HLS (shaka-project#15)

bdfa835

* fix sorting audio groups also in HLS

* making sure AdaptationSets are sequential

994bf7f

* fixing unit tests for forced AdaptationSet IDs

fix: making sure AdaptationSets are sequential (shaka-project#14)

7067843

* making sure AdaptationSets are sequential * fixing unit tests for forced AdaptationSet IDs

lint

d082b70

Default force_cl_index to true for the CLI

d8f0616

Fix build error on Windows

107939b

Fixes the shadowing of the parameter `playlists`

the force_cl_index bool member variable needs to be set for default o…

cfcdded

…f true

cosmin previously requested changes Feb 13, 2024

View reviewed changes

cosmin added 2 commits February 13, 2024 15:05

decouple sorting concerns from CLI forcing and make the index optional

50722fb

update test files

7c0275a

cosmin force-pushed the forceorder branch from b238bc0 to 7c0275a Compare February 13, 2024 23:26

cosmin self-requested a review February 13, 2024 23:26

fix format

498a853

joeyparrish reviewed Feb 14, 2024

View reviewed changes

add missing import

acabc42

cosmin force-pushed the forceorder branch from 4ebec4e to acabc42 Compare February 14, 2024 16:51

joeyparrish approved these changes Feb 14, 2024

View reviewed changes

joeyparrish merged commit aad2a12 into shaka-project:main Feb 14, 2024
31 checks passed

github-actions bot added the status: archived Archived and locked; will not be updated label Apr 14, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: order streams in manifest based on command-line order #1329

feat: order streams in manifest based on command-line order #1329

SteveR-PMP commented Jan 18, 2024 •

edited by joeyparrish

Loading

joeyparrish left a comment

joeyparrish commented Feb 9, 2024

vish91 commented Feb 9, 2024

vish91 Feb 9, 2024

SteveR-PMP Feb 9, 2024

vish91 Feb 9, 2024

SteveR-PMP Feb 9, 2024

vish91 Feb 9, 2024

vish91 Feb 9, 2024

joeyparrish commented Feb 10, 2024

cosmin commented Feb 13, 2024

joeyparrish commented Feb 13, 2024

cosmin left a comment

cosmin commented Feb 13, 2024

cosmin commented Feb 13, 2024

SteveR-PMP commented Feb 14, 2024 •

edited

Loading

cosmin commented Feb 14, 2024

joeyparrish left a comment •

edited

Loading

cosmin commented Feb 14, 2024

feat: order streams in manifest based on command-line order #1329

feat: order streams in manifest based on command-line order #1329

Conversation

SteveR-PMP commented Jan 18, 2024 • edited by joeyparrish Loading

joeyparrish left a comment

Choose a reason for hiding this comment

joeyparrish commented Feb 9, 2024

vish91 commented Feb 9, 2024

vish91 Feb 9, 2024

Choose a reason for hiding this comment

SteveR-PMP Feb 9, 2024

Choose a reason for hiding this comment

vish91 Feb 9, 2024

Choose a reason for hiding this comment

SteveR-PMP Feb 9, 2024

Choose a reason for hiding this comment

vish91 Feb 9, 2024

Choose a reason for hiding this comment

vish91 Feb 9, 2024

Choose a reason for hiding this comment

joeyparrish commented Feb 10, 2024

cosmin commented Feb 13, 2024

joeyparrish commented Feb 13, 2024

cosmin left a comment

Choose a reason for hiding this comment

cosmin commented Feb 13, 2024

cosmin commented Feb 13, 2024

SteveR-PMP commented Feb 14, 2024 • edited Loading

cosmin commented Feb 14, 2024

joeyparrish left a comment • edited Loading

Choose a reason for hiding this comment

cosmin commented Feb 14, 2024

SteveR-PMP commented Jan 18, 2024 •

edited by joeyparrish

Loading

SteveR-PMP commented Feb 14, 2024 •

edited

Loading

joeyparrish left a comment •

edited

Loading