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

Jump to bottom

Omit SegmentTimeline completely, with allow_approximate_segment_timel… #747

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

Omit SegmentTimeline completely, with allow_approximate_segment_timel… #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e1371d8
Omit SegmentTimeline completely, with allow_approximate_segment_timel…
sr1990 Apr 8, 2020
e07db24
Changes as per comments - 1
sr1990 Apr 12, 2020
d76d778
Changes as per comments - 2
sr1990 Apr 21, 2020
4c6b29e
Changes as per comments - 3
sr1990 Apr 28, 2020
02c8730
Get start number from muxer
sr1990 May 12, 2020
606fd69
Removing std::max.
sr1990 May 12, 2020
33f3a59
Removing unwanted logs.
sr1990 May 12, 2020
07e1e0d
Using buffer instead of renaming file in FinalizeSegment for m2ts and…
sr1990 Jun 11, 2020
f522c01
Changing unit tests as per previous changes.
sr1990 Jun 12, 2020
d5d376c
Reverting back a few style changes for better review.
sr1990 Jun 12, 2020
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
1 change: 1 addition & 0 deletions packager/media/base/media_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ struct SegmentInfo {
bool is_encrypted = false;
int64_t start_timestamp = -1;
int64_t duration = 0;
int64_t segment_index = 0;
// This is only available if key rotation is enabled. Note that we may have
// a |key_rotation_encryption_config| even if the segment is not encrypted,
// which is the case for clear lead.
Expand Down
1 change: 1 addition & 0 deletions packager/media/base/media_handler_test_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. ChunkHandler - for sending segment index
  2. TS writer/ TS segmenter
  3. Webm
  4. MP4
  5. WebVTT + rest

How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. Thanks a lot for doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


return info;
}
Expand Down
13 changes: 12 additions & 1 deletion packager/media/chunking/chunking_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ bool IsNewSegmentIndex(int64_t new_index, int64_t current_index) {
new_index != current_index - 1;
}

bool isGreaterSegmentIndex(int64_t new_index, int64_t current_index) {
return new_index > current_index;
}

} // namespace

ChunkingHandler::ChunkingHandler(const ChunkingParams& chunking_params)
Expand Down Expand Up @@ -60,6 +64,7 @@ Status ChunkingHandler::Process(std::unique_ptr<StreamData> stream_data) {
}

Status ChunkingHandler::OnFlushRequest(size_t input_stream_index) {
set_segment_index_++;
RETURN_IF_ERROR(EndSegmentIfStarted());
return FlushDownstream(kStreamIndex);
}
Expand All @@ -74,6 +79,7 @@ Status ChunkingHandler::OnStreamInfo(std::shared_ptr<const StreamInfo> info) {
}

Status ChunkingHandler::OnCueEvent(std::shared_ptr<const CueEvent> event) {
set_segment_index_++;
RETURN_IF_ERROR(EndSegmentIfStarted());
const double event_time_in_seconds = event->time_in_seconds;
RETURN_IF_ERROR(DispatchCueEvent(kStreamIndex, std::move(event)));
Expand All @@ -89,7 +95,6 @@ Status ChunkingHandler::OnCueEvent(std::shared_ptr<const CueEvent> event) {
Status ChunkingHandler::OnMediaSample(
std::shared_ptr<const MediaSample> sample) {
DCHECK_NE(time_scale_, 0u) << "kStreamInfo should arrive before kMediaSample";

const int64_t timestamp = sample->pts();

bool started_new_segment = false;
Expand All @@ -101,6 +106,11 @@ Status ChunkingHandler::OnMediaSample(
: (timestamp - cue_offset_) / segment_duration_;
if (!segment_start_time_ ||
IsNewSegmentIndex(segment_index, current_segment_index_)) {
if (!isGreaterSegmentIndex(segment_index, set_segment_index_)) {
set_segment_index_ = current_segment_index_ + 1;
} else {
set_segment_index_ = segment_index;
}
current_segment_index_ = segment_index;
// Reset subsegment index.
current_subsegment_index_ = 0;
Expand Down Expand Up @@ -151,6 +161,7 @@ Status ChunkingHandler::EndSegmentIfStarted() const {
auto segment_info = std::make_shared<SegmentInfo>();
segment_info->start_timestamp = segment_start_time_.value();
segment_info->duration = max_segment_time_ - segment_start_time_.value();
segment_info->segment_index = set_segment_index_;
return DispatchSegmentInfo(kStreamIndex, std::move(segment_info));
}

Expand Down
2 changes: 1 addition & 1 deletion packager/media/chunking/chunking_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sr1990 sr1990 Jun 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// Current segment index, useful to determine where to do chunking.
int64_t current_segment_index_ = -1;
// Current subsegment index, useful to determine where to do chunking.
Expand Down
5 changes: 5 additions & 0 deletions packager/media/chunking/text_chunker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Status TextChunker::OnFlushRequest(size_t input_stream_index) {
// Keep outputting segments until all the samples leave the system. Calling
// |DispatchSegment| will remove samples over time.
while (samples_in_current_segment_.size()) {
segment_index_++;
RETURN_IF_ERROR(DispatchSegment(segment_duration_));
}

Expand Down Expand Up @@ -107,6 +108,10 @@ Status TextChunker::DispatchSegment(int64_t duration) {
std::shared_ptr<SegmentInfo> info = std::make_shared<SegmentInfo>();
info->start_timestamp = segment_start_;
info->duration = duration;
if (((segment_start_ / segment_duration_) + 1) > segment_index_) {
segment_index_ = (segment_start_ / segment_duration_) + 1;
}
info->segment_index = segment_index_;
RETURN_IF_ERROR(DispatchSegmentInfo(kStreamIndex, std::move(info)));

// Move onto the next segment.
Expand Down
1 change: 1 addition & 0 deletions packager/media/chunking/text_chunker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class TextChunker : public MediaHandler {
int64_t segment_start_ = -1; // Set when the first sample comes in.
int64_t segment_duration_ = -1; // Set in OnStreamInfo.

int64_t segment_index_ = 0;
// All samples that make up the current segment. We must store the samples
// until the segment ends because a cue event may end the segment sooner
// than we expected.
Expand Down
6 changes: 4 additions & 2 deletions packager/media/event/combined_muxer_listener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,11 @@ void CombinedMuxerListener::OnMediaEnd(const MediaRanges& media_ranges,
void CombinedMuxerListener::OnNewSegment(const std::string& file_name,
int64_t start_time,
int64_t duration,
uint64_t segment_file_size) {
uint64_t segment_file_size,
uint64_t segment_index) {
for (auto& listener : muxer_listeners_) {
listener->OnNewSegment(file_name, start_time, duration, segment_file_size);
listener->OnNewSegment(file_name, start_time, duration, segment_file_size,
segment_index);
}
}

Expand Down
3 changes: 2 additions & 1 deletion packager/media/event/combined_muxer_listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
uint64_t index_segment) override;
void OnKeyFrame(int64_t timestamp, uint64_t start_byte_offset, uint64_t size);
void OnCueEvent(int64_t timestamp, const std::string& cue_data) override;
/// @}
Expand Down
3 changes: 2 additions & 1 deletion packager/media/event/hls_notify_muxer_listener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,8 @@ void HlsNotifyMuxerListener::OnMediaEnd(const MediaRanges& media_ranges,
void HlsNotifyMuxerListener::OnNewSegment(const std::string& file_name,
int64_t start_time,
int64_t duration,
uint64_t segment_file_size) {
uint64_t segment_file_size,
uint64_t segment_index) {
if (!media_info_->has_segment_template()) {
EventInfo event_info;
event_info.type = EventInfoType::kSegment;
Expand Down
3 changes: 2 additions & 1 deletion packager/media/event/hls_notify_muxer_listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ class HlsNotifyMuxerListener : 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,
uint64_t segment_index) override;
void OnKeyFrame(int64_t timestamp, uint64_t start_byte_offset, uint64_t size);
void OnCueEvent(int64_t timestamp, const std::string& cue_data) override;
/// @}
Expand Down
12 changes: 6 additions & 6 deletions packager/media/event/hls_notify_muxer_listener_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// Verify that the notifier is called for every segment in OnMediaEnd if
Expand All @@ -363,7 +363,7 @@ TEST_F(HlsNotifyMuxerListenerTest, NoSegmentTemplateOnMediaEnd) {

listener_.OnCueEvent(kCueStartTime, "dummy cue data");
listener_.OnNewSegment("filename.mp4", kSegmentStartTime, kSegmentDuration,
kSegmentSize);
kSegmentSize, 0);

EXPECT_CALL(mock_notifier_, NotifyCueEvent(_, kCueStartTime));
EXPECT_CALL(
Expand Down Expand Up @@ -393,7 +393,7 @@ TEST_F(HlsNotifyMuxerListenerTest, NoSegmentTemplateOnMediaEndTwice) {
listener_.OnMediaStart(muxer_options1, *video_stream_info, 90000,
MuxerListener::kContainerMpeg2ts);
listener_.OnNewSegment("filename1.mp4", kSegmentStartTime, kSegmentDuration,
kSegmentSize);
kSegmentSize, 1);
listener_.OnCueEvent(kCueStartTime, "dummy cue data");

EXPECT_CALL(mock_notifier_, NotifyNewStream(_, _, _, _, _))
Expand All @@ -410,7 +410,7 @@ TEST_F(HlsNotifyMuxerListenerTest, NoSegmentTemplateOnMediaEndTwice) {
listener_.OnMediaStart(muxer_options2, *video_stream_info, 90000,
MuxerListener::kContainerMpeg2ts);
listener_.OnNewSegment("filename2.mp4", kSegmentStartTime + kSegmentDuration,
kSegmentDuration, kSegmentSize);
kSegmentDuration, kSegmentSize, 2);
EXPECT_CALL(mock_notifier_,
NotifyNewSegment(_, StrEq("filename2.mp4"),
kSegmentStartTime + kSegmentDuration, _, _, _));
Expand All @@ -436,7 +436,7 @@ TEST_F(HlsNotifyMuxerListenerTest,
MuxerListener::kContainerMpeg2ts);

listener_.OnNewSegment("filename.mp4", kSegmentStartTime, kSegmentDuration,
kSegmentSize);
kSegmentSize, 0);
EXPECT_CALL(
mock_notifier_,
NotifyNewSegment(_, StrEq("filename.mp4"), kSegmentStartTime,
Expand Down Expand Up @@ -498,7 +498,7 @@ TEST_P(HlsNotifyMuxerListenerKeyFrameTest, NoSegmentTemplate) {
listener_.OnKeyFrame(kKeyFrameTimestamp, kKeyFrameStartByteOffset,
kKeyFrameSize);
listener_.OnNewSegment("filename.mp4", kSegmentStartTime, kSegmentDuration,
kSegmentSize);
kSegmentSize, 0);

EXPECT_CALL(mock_notifier_,
NotifyKeyFrame(_, kKeyFrameTimestamp,
Expand Down
8 changes: 4 additions & 4 deletions packager/media/event/mock_muxer_listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ class MockMuxerListener : public MuxerListener {

// Windows 32 bit cannot mock MediaRanges because it has Optionals that use
// memory alignment of 8 bytes. The compiler fails if it is mocked.
void OnMediaEnd(const MediaRanges& range,
float duration_seconds) override;
void OnMediaEnd(const MediaRanges& range, float duration_seconds) override;

MOCK_METHOD4(OnNewSegment,
MOCK_METHOD5(OnNewSegment,
void(const std::string& segment_name,
int64_t start_time,
int64_t duration,
uint64_t segment_file_size));
uint64_t segment_file_size,
uint64_t segment_index));

MOCK_METHOD3(OnKeyFrame,
void(int64_t timestamp,
Expand Down
32 changes: 16 additions & 16 deletions packager/media/event/mpd_notify_muxer_listener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,13 @@ void MpdNotifyMuxerListener::OnEncryptionInfoReady(

void MpdNotifyMuxerListener::OnEncryptionStart() {}

void MpdNotifyMuxerListener::OnMediaStart(
const MuxerOptions& muxer_options,
const StreamInfo& stream_info,
uint32_t time_scale,
ContainerType container_type) {
void MpdNotifyMuxerListener::OnMediaStart(const MuxerOptions& muxer_options,
const StreamInfo& stream_info,
uint32_t time_scale,
ContainerType container_type) {
std::unique_ptr<MediaInfo> media_info(new MediaInfo());
if (!internal::GenerateMediaInfo(muxer_options,
stream_info,
time_scale,
container_type,
media_info.get())) {
if (!internal::GenerateMediaInfo(muxer_options, stream_info, time_scale,
container_type, media_info.get())) {
LOG(ERROR) << "Failed to generate MediaInfo from input.";
return;
}
Expand All @@ -79,7 +75,8 @@ void MpdNotifyMuxerListener::OnMediaStart(
if (is_encrypted_) {
internal::SetContentProtectionFields(protection_scheme_, default_key_id_,
key_system_info_, media_info.get());
media_info->mutable_protected_content()->set_include_mspr_pro(mpd_notifier_->include_mspr_pro());
media_info->mutable_protected_content()->set_include_mspr_pro(
mpd_notifier_->include_mspr_pro());
}

// The content may be splitted into multiple files, but their MediaInfo
Expand All @@ -102,8 +99,7 @@ void MpdNotifyMuxerListener::OnMediaStart(

// Record the sample duration in the media info for VOD so that OnMediaEnd, all
// the information is in the media info.
void MpdNotifyMuxerListener::OnSampleDurationReady(
uint32_t sample_duration) {
void MpdNotifyMuxerListener::OnSampleDurationReady(uint32_t sample_duration) {
if (mpd_notifier_->dash_profile() == DashProfile::kLive) {
mpd_notifier_->NotifySampleDuration(notification_id_.value(),
sample_duration);
Expand Down Expand Up @@ -157,7 +153,10 @@ void MpdNotifyMuxerListener::OnMediaEnd(const MediaRanges& media_ranges,
mpd_notifier_->NotifyNewSegment(
notification_id_.value(), event_info.segment_info.start_time,
event_info.segment_info.duration,
event_info.segment_info.segment_file_size);
event_info.segment_info.segment_file_size,
(event_info.segment_info.start_time /
event_info.segment_info.duration) +
1);
break;
case EventInfoType::kKeyFrame:
// NO-OP for DASH.
Expand All @@ -175,10 +174,11 @@ void MpdNotifyMuxerListener::OnMediaEnd(const MediaRanges& media_ranges,
void MpdNotifyMuxerListener::OnNewSegment(const std::string& file_name,
int64_t start_time,
int64_t duration,
uint64_t segment_file_size) {
uint64_t segment_file_size,
uint64_t segment_index) {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand Down
3 changes: 2 additions & 1 deletion packager/media/event/mpd_notify_muxer_listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class MpdNotifyMuxerListener : 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,
uint64_t segment_index) override;
void OnKeyFrame(int64_t timestamp, uint64_t start_byte_offset, uint64_t size);
void OnCueEvent(int64_t timestamp, const std::string& cue_data) override;
/// @}
Expand Down
Loading