Omit SegmentTimeline completely, with allow_approximate_segment_timel… #747

sr1990 · 2020-04-08T06:05:25Z

…ine option
Creating a PR as per discussion at #554

kqyang · 2020-04-09T05:39:05Z

packager/mpd/base/representation.cc

@@ -332,6 +332,10 @@ void Representation::AddSegmentInfo(int64_t start_time, int64_t duration) {
  const uint64_t kNoRepeat = 0;
  const int64_t adjusted_duration = AdjustDuration(duration);

+  if (segment_infos_.empty()) {
+    start_number_ = start_time / duration;


The initial start_number should propagate from the muxer, but we can worry about that later.

The duration here should be scaled_target_duration:

const int64_t scaled_target_duration = mpd_options_.mpd_params.target_segment_duration * media_info_.reference_time_scale(); start_number_ = start_time / scaled_target_duration + 1;

Also, to simplify the handling later, let's just exclude the small first segment if there is. Combined with the logic above, the code would be:

if (stream_just_started_ && segment_infos_.size() == 2) { const SegmentInfo& first_segment = segment_infos.front(); const SegmentInfo& last_segment = segment_infos.back(); if (first_segment.repeat == 0 && first_segment.duration < last_segment.duration && last_segment.repeat > 0) { segment_infos_.pop_front(); start_number_ += 1; } stream_just_started_ = false; }

kqyang · 2020-04-09T06:22:27Z

packager/mpd/base/xml/xml_node.cc

@@ -56,14 +64,15 @@ bool IsTimelineConstantDuration(const std::list<SegmentInfo>& segment_infos,
    return false;

  const SegmentInfo& first_segment = segment_infos.front();
-  if (first_segment.start_time != first_segment.duration * (start_number - 1))
+  if (!ApproximiatelyEqual(first_segment.start_time,


It may be difficult to be approximately equal with 5% error for live streaming.

I suggest just checking:

if (first_segment.start_time / first_segment.duration != start_number - 1) return false;

kqyang · 2020-04-09T06:22:52Z

packager/mpd/base/xml/xml_node.cc

    return false;

  if (segment_infos.size() == 1)
    return true;

  const SegmentInfo& last_segment = segment_infos.back();
-  if (last_segment.repeat != 0)
+  if (!(last_segment.repeat == 0 || first_segment.repeat == 0))


This change will not be needed with the initial shorter segment removed.

kqyang · 2020-04-12T23:08:09Z

packager/mpd/base/representation.cc

+      if (adjusted_duration == scaled_target_duration) {
+        start_number_ = start_time / scaled_target_duration + 1;
+      } else {
+        start_number_ = start_time / scaled_target_duration;
+      }


It should always be: start_number_ = start_time / scaled_target_duration + 1 as start number is one-based.

Thanks for the review. Yes you are right. The reason to add this was to have the calculated start segment equal to segment_index which is calculated in ChunkingHandler::OnMediaSample. For example, following is the case where start number is calculated with only start_time/scaled_target_duration for shorter duration segment:

[0412/235722:ERROR:chunking_handler.cc(103)] -> Segment Index: 5488 [0412/235724:ERROR:chunking_handler.cc(103)] -> Segment Index: 5488 [0412/235726:ERROR:chunking_handler.cc(103)] -> Segment Index: 5489 [0412/235726:ERROR:representation.cc(348)] ->: start_number: 5488

If start_number_ = start_time / scaled_target_duration +1; for segments that are smaller than the target_duration provided, won't it set the start number to the next segment?

Start number is one-based and segment index is zero-based, so start number is expected to be segment index + 1.

Also note that this is a temporary code, we'll need to get start number from muxer ultimately. We can discuss about that later when this code is approved working.

start_number is one based while segment index is zero based, so it is expected that start number = segment index + 1.

Note that this is a temporary code as I mentioned earlier. We'll need to get start number from muxer ultimately, which we can discuss later once we got this piece working.

kqyang · 2020-04-13T00:52:01Z

packager/mpd/base/xml/xml_node.cc

+  if (first_segment.duration < target_duration) {
+    if (static_cast<uint32_t>(first_segment.start_time / target_duration) !=
+        start_number)
+      return false;
+  } else {
+    if (static_cast<uint32_t>(first_segment.start_time /
+                              first_segment.duration) != start_number - 1)
+      return false;
+  }


The logic is incorrect. We'll need to handle the possible short segment here if we do not want to remove it earlier. There are a few problems with the code above. For example, the code will return false in line 76 when there is a shorter initial segment.

We can handle it properly with:

bool GetTimelineConstantDuration(const std::list<SegmentInfo>& segment_infos, uint32_t start_number, int64_t* constant_duration) { if (!FLAGS_segment_template_constant_duration) return false; DCHECK(!segment_infos.empty()); if (segment_infos.size() > 2) return false; int64_t first_segment_timestamp = segment_infos.front().start_time; if (segment_infos.size() == 1) { *constant_duration = segment_infos.front().duration; return first_segment_timestamp / *constant_duration == start_number - 1; } if (segment_infos.front().repeat != 0 && segment_infos.back().repeat() != 0) return false; int64_t last_segment_timestamp = segment_infos.back().start_time; int64_t last_segment_start_number = start_number + segment_infos.front().repeat + 1; *constant_duration = segment_infos.front().repeat == 0 ? segment_infos.back().duration : // shorter initial segment segment_infos.front().duration; // shorter ending segment return first_segment_timestamp / *constant_duration == start_number - 1 && last_segment_timestamp / *constant_duration == last_segment_start_number - 1; }

After adding

if (segment_infos.size() == 1) { *constant_duration = segment_infos.front().duration; return first_segment_timestamp / *constant_duration == start_number - 1; }

and the first segment is shorter than target segment, then won't the calculated start time in representation.cc (start_time / scaled_target_duration + 1) be different than the one that it is compared against here? Also if we calculate the start number based on smaller duration than it will be greater than one calculated in ChunkingHandler::OnMediaSample and representation.cc.

For example, the code will return false in line 76 when there is a shorter initial segment.
As per my understanding:

If smaller segment is added, then IsTimelineConstantDuration will return true at 72.

If there is a target duration segment added, then IsTimelineConstantDuration will return true at 82.

Then as soon as the target duration repeat count goes above 0, the code will pop out the first shorter segment and in this case, the method will return true at line 72 and won't reach 76.

Can you please clarify?

Then as soon as the target duration repeat count goes above 0, the code will pop out the first shorter segment and in this case, the method will return true at line 72 and won't reach 76.

The initial shorter segment will not be removed until the live window is filled up.

Let's assume a buffer depth of 10000 and target duration of 1000.
(1) Initially there is only the initial shorter segment: S1 (timestamp, duration) where timestamp = 10500, duration = 500
(2) S2 (11000, 1000)
(3) S3 (12000, 1000)
(4) S4 (13000, 1000)
...
(10) S10 (19000, 1000)
(11) S11 (20000, 1000)
(12) S12 (21000, 1000)
(13) S12 (22000, 1000)

The start number is 10500 / 1000 + 1 = 11.

segment_infos is
(1) [(timestamp, duration, repeat)], where timestamp = 10500, duration = 500, repeat = 0
(2) [(10500, 500, 0), (11000, 1000, 0)]
(3) [(10500, 500, 0), (11000, 1000, 1)]
(4) [(10500, 500, 0), (11000, 1000, 2)]
...
(10) [(10500, 500, 0), (11000, 1000, 8)]
(11) [(10500, 500, 0), (11000, 1000, 9)]
(12) [(11000, 1000, 10)]
(13) [(12000, 1000, 10)]

With your code,
(1) True, constant_duration = 1000
(2) True, constant_duration = 1000
(3) False
(4) False
...
(10) False
(11) False
(12) True, constant_duration = 1000
(13) True, constant_duration = 1000

With my proposed code,
(1) False
(2) True, constant_duration = 1000
(3) True, constant_duration = 1000
(4) True, constant_duration = 1000
...
(10) True, constant_duration = 1000
(11) True, constant_duration = 1000
(12) True, constant_duration = 1000
(13) True, constant_duration = 1000

You are right that (1) fails in my proposed code, which I intentionally left out as I don't want the code to be dependent on another parameter. If you feel strongly about it, we can also merge your solution and my solution so it returns true in all cases.

Thanks for the example. By popping out, I meant code added at representation.cc::368 to 380 to pop the first short segment if approximate_segment_timeline and target_segment_duration are set. At (3), the first shorter segment will be popped out and only [(11000, 1000, 1] will be present and True will be returned from line 72 in xml_node.cc.

After trying out the changes with flags set to --mpd_output live.mpd --allow_approximate_segment_timeline --segment_template_constant_duration --segment_duration 10 --time_shift_buffer_depth 50, I get the following values of segment_infos_:

[0413/204712:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204712:ERROR:representation.cc(175)] 0 Start Time: 1798126434 Duration: 180000 Repeat: 0 -> shorter 2 seconds segment [0413/204722:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204722:ERROR:representation.cc(175)] 0 Start Time: 1798126434 Duration: 180000 Repeat: 0 [0413/204722:ERROR:representation.cc(175)] 1 Start Time: 1798306434 Duration: 900000 Repeat: 0 [0413/204732:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204732:ERROR:representation.cc(175)] 0 Start Time: 1798306434 Duration: 900000 Repeat: 1 -> shorter segment popped out [0413/204742:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204742:ERROR:representation.cc(175)] 0 Start Time: 1798306434 Duration: 900000 Repeat: 2 [0413/204752:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204752:ERROR:representation.cc(175)] 0 Start Time: 1798306434 Duration: 900000 Repeat: 3 [0413/204802:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204802:ERROR:representation.cc(175)] 0 Start Time: 1798306434 Duration: 900000 Repeat: 4 [0413/204812:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204812:ERROR:representation.cc(175)] 0 Start Time: 1798306434 Duration: 900000 Repeat: 5 [0413/204822:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204822:ERROR:representation.cc(175)] 0 Start Time: 1799206434 Duration: 900000 Repeat: 5 [0413/204832:ERROR:representation.cc(172)] SEGMENT_INFO List: [0413/204832:ERROR:representation.cc(175)] 0 Start Time: 1800106434 Duration: 900000 Repeat: 5 -> repeat count remains 5 due to tsbd and start time increases.

Is this correct? What am I missing?

Ah, I completely missed your change representation.cc::368 to 380. Sorry about that. Yes, I think that works. How does the generated mpd looks like with this change using your testing live stream? Are both audio and video representations the same as expected?

I think we were too conservative when we check if the timestamp is equal: https://github.com/google/shaka-packager/blob/master/packager/mpd/base/representation.cc#L383.

I think we can increase the error threshold. Can you try what happens if we increase the error threshold to 0.1 in https://github.com/google/shaka-packager/blob/master/packager/mpd/base/representation.cc#L395?

@kqyang Thanks. Looks like the issue was with the source file. After adding -async 1 option while creating mpeg2ts from mp4, I am get the following mpd:

<?xml version="1.0" encoding="UTF-8"?>  <MPD xmlns="urn:mpeg:dash:schema:mpd:2011" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:mpeg:dash:schema:mpd:2011 DASH- MPD.xsd" profiles="urn:mpeg:dash:profile:isoff-live:2011" minBufferTime="PT2S" type="dynamic" publishTime="2020-04-18T02:47:11Z" availabilityStartTime=" 2020-04-18T01:48:38Z" minimumUpdatePeriod="PT5S" timeShiftBufferDepth="PT50S"> <Period id="0" start="PT0S"> <AdaptationSet id="0" contentType="audio" segmentAlignment="true"> <Representation id="0" bandwidth="130986" codecs="mp4a.40.2" mimeType="audio/mp4" audioSamplingRate="48000"> <AudioChannelConfiguration schemeIdUri="urn:mpeg:dash:23003:3:audio_channel_configuration:2011" value="2"/> <SegmentTemplate timescale="90000" initialization="live-audio.mp4" media="live-audio-$Number$.mp4" startNumber="347" duration="900000"/> </Representation> </AdaptationSet> <AdaptationSet id="1" contentType="video" width="1920" height="1080" frameRate="90000/3750" segmentAlignment="true" par="16:9"> <Representation id="1" bandwidth="2990276" codecs="avc1.4d401f" mimeType="video/mp4" sar="1:1"> <SegmentTemplate timescale="90000" initialization="live-video.mp4" media="live-video-$Number$.mp4" startNumber="347" duration="900000"/> </Representation> </AdaptationSet> </Period> </MPD>

Tired increasing the error threshold but the video frame duration is 3750, audio frame duration is 1920 and time scale is 90000. As

std::min(frame_duration_, static_cast<uint32_t>(kErrorThresholdSeconds * media_info_.reference_time_scale()));

looks like frame duration will always be selected as error threshold.

Looks like the issue was with the source file. After adding -async 1 option while creating mpeg2ts from mp4, I am get the following mpd:

Great! This looks like exactly what we want.

looks like frame duration will always be selected as error threshold.

If needed, I am fine with removing the frame duration from the calculation.

Back to the original comment, I am still not very comfortable with depending on an additional parameter in IsTimelineConstantDuration.

Also, with the shorter segment removed, it isn't really necessary to have the additional parameter.

We can keep the original code other than changing

if (first_segment.start_time != first_segment.duration * (start_number - 1))

to

if (first_segment.start_time / first_segment.duration != * (start_number - 1))

It may return false for the very first two segments, but that is fine, as it is true that the very first segment is not really compliant to the constant duration.

@kqyang I have made the changes as per above comments.

If needed, I am fine with removing the frame duration from the calculation.

Sounds good. We can remove it if the problem still persists. What can be the limit of the error threshold? Will it be +-50% as per DASH IOP "Segments should have almost equal duration. The maximum tolerance of segment duration shall be ±50% and the maximum accumulated deviation over multiple segments shall be ±50% of the signaled segment duration (i.e. the @duration). "? Also, should the player rely on sidx/traf/tdft box information to tolerate the segment duration variation?

Looking at sidx boxes in the segments generated, the subsegment_duration look consistent for both video and audio and within error threshold but sometimes seem to go way over the error threshold for audio (example: 935040).

Video subsegment duration: live-video-100.mp4 entry 0000 = reference_type=0, referenced_size=2827855, subsegment_duration=900007, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-video-101.mp4 entry 0000 = reference_type=0, referenced_size=2997414, subsegment_duration=900007, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-video-102.mp4 entry 0000 = reference_type=0, referenced_size=2714349, subsegment_duration=900007, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-video-103.mp4 entry 0000 = reference_type=0, referenced_size=2573218, subsegment_duration=900008, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-video-104.mp4 Audio subsegement_duration: entry 0000 = reference_type=0, referenced_size=163055, subsegment_duration=900480, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-audio-101.mp4 entry 0000 = reference_type=0, referenced_size=162631, subsegment_duration=898560, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-audio-102.mp4 entry 0000 = reference_type=0, referenced_size=162238, subsegment_duration=900480, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-audio-103.mp4 entry 0000 = reference_type=0, referenced_size=163030, subsegment_duration=900480, starts_with_SAP=1, SAP_type=1, SAP_delta_time=0 live-audio-104.mp4

On player side,
Exoplayer sidx parser calculates the duration based on segment_duration:
https://github.com/google/ExoPlayer/blob/7d3f54a375fac04a746ca76a8f2e1ad32c8b45b2/library/core/src/main/java/com/google/android/exoplayer2/extractor/mp4/FragmentedMp4Extractor.java#L1155

Segment Duration is retrieved at:
https://github.com/google/ExoPlayer/blob/7d3f54a375fac04a746ca76a8f2e1ad32c8b45b2/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/SegmentBase.java#L183
while loading media chunks at the given renderer position and in our case it should rely on :
((duration * C.MICROS_PER_SECOND) / timescale).
@kqyang Is my understanding correct or am I missing anything? Thanks.

kqyang · 2020-04-27T04:17:36Z

packager/mpd/base/xml/xml_node.cc

@@ -47,7 +47,8 @@ std::string RangeToString(const Range& range) {
 // Check if segments are continuous and all segments except the last one are of
 // the same duration.
 bool IsTimelineConstantDuration(const std::list<SegmentInfo>& segment_infos,
-                                uint32_t start_number) {
+                                uint32_t start_number,
+                                const double target_duration) {


There is no need to pass |target_duration| to this function with the new code.

Have made the above change. Sorry about that.

kqyang · 2020-04-27T04:18:02Z

packager/mpd/base/xml/xml_node.cc

+    if (IsTimelineConstantDuration(
+            segment_infos, start_number,
+            target_duration * media_info.reference_time_scale())) {
+      if (target_duration > 0) {
+        segment_template.SetIntegerAttribute(
+            "duration", target_duration * media_info.reference_time_scale());
+      } else {
+        segment_template.SetIntegerAttribute("duration",
+                                             segment_infos.front().duration);
+      }


Similarly here. The code does not need to change.

The same below.

kqyang

@sr1990 Good job!

Now the second part of the problem, which is also more challenging than the first part: we'll need to make sure the file name matches with the new |start_number|.

Here is a possible approach:

Define a new member in SegmentInfo: int64_t segment_index.
Fill in segment_index in ChunkingHandler
Similarly for TextChunker
Update MuxerListener::OnNewSegment to include a new parameter for segment index.
In MP4 multi-segmenter, use the segment_index from SegmentInfo to generate the file name: https://github.com/google/shaka-packager/blob/6b4a75b6bacdac6b0b9f598c365c8f84f0a7fd70/packager/media/formats/mp4/multi_segment_segmenter.cc#L164. Update https://github.com/google/shaka-packager/blob/6b4a75b6bacdac6b0b9f598c365c8f84f0a7fd70/packager/media/formats/mp4/multi_segment_segmenter.cc#L212 accordingly.
Similarly for WebM, mpeg2ts and other formats.
Update SimpleMpdNotifier::NotifyNewSegment with a new segment_index parameter. Similarly for Representation class.

There could be clean-ups here and there.

@sr1990 Let me know what you think! Thanks again for working on this feature.

kqyang · 2020-04-28T05:14:58Z

packager/mpd/base/representation_unittest.cc

@@ -799,12 +799,16 @@ TEST_P(ApproximateSegmentTimelineTest,
  if (allow_approximate_segment_timeline_) {
    expected_s_elements = base::StringPrintf(
        kSElementTemplateWithoutR, kStartTime, kScaledTargetSegmentDuration);
+    EXPECT_THAT(representation_->GetXml().get(),
+                XmlNodeEqual(SegmentTimelineTestBase::ExpectedXml(


Is "SegmentTimelineTestBase::" necessary here? Same below.

@kqyang It's a name hiding issue. So, I can either do the above or add
using SegmentTimelineTestBase::ExpectedXml;
in ApproximateSegmentTimelineTest class. Let me know what you think would be right here.

sr1990 · 2020-05-05T01:58:08Z

@kqyang Thanks for the review. I am trying out the approach that you have mentioned with the above stream and able to create segment names with correct segment index. I am looking into changes to other classes related to this and will update the PR after making those changes.

kqyang · 2020-05-05T02:05:37Z

Cool. Thanks.

sr1990 · 2020-05-13T04:39:15Z

@kqyang I have updated the PR. In cases where FinalizeSegment does not create the segment file (ts_segmenter and webm's multisegment segmenter), the file that is created gets renamed with the correct segment index in it . Other than that the code follows the steps that you have mentioned above. Please let me know what you think.

sr1990 · 2020-05-18T22:09:10Z

Hey @kqyang Any thoughts about this update? and review?

kqyang · 2020-05-18T22:12:41Z

@sr1990 Sorry, was a little bit busy recently. I'll take a look ASAP.

sr1990 · 2020-05-18T23:19:34Z

Great. Thanks.

sr1990 · 2020-05-29T18:06:29Z

@kqyang ping.

kqyang · 2020-06-01T07:12:20Z

Sorry for the delay. Reviewing right now. Will try to complete it this week.

sr1990 · 2020-06-02T04:15:28Z

Thanks!

kqyang

Thanks for the hard work. Sorry for the late review.

kqyang · 2020-06-04T05:31:22Z

packager/file/udp_file.cc

@@ -27,7 +27,7 @@
 // IP_MULTICAST_ALL has been supported since kernel version 2.6.31 but we may be
 // building on a machine that is older than that.
 #ifndef IP_MULTICAST_ALL
-#define IP_MULTICAST_ALL      49


Do you mind moving style changes to a separate CL? It will make it easier for code review.

Have reverted back most of style changes. Will try to revert more changes.

kqyang · 2020-06-04T06:18:26Z

packager/media/formats/mp2t/ts_segmenter.cc

+    // RENAME FILE:
+    std::string segment_name_segment_index =
+        GetSegmentName(muxer_options_.segment_template, next_pts_,
+                       segment_index - 1, muxer_options_.bandwidth);
+
+    if (!ts_writer_->RenameFile(segment_name_segment_index)) {
+      LOG(ERROR) << "TS Error renaming the file";
+    }


Unfortunately, renaming the file after the segment is completed won't work for "Low Latency Live Streaming" we want to achieve later.

We have two options:

(1) Store the segment data in a buffer first, and write it to the file only after the segment is completed.
- We could either store the data in a list if MediaSamples or
- Write the samples to a uint8_t byte buffer.
(2) Add a new kSegmentInfoStart stream data type and send it when the segment begins in ChunkingHandler

@kqyang, thanks for the review. For option (1), how about using BufferWriter as a member in Writer classes to store the bytes to be written in file and then in FinalizeSegment, this buffer can be written to the file? Does that make sense for both mpegts (write to buffer instead of file in WritePesToFile) and webm (MkvWriter::Write and MkvWriter::WriteFromFile)?

@kqyang Have updated the PR by reverting the file renaming and used buffer for mp2ts and webm. Please review and let me know.

kqyang · 2020-06-04T06:20:45Z

packager/media/chunking/chunking_handler.h

@@ -72,7 +72,7 @@ class ChunkingHandler : public MediaHandler {
  // Segment and subsegment duration in stream's time scale.
  int64_t segment_duration_ = 0;
  int64_t subsegment_duration_ = 0;
-
+  int64_t set_segment_index_ = 0;


There is already a variable for segment index: current_segment_index_. Why do we need another one?

current_segment_index_ is used to compare with segment index calculated at line: https://github.com/google/shaka-packager/blob/8913dbda85967b26e4968495b1d0ed70fbffe1fd/packager/media/chunking/chunking_handler.cc#L99 and
https://github.com/google/shaka-packager/blob/8913dbda85967b26e4968495b1d0ed70fbffe1fd/packager/media/chunking/chunking_handler.cc#L103
whereas
set_segment_index_ is used for keeping the segment count for file creation and needs to be incremented in onCueEvent and ChunkingHandler::OnFlushRequest (which also call EndSegmentIfStarted()). If the current_segment_index_ is used here instead of set_segment_index_, the result of comparison in https://github.com/google/shaka-packager/blob/8913dbda85967b26e4968495b1d0ed70fbffe1fd/packager/media/chunking/chunking_handler.cc#L103 will change.

Please let me know what you think.

Also I noticed that I made a mistake with isGreaterSegmentIndex method. It should use set_segment_index_ instead of current_segment_index_ (as set_segment_index_ should never be decremented). I will make this change if you think the above explanation is correct.

Good point! I am afraid that won't work though as the increment is undone by the code from line 109 to 113.

Since the cue basically resets segment index back to 0 [1], I think we can keep a record of segment count: num_segments_before_last_cue_.

In OnCueEvent function:

Status ChunkingHandler::OnCueEvent(std::shared_ptr<const CueEvent> event) { ... num_segments_before_last_cue_ = current_segment_index_ + 1; return Status::OK; }

In EndSegmentIfStarted function:

Status ChunkingHandler::EndSegmentIfStarted() const { ... segment_info->segment_index = current_segment_index_ + num_segments_before_last_cue_; return DispatchSegmentInfo(kStreamIndex, std::move(segment_info)); }

[1] See the code at line 106: timestamp - cue_offset_

ChunkingHandler::OnFlushRequest does not need to be updated as it is actually called only once in the end.

…ine option

… webm.

kqyang

Thanks. This is a partial review as I haven't looked at your changes on WebM and mp4.

I like your change on TS writer! Left a few comments, which probably applies to WebM and mp4 too.

Would you mind breaking this PR into multiple smaller PRs? Sorry I should have talked to you about it earlier as it is really difficult to review a gigantic PR. It would also take a long time to finish.

For example, one of the PR could be TS writer / TS segmenter.

kqyang · 2020-06-16T06:03:11Z

packager/media/base/media_handler_test_base.cc

@@ -249,6 +249,7 @@ std::unique_ptr<SegmentInfo> MediaHandlerTestBase::GetSegmentInfo(
  info->start_timestamp = start_timestamp;
  info->duration = duration;
  info->is_subsegment = is_subsegment;
+  info->segment_index = start_timestamp / duration + 1;


Why is there a "+1" in the end? Shouldn't segment_index be 0 based?

Thanks. This is a partial review as I haven't looked at your changes on WebM and mp4.

I like your change on TS writer! Left a few comments, which probably applies to WebM and mp4 too.

Would you mind breaking this PR into multiple smaller PRs? Sorry I should have talked to you about it earlier as it is really difficult to review a gigantic PR. It would also take a long time to finish.

For example, one of the PR could be TS writer / TS segmenter.

@kqyang
Makes sense. I will divide the pr into

ChunkHandler - for sending segment index

TS writer/ TS segmenter

Webm

MP4

WebVTT + rest

How does that sound?

SGTM. Thanks.

@kqyang Have created 2 PRs for WebM and MPEG2TS changes. Will create a PR for sending segment index and handling when these two are merged. Please review and let me know.

Great. Thanks a lot for doing that.

@kqyang Have created 1 PR that sends segment index from (chunking_handler, text_chunker) -> muxer -> segmenters to generate segment with segment name based on segment index -> MpdNotifyMuxerListener -> SimpleMpdNotifier -> Representation which sets the start number based on segment index that it receives.
Please review and let me know. Thanks.

@sr1990 Thanks. Will take a look in the next few days.

kqyang · 2020-06-16T06:24:41Z

packager/media/chunking/chunking_handler.h

@@ -72,7 +72,7 @@ class ChunkingHandler : public MediaHandler {
  // Segment and subsegment duration in stream's time scale.
  int64_t segment_duration_ = 0;
  int64_t subsegment_duration_ = 0;
-
+  int64_t set_segment_index_ = 0;


Good point! I am afraid that won't work though as the increment is undone by the code from line 109 to 113.

Since the cue basically resets segment index back to 0 [1], I think we can keep a record of segment count: num_segments_before_last_cue_.

In OnCueEvent function:

Status ChunkingHandler::OnCueEvent(std::shared_ptr<const CueEvent> event) { ... num_segments_before_last_cue_ = current_segment_index_ + 1; return Status::OK; }

In EndSegmentIfStarted function:

Status ChunkingHandler::EndSegmentIfStarted() const { ... segment_info->segment_index = current_segment_index_ + num_segments_before_last_cue_; return DispatchSegmentInfo(kStreamIndex, std::move(segment_info)); }

[1] See the code at line 106: timestamp - cue_offset_

ChunkingHandler::OnFlushRequest does not need to be updated as it is actually called only once in the end.

kqyang · 2020-06-16T06:31:09Z

packager/media/event/hls_notify_muxer_listener_unittest.cc

@@ -345,7 +345,7 @@ TEST_F(HlsNotifyMuxerListenerTest, OnNewSegmentAndCueEvent) {
                       kSegmentDuration, _, kSegmentSize));
  listener_.OnCueEvent(kCueStartTime, "dummy cue data");
  listener_.OnNewSegment("new_segment_name10.ts", kSegmentStartTime,
-                         kSegmentDuration, kSegmentSize);
+                         kSegmentDuration, kSegmentSize, 10);


Prefer using a named constant to make it easier to readers. For example, can we use kSegmentIndex? Same below

kqyang · 2020-06-16T06:34:01Z

packager/media/event/mpd_notify_muxer_listener.cc

  if (mpd_notifier_->dash_profile() == DashProfile::kLive) {
    mpd_notifier_->NotifyNewSegment(notification_id_.value(), start_time,
-                                    duration, segment_file_size);
+                                    duration, segment_file_size, segment_index);
    if (mpd_notifier_->mpd_type() == MpdType::kDynamic)
      mpd_notifier_->Flush();
  } else {


We should save segment_index in event_info.segment_info below to avoid calculating segment_index in line 156 - 159 above.

kqyang · 2020-06-16T06:37:30Z

packager/media/formats/mp2t/ts_segmenter.cc

+  if (ts_writer_buffer_initialized_) {
+    std::string segment_name_segment_index =
+        GetSegmentName(muxer_options_.segment_template, next_pts_,
+                       segment_index - 1, muxer_options_.bandwidth);


It should be segment_index instead of segment_index - 1.

Sending segment_index-1 as GetSegmentName increments the segment index by 1 to create file name. Will modify the code so that segment index that is sent is 0 based.

kqyang · 2020-06-16T06:37:50Z

packager/media/formats/mp2t/ts_segmenter.cc

-      GetSegmentName(muxer_options_.segment_template, next_pts,
-                     segment_number_++, muxer_options_.bandwidth);
-  if (!ts_writer_->NewSegment(segment_name))
+  next_pts_ = next_pts;


Can we call it segment_start_timestamp_?

kqyang · 2020-06-16T06:40:06Z

packager/media/formats/mp2t/ts_segmenter.h

@@ -70,6 +73,8 @@ class TsSegmenter {
  /// Only for testing.
  void SetTsWriterFileOpenedForTesting(bool value);

+  int next_pts_ = -1;


Class members should be private instead of protected.

kqyang · 2020-06-16T06:42:55Z

packager/media/formats/mp2t/ts_segmenter.cc

+        GetSegmentName(muxer_options_.segment_template, next_pts_,
+                       segment_index - 1, muxer_options_.bandwidth);
+
+    current_segment_path_ = segment_name_segment_index;


Does this variable still need to be a class member?

Does this variable still need to be a class member?

not needed. Will remove it.

kqyang · 2020-06-16T06:43:36Z

packager/media/formats/mp2t/ts_segmenter.cc

  // be false.
-  if (ts_writer_file_opened_) {
+  if (ts_writer_buffer_initialized_) {
+    std::string segment_name_segment_index =


I'd just call it segment_name or segment_path.

kqyang · 2020-06-16T06:44:00Z

packager/media/formats/mp2t/ts_segmenter.cc

+                       segment_index - 1, muxer_options_.bandwidth);
+
+    current_segment_path_ = segment_name_segment_index;
+    if (!ts_writer_->CreateFileAndFlushBuffer(segment_name_segment_index)) {


Can we call the function: WriteToFile?

ProIcons · 2021-06-15T07:14:46Z

bump

hiimdoublej-swag · 2023-10-13T11:04:13Z

Bump

cosmin · 2024-05-01T05:29:29Z

This needs to be rebased on top of main, and really on top of the changes from #879 as well.

cosmin · 2024-05-14T17:12:11Z

@sr1990 do you want to take another stab at this on top of current main?

sr1990 · 2024-05-18T05:27:46Z

@sr1990 do you want to take another stab at this on top of current main?

@cosmin do not have the bandwidth to look into this right now.

kqyang reviewed Apr 9, 2020

View reviewed changes

kqyang reviewed Apr 13, 2020

View reviewed changes

sr1990 force-pushed the omit_segmentTimline branch from 98ecdab to c7283e7 Compare April 21, 2020 08:14

kqyang reviewed Apr 27, 2020

View reviewed changes

sr1990 force-pushed the omit_segmentTimline branch from c7283e7 to 4ad6161 Compare April 28, 2020 02:58

kqyang reviewed Apr 28, 2020

View reviewed changes

sr1990 force-pushed the omit_segmentTimline branch from 4ad6161 to 0f703bf Compare May 12, 2020 05:40

kqyang reviewed Jun 4, 2020

View reviewed changes

sr1990 added 9 commits June 11, 2020 12:43

Omit SegmentTimeline completely, with allow_approximate_segment_timel…

e1371d8

…ine option

Changes as per comments - 1

e07db24

Changes as per comments - 2

d76d778

Changes as per comments - 3

4c6b29e

Get start number from muxer

02c8730

Removing std::max.

606fd69

Removing unwanted logs.

33f3a59

Using buffer instead of renaming file in FinalizeSegment for m2ts and…

07e1e0d

… webm.

Changing unit tests as per previous changes.

f522c01

sr1990 force-pushed the omit_segmentTimline branch from 275ae6e to f522c01 Compare June 12, 2020 06:34

Reverting back a few style changes for better review.

d5d376c

sr1990 requested a review from kqyang June 14, 2020 20:11

kqyang reviewed Jun 16, 2020

View reviewed changes

cosmin added this to the Backlog milestone May 1, 2024

cosmin marked this pull request as draft May 15, 2024 01:06

Omit SegmentTimeline completely, with allow_approximate_segment_timel… #747

Are you sure you want to change the base?

Omit SegmentTimeline completely, with allow_approximate_segment_timel… #747

Conversation

sr1990 commented Apr 8, 2020

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

Choose a reason for hiding this comment

sr1990 Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

sr1990 Apr 24, 2020 • edited Loading

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

kqyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr1990 May 15, 2020 • edited Loading

Choose a reason for hiding this comment

sr1990 commented May 5, 2020 • edited Loading

kqyang commented May 5, 2020

sr1990 commented May 13, 2020 • edited Loading

sr1990 commented May 18, 2020

kqyang commented May 18, 2020

sr1990 commented May 18, 2020

sr1990 commented May 29, 2020

kqyang commented Jun 1, 2020

sr1990 commented Jun 2, 2020

kqyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr1990 Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr1990 Jun 7, 2020 • edited Loading

Choose a reason for hiding this comment

sr1990 Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr1990 Jun 7, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProIcons commented Jun 15, 2021

hiimdoublej-swag commented Oct 13, 2023

cosmin commented May 1, 2024

cosmin commented May 14, 2024

sr1990 Apr 21, 2020 •

edited

Loading

sr1990 Apr 24, 2020 •

edited

Loading

sr1990 May 15, 2020 •

edited

Loading

sr1990 commented May 5, 2020 •

edited

Loading

sr1990 commented May 13, 2020 •

edited

Loading

sr1990 Jun 12, 2020 •

edited

Loading

sr1990 Jun 7, 2020 •

edited

Loading

sr1990 Jun 12, 2020 •

edited

Loading

sr1990 Jun 7, 2020 •

edited

Loading