-
Notifications
You must be signed in to change notification settings - Fork 491
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.
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
[DASH] Get start number from muxer. #818
Conversation
Have not run clang-format on all files yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Mostly LGTM with some style nits.
@@ -144,13 +147,17 @@ Status ChunkingHandler::OnMediaSample( | |||
return DispatchMediaSample(kStreamIndex, std::move(sample)); | |||
} | |||
|
|||
Status ChunkingHandler::EndSegmentIfStarted() const { | |||
Status ChunkingHandler::EndSegmentIfStarted(bool fromCueEvent) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_cue_event
// If fromCueEvent then current_segment_index_ is already added to the | ||
// number of segments before last cue. | ||
segment_info->segment_index = ((fromCueEvent) ? 0 : current_segment_index_) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simpler way to handle it is to set current_segment_index_ to 0 in line 79.
// If fromCueEvent then current_segment_index_ is already added to the | ||
// number of segments before last cue. | ||
segment_info->segment_index = ((fromCueEvent) ? 0 : current_segment_index_) + | ||
num_segments_before_last_cue_ - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be just num_segments_before_last_cue_ without "-1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am subtracting 1 to send the segment index as 0 based. To remove -1, I can move the lines
|108 current_segment_index_ = segment_index;
|109 // Reset subsegment index.
|110 current_subsegment_index_ = 0;
in ChunkingHandler::OnMediaSample after the call to
|112 RETURN_IF_ERROR(EndSegmentIfStarted());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the change you mentioned is needed. current_segment_index_
should also be set to -1 instead of 0 in OnCueEvent.
Here is a simple illustration with segment size of 2.
(| is segment boundary; C is for cue).
_ _ | _ C _ _ | _ _
1 | 2 | 3 | 4
Region 1: current_segment_index_ = 0, num_segments_before_last_cue_ = 0 so segment_info->segment_index = 0
Region 2: current_segment_index_ = 1 => num_segments_before_last_cue_ = 2, current_segment_index_=-1 so segment_info->segment_index = 1
Region 3: current_segment_index_ = 0, num_segments_before_last_cue_ = 2 so segment_info->segment_index = 2
Region 4: current_segment_index_ = 1, num_segments_before_last_cue_ = 2 so segment_info->segment_index = 3
If we don't have unit-tests, we should add unit-tests for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kqyang Thanks for the illustration. I have made this change.
Also looks like there is a unit test for this: ChunkingHandlerTest,CueEvent.
@@ -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_++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have removed it.
if (((segment_start_ / segment_duration_) + 1) > segment_index_) { | ||
segment_index_ = (segment_start_ / segment_duration_) + 1; | ||
} | ||
info->segment_index = segment_index_ - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider the number of cues too.
I think it can just be:
info->segment_index = segment_start_ / segment_duration_ + num_cues_;
packager/mpd/base/mpd_notifier.h
Outdated
/// @return true on success, false otherwise. | ||
virtual bool NotifyNewSegment(uint32_t container_id, | ||
uint64_t start_time, | ||
uint64_t duration, | ||
uint64_t size) = 0; | ||
uint64_t size, | ||
uint64_t segment_index) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment
packager/mpd/base/representation.h
Outdated
virtual void AddNewSegment(int64_t start_time, | ||
int64_t duration, | ||
uint64_t size); | ||
uint64_t size, | ||
uint64_t segment_index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment
EXPECT_THAT(representation_->GetXml().get(), | ||
XmlNodeEqual(ExpectedXml(expected_s_elements))); | ||
XmlNodeEqual(SegmentTimelineTestBase::ExpectedXml( | ||
expected_s_elements, 1372))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is 1372 from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(kStartTime/kDurationSmaller) + 1
@@ -169,6 +169,8 @@ TEST_F(SimpleMpdNotifierTest, NotifyNewSegment) { | |||
SimpleMpdNotifier notifier(empty_mpd_option_); | |||
|
|||
const uint32_t kRepresentationId = 447834u; | |||
const uint64_t kSegmentIndex0 = 0u; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just call it kSegmentIndex
packager/mpd/base/representation.cc
Outdated
if (segment_infos_.empty()) { | ||
if (mpd_options_.mpd_params.target_segment_duration > 0) { | ||
// Store segment index as 1 based. | ||
start_number_ = segment_index + 1; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that segment_index or start_segment_index should be added as a member of SegmentInfo. Then we can remove |start_number_|.
Otherwise the code at line 462 is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kqyang Thanks for the review. Let me try this out. Thinking about how copy constructor changes representation will affect the unit test (RepresentationClone).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kqyang For representation_unittest.cc::TEST_P(TimeShiftBufferDepthTest, Generic) as the segment duration changes, segment index is 34 instead of 1002 which it would have been because of increasing |start_number_|. How can this test be changed in order to rely on start_segment_index which is stored in SegmentInfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kqyang any thoughts? Am I missing anything here? Do you want to move ahead without this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. I think it actually surfaces a problem in the unit-test. I think it is better to have your logic here in the test :).
We can define a member variable in SegmentTemplateTest called: start_segment_index_. It is initialized to start_time/duration in AddSegments if not initialized; increment by 1 in for-loop.
void AddSegments(int64_t start_time,
int64_t duration,
uint64_t size,
int repeat) {
...
if (start_segment_index == -1)
start_segment_index = start_time / duration;
for (int i = 0; i < repeat + 1; ++i) {
representation_->AddNewSegment(start_time, duration, size, start_segment_index++);
start_time += duration;
bandwidth_estimator_.AddBlock(
size, static_cast<double>(duration) / kDefaultTimeScale);
}
}
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added this change.
d006e81
to
bfd88c0
Compare
4cb7f60
to
17a5eb4
Compare
Hi @kqyang, any thoughts on the PR update? |
Sorry for the late reply. Quite busy these days. I haven't had a chance to look through it. I'll do it some time this week. |
Ping |
bump |
Any Update on this. |
replaced by #879 |
This PR will set the start number in representation to the segment index that is sent by muxer.