feat: get start number from muxer and specify initial sequence number #879

sr1990 · 2021-01-09T03:58:08Z

@kqyang Have merged #818 and #519 by @sammirata along with the changes mentioned in #519.
Example:
../packager 'in=Sintel-1280x720.mp4,stream=video,init_segment=h264_1280p/init.mp4,segment_templa te=h264_1280p/$Number$.m4s' --mpd_output h264.mpd --mp4_initial_sequence_number 0
Please review and let me know.
Thanks.

vish91 · 2021-01-10T03:49:12Z

@sr1990 thanks for putting in PR. Initial testing looks good to me. I can try and run more variants if I get time.
Command:

./packager 'in=/home/vshah/shaka/sample.mp4,stream=video,init_segment=/tmp/shaka/dash/init.m4v,segment_template=/tmp/shaka/dash/output/sample/seg_$Number$.m4s' --segment_duration 6 --fragment_duration 6 --mp4_initial_sequence_number 0 --mpd_output /home/vshah/shaka/output/stream-sr1990.mpd

<SegmentTemplate timescale="12800" initialization="/tmp/shaka/dash/init.m4v" 
   media="/tmp/shaka/dash/output/sample/seg_$Number$.m4s" startNumber="0">

jigneshdhruv · 2021-01-13T21:48:53Z

@kqyang Do you think this PR can be merged? Our initial testing looks good. We are waiting on this patch do deliver dash manifest to ps4 devices. Thanks.

kqyang · 2021-01-26T23:21:26Z

docs/source/options/mp4_output_options.rst

@@ -17,3 +17,8 @@ MP4 output options
    template).

    Default enabled.
+
+--mp4_initial_sequence_number


Thanks for working on this feature. Here is my high level feedback.

There are two concepts in this CL.

mp4_initial_sequence_number which specifies the initial sequence number in moof->mfhd.

the starting segment index.

They are different concepts. They may not have the same value as a segment can contain multiple fragments.

Can we have a separate flag for segment index, e.g. start_segment_number to match the startNumber in DASH SegmentTemplate?

I think we can remove the initial sequence number from this PR (or move it to a separate PR).

I also think that it can be a ChunkingParam as it is not restricted to mp4.

Thanks for the feedback.
They are different concepts. They may not have the same value as a segment can contain multiple fragments.
You are right. My mistake. What is the use-case for specifying moof->mfhd sequence number?
I do not see exoplayer's FragmentedMp4Extractor.java parsing it
https://github.com/google/ExoPlayer/blob/03263db378392385e290c40505d80e990c85a5cb/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/FragmentedMp4Extractor.java#L708

Can we have a separate flag for segment index, e.g. start_segment_number to match the startNumber in DASH SegmentTemplate? I also think that it can be a ChunkingParam as it is not restricted to mp4.

Yes that makes sense. Will look into it.

What is the use-case for specifying moof->mfhd sequence number?

I don't really know. None of the players and devices I know care about the sequence number in mfhd. That is actually the question I have in the original PR (#270) too.

Yeah that makes sense.. we can keep it out. I don't know of use-cases either where it would be needed to change the initial sequence number of moof -> mfhd.
From what I understand was that segment index start should be enough to fix console platform issues and align us with the DASH spec, that should be the only validation happening player side. Beyond that it just uses either index ranges or the indexed fragmented files from origin for delivery.

sr1990 · 2021-01-31T03:13:57Z

@vish91, @kqyang, I have added a start_segment_number flag to indicate startNumber in SegmentTemplate.
Example:

../packager \                                                                   
  'in=bear-640x360.mp4,stream=video,init_segment=bear_360p/init.mp4,segment_template=bear_360p/$Number$.m4s' \
  'in=bear-640x360.mp4,stream=audio,init_segment=bear_audio/init.mp4,segment_template=bear_audio/$Number$.m4s' \
  'in=bear-english-text.vtt,stream=text,init_segment=bear_text/init.mp4,segment_template=bear_text/$Number$.m4s' \
  --mpd_output bear.mpd --generate_static_live_mpd --start_segment_number 0

Please review and let me know.

kqyang

Thanks for updating the code. It looks mostly good.

I left a number of comments, with most of them as formatting issues.

As I have suggested in your earlier PRs, you can use clang-format to format the code properly:

$ git clang-format

If HEAD^ is the original commit:

$ git clang-format HEAD^

kqyang · 2021-01-31T23:23:37Z

docs/source/options/chunking_options.rst

+
+--start_segment_number
+
+   Indicates the startNumber in DASH SegmentTemplate.


Technically it applies to HLS as well. Can we rephrase it to something like:

Indicates the startNumber in DASH SegmentTemplate and HLS segment name.

Same elsewhere.

kqyang · 2021-01-31T23:26:27Z

packager/app/packager_main.cc

+  if (chunking_params.start_segment_number < 0) {
+    LOG(ERROR) << "Negative --start_segment_number not allowed.";
+    return base::nullopt;
+  }


The check should be moved to packager.cc, e.g. in ValidateParams

kqyang · 2021-01-31T23:27:18Z

packager/app/test/packager_test.py

@@ -478,7 +478,8 @@ def _GetFlags(self,
                default_language=None,
                segment_duration=1.0,
                use_fake_clock=True,
-                allow_codec_switching=False):
+                allow_codec_switching=False,
+                start_segment_number=-1):


Use None to indicate that it is not set:

start_segment_number=None

kqyang · 2021-01-31T23:27:51Z

packager/app/test/packager_test.py

+    self.assertPackageSuccess(streams, self._GetFlags(output_dash=True,
+                                               start_segment_number=0))


Can you align it properly?

kqyang · 2021-01-31T23:30:48Z

packager/media/base/text_muxer.cc

@@ -49,7 +49,7 @@ Status TextMuxer::Finalize() {

    muxer_listener()->OnNewSegment(
        options().output_file_name, 0,
-        duration_seconds * streams()[0]->time_scale(), size);
+        duration_seconds * streams()[0]->time_scale(), size, 0);


Can you define a named constant or add a comment here for better readability?

e.g.

// The segment index does not matter for single segment output.
const uint32_t kArbitrarySegmentIndex = 0;

OnNewSegment(....,, kArbitrarySegmentIndex);

kqyang · 2021-02-01T00:17:18Z

packager/mpd/base/representation.h

-  // startNumber attribute for SegmentTemplate.
-  // Starts from 1.
-  uint32_t start_number_ = 1;
+  bool stream_just_started_ = false;


Is it part of this this PR?

Removed it.

kqyang · 2021-02-01T00:18:38Z

packager/mpd/base/representation_unittest.cc

+    if (start_segment_index_ == -1)
+      start_segment_index_ = start_time / duration;


Is it necessary?

No. Removed it.

kqyang · 2021-02-01T00:33:38Z

packager/media/chunking/chunking_handler.cc

+  segment_info->segment_index = current_segment_index_ +
+                                num_segments_before_last_cue_ +
+                                chunking_params_.start_segment_number - 1;


There is a potential behavior change.

In the original code, the segment_index starts with 0 and is always monotonically increasing.

In the new code, the segment_index is related to the segment timestamp.

Can we move the behavior change to a separate PR and focus this PR on start segment number?

So in this PR, we can just set the initial segment_index to chunking_params_.start_segment_number - 1 and increases by 1 for every new segment.

The comment applies to text_chunker below too.

Sure. Let's do that.

kqyang · 2021-02-01T00:33:59Z

packager/media/chunking/text_chunker.cc

+TextChunker::TextChunker(double segment_duration_in_seconds,
+                         int64_t start_segment_number)
+    : segment_duration_in_seconds_(segment_duration_in_seconds),
+      start_segment_number_(start_segment_number){};


needs to be reformatted

kqyang · 2021-02-01T00:35:48Z

packager/media/event/combined_muxer_listener.h

@@ -42,7 +42,8 @@ class CombinedMuxerListener : public MuxerListener {
  void OnNewSegment(const std::string& file_name,
                    int64_t start_time,
                    int64_t duration,
-                    uint64_t segment_file_size) override;
+                    uint64_t segment_file_size,
+                    int64_t segment_index) override;


Prefer segment_number so it matches with the file_name. What do you think? A number of files will need to be updated.

@kqyang Do you prefer changing segment_index in struct SegmentInfo, SegmentEventInfo, and kSegmentIndex in unit tests to kSegmentNumberl? Or just the methods with file_name parameter in it?

I think we should. There should be as few unnecessary "+1" or "-1" as possible. (If I have overlooked anything, let me know)

kqyang

Thanks.

kqyang · 2021-02-04T07:13:04Z

packager/media/formats/packed_audio/packed_audio_writer_unittest.cc

-const char kSegment2Name[] = "memory://test_2.aac";
+const char kSegment124Name[] = "memory://test_124.aac";
+const char kSegment125Name[] = "memory://test_125.aac";
+const int64_t kSegmentIndex123 = 123;


Ah, didn't notice that. It is fine to keep it as is then.

kqyang · 2021-02-04T07:17:20Z

packager/mpd/base/representation.cc

@@ -271,8 +268,11 @@ base::Optional<xml::XmlNode> Representation::GetXml() {
  }

  if (HasLiveOnlyFields(media_info_) &&
-      !representation.AddLiveOnlyInfo(media_info_, segment_infos_,
-                                      start_number_)) {
+      !representation.AddLiveOnlyInfo(


Or perhaps adding start_number only if segment_infos is not empty, e.g.

if (media_info.has_segment_template_url() && !segment_infos.empty()) {

kqyang · 2021-02-04T07:23:25Z

packager/media/event/combined_muxer_listener.h

@@ -42,7 +42,8 @@ class CombinedMuxerListener : public MuxerListener {
  void OnNewSegment(const std::string& file_name,
                    int64_t start_time,
                    int64_t duration,
-                    uint64_t segment_file_size) override;
+                    uint64_t segment_file_size,
+                    int64_t segment_index) override;


I think we should. There should be as few unnecessary "+1" or "-1" as possible. (If I have overlooked anything, let me know)

vish91 · 2021-02-10T19:56:02Z

Hey @sr1990 or @kqyang just noticed that we are also implementing HLS for this flag.
Would you know why or if that was a requirement for some scenario ?
Not worried since it's an optional flag but curious because for HLS I assumed players always look for the #EXT-X-MEDIA-SEQUENCE tag in playlist.https://tools.ietf.org/html/rfc8216#section-4.3.3.2

sr1990 · 2021-02-11T01:37:23Z

@vish91 Looks like there is already a flag to specify EXT-X-MEDIA-SEQUENCE explicitly.
bldsoft@2584ffb
Maybe we can set it's value to start_segment_number if the stream is HLS. What do you think @kqyang?

packager/media/chunking/chunking_handler.cc

packager/mpd/base/mock_mpd_notifier.h

packager/mpd/base/representation.cc

packager/mpd/base/xml/xml_node.cc

kqyang · 2021-02-16T07:47:05Z

Would you know why or if that was a requirement for some scenario ?

@vish91 Can you clarify what you mean? The current change does not affect the media sequence number.

@vish91 Looks like there is already a flag to specify EXT-X-MEDIA-SEQUENCE explicitly.
bldsoft@2584ffb
Maybe we can set it's value to start_segment_number if the stream is HLS. What do you think @kqyang?

Yes, there are overlaps. We could set the media sequence number to start_segment_number; we could also just keep them separate. I prefer having them separate for now to avoid affecting the current behavior.

vish91 · 2021-02-17T05:49:01Z

Yes, there are overlaps. We could set the media sequence number to start_segment_number; we could also just keep them separate. I prefer having them separate for now to avoid affecting the current behavior.

Yes agree. I prefer it separate as well.

Would you know why or if that was a requirement for some scenario ?
@vish91 Can you clarify what you mean? The current change does not affect the media sequence number.

I meant to ask do we even need a feature like this for HLS in first place ? since HLS uses the Media Sequence tag which already starts from 0 and isn't really affected by sequence ID on the segment name.
Or to put it in other words do we have an example of what will this flag start_segment_number do when applied on an HLS packager command ?

vish91 · 2021-03-23T02:37:03Z

@kqyang and @sr1990 please take a look when you have some time.

sr1990 · 2021-03-24T06:28:13Z

@kqyang I have updated the PR to change segment number as 1 based. Please review and let me know.

ProIcons · 2021-07-21T10:07:40Z

Looks good to me. And it would be useful when the packager restarts for any reason to "continue" from where it left off.
Also a good other solution would be the ability to define the segment offsetting in unix epoch timestamps.
While that would increase the manifest size, it would allow you in case of an interruption instead of resetting the offsets to 0 , to have them consecutively so you won't have to change periods on the manifest if you want to keep on streaming.

Vinayk93 · 2022-12-29T09:32:52Z

When can we expect to be merged.

joeyparrish · 2023-01-03T19:28:06Z

This can't be merged as-is. There are many merge conflicts, and that doesn't even account for the changes coming from the cmake branch when that work is complete.

Please be patient with us as we clean up and modernize the code, its dependencies, and its build system. Then we can work with PR authors to fix/reimplement their changes and merge them.

This reverts commit 89983c1.

cosmin · 2024-05-01T04:48:25Z

I rebased this and made a few updates and all tests seem to pass locally.

joeyparrish · 2024-05-01T21:51:25Z

packager/app/test/packager_test.py

@@ -575,6 +575,9 @@ def _GetFlags(self,
    elif force_cl_index is False:
      flags += ['--noforce_cl_index']

+    if start_segment_number:


Maybe if start_segment_number is not None: so that a start number of 0 is permitted.

packager/app/test/packager_test.py

🤖 I have created a release *beep* *boop* --- ## [3.1.0](v3.0.4...v3.1.0) (2024-05-03) ### Features * add missing DASH roles from ISO/IEC 23009-1 section 5.8.5.5 ([#1390](#1390)) ([fe885b3](fe885b3)) * get start number from muxer and specify initial sequence number ([#879](#879)) ([bb104fe](bb104fe)) * teletext formatting ([#1384](#1384)) ([4b5e80d](4b5e80d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

sr1990 mentioned this pull request Jan 9, 2021

Specify Initial Sequence Number #519

Closed

sr1990 changed the title ~~Getting start number from muxer and specify initial sequence number though option.~~ [DASH] Getting start number from muxer and specify initial sequence number though option. Jan 15, 2021

kqyang reviewed Jan 26, 2021

View reviewed changes

sr1990 force-pushed the seq_from_muxer2 branch from 89983c1 to b31c7cd Compare January 31, 2021 02:58

kqyang reviewed Feb 1, 2021

View reviewed changes

kqyang reviewed Feb 4, 2021

View reviewed changes

sr1990 force-pushed the seq_from_muxer2 branch from b31c7cd to c496378 Compare February 6, 2021 22:58

sr1990 requested a review from kqyang February 7, 2021 01:42

kqyang reviewed Feb 16, 2021

View reviewed changes

packager/media/chunking/chunking_handler.cc Outdated Show resolved Hide resolved

packager/mpd/base/mock_mpd_notifier.h Outdated Show resolved Hide resolved

packager/mpd/base/representation.cc Outdated Show resolved Hide resolved

packager/mpd/base/xml/xml_node.cc Show resolved Hide resolved

sr1990 force-pushed the seq_from_muxer2 branch from c496378 to f8c6d87 Compare March 24, 2021 06:12

sr1990 requested a review from kqyang April 5, 2021 20:20

cosmin mentioned this pull request Feb 15, 2024

[DASH] Get start number from muxer. #818

Closed

sr1990 added 6 commits April 30, 2024 18:39

[DASH] Get start number from muxer.

d79d04e

[DASH] Specify initial sequence number.

46adcc3

Revert "[DASH] Specify initial sequence number."

438aaf1

This reverts commit 89983c1.

[DASH] Indicate startNumber in SegmentTemplate.

364c3c6

Changes as per comments.

31195c1

StartNumber as 1 based.

ac64c92

fix some bad merge conflict resolutions, fix build and tests

d75752c

cosmin force-pushed the seq_from_muxer2 branch from f8c6d87 to d75752c Compare May 1, 2024 04:42

cosmin changed the title ~~[DASH] Getting start number from muxer and specify initial sequence number though option.~~ feat: get start number from muxer and specify initial sequence number though option May 1, 2024

cosmin changed the title ~~feat: get start number from muxer and specify initial sequence number though option~~ feat: get start number from muxer and specify initial sequence number May 1, 2024

cosmin requested a review from joeyparrish May 1, 2024 04:47

remove extra declaration of kAnyData

702000d

cosmin mentioned this pull request May 1, 2024

Omit SegmentTimeline completely, with allow_approximate_segment_timel… #747

Open

cosmin added this to the v3.1 milestone May 1, 2024

joeyparrish approved these changes May 1, 2024

View reviewed changes

cosmin added 2 commits May 1, 2024 16:39

allow start_segment_number of 0 in packager_test.py

69a4ad8

fix formatting for start_segment_number

43d2366

cosmin removed the request for review from kqyang May 2, 2024 20:24

cosmin merged commit bb104fe into shaka-project:main May 2, 2024
35 checks passed

shaka-bot mentioned this pull request May 2, 2024

chore(main): release 3.1.0 #1391

Merged

cosmin mentioned this pull request May 9, 2024

Specify segment start number #852

Closed


		--start_segment_number

		Indicates the startNumber in DASH SegmentTemplate.

		self.assertPackageSuccess(streams, self._GetFlags(output_dash=True,
		start_segment_number=0))

		if (start_segment_index_ == -1)
		start_segment_index_ = start_time / duration;

feat: get start number from muxer and specify initial sequence number #879

feat: get start number from muxer and specify initial sequence number #879

Conversation

sr1990 commented Jan 9, 2021 • edited

vish91 commented Jan 10, 2021

jigneshdhruv commented Jan 13, 2021

Choose a reason for hiding this comment

sr1990 Jan 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr1990 commented Jan 31, 2021

kqyang left a 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

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

sr1990 Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqyang left a 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

vish91 commented Feb 10, 2021

sr1990 commented Feb 11, 2021

kqyang commented Feb 16, 2021

vish91 commented Feb 17, 2021

vish91 commented Mar 23, 2021

sr1990 commented Mar 24, 2021

ProIcons commented Jul 21, 2021

Vinayk93 commented Dec 29, 2022

joeyparrish commented Jan 3, 2023

cosmin commented May 1, 2024

Choose a reason for hiding this comment

sr1990 commented Jan 9, 2021 •

edited

sr1990 Jan 27, 2021 •

edited

sr1990 Feb 4, 2021 •

edited