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.

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

fix: Fix playRangeEnd for certain content #4068

Merged
merged 2 commits into from Mar 29, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/media/presentation_timeline.js
Expand Up @@ -361,6 +361,13 @@ shaka.media.PresentationTimeline = class {
getSegmentAvailabilityEnd() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The JSDoc for the return value, above, says "Aways returns the presentation's duration for video-on-demand." That seems to be pretty out-of-date, now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

if (!this.isLive() && !this.isInProgress()) {
// It's a static manifest (can also be a dynamic->static conversion)
if (this.maxSegmentEndTime_ && this.duration_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we care about the possibility of duration_ or maxSegmentEndTime_ being 0 here?

If not, you don't need to check if duration_ is truthy, because it's a non-nullable number and it initializes to Infinity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, true. Fixed.

// If we have both, use the min of the two.
// Note that the playRangeEnd configuration changes this.duration_.
// See https://github.com/shaka-project/shaka-player/issues/4026
return Math.min(this.maxSegmentEndTime_, this.duration_);
}
// If we don't have both, use whichever we do have.
return this.maxSegmentEndTime_ || this.duration_;
}
// Can be either live or "in-progress recording" (live with known duration)
Expand Down
2 changes: 1 addition & 1 deletion test/player_integration.js
Expand Up @@ -801,7 +801,7 @@ describe('Player', () => {

// Check that the final seek range is as expected.
const seekRange = player.seekRange();
expect(seekRange.end).toBe(14);
expect(seekRange.end).toBeCloseTo(14);
});
});

Expand Down
34 changes: 34 additions & 0 deletions test/player_unit.js
Expand Up @@ -690,6 +690,7 @@ describe('Player', () => {
player.configure({playRangeStart: 5, playRangeEnd: 10});
const timeline = new shaka.media.PresentationTimeline(300, 0);
timeline.setStatic(true);
timeline.setDuration(300);
manifest = shaka.test.ManifestGenerator.generate((manifest) => {
manifest.presentationTimeline = timeline;
manifest.addVariant(0, (variant) => {
Expand All @@ -703,6 +704,39 @@ describe('Player', () => {
expect(seekRange.end).toBe(10);
});

// Test for https://github.com/shaka-project/shaka-player/issues/4026
it('configures play and seek range with notifySegments', async () => {
player.configure({playRangeStart: 5, playRangeEnd: 10});
const timeline = new shaka.media.PresentationTimeline(300, 0);
timeline.setStatic(true);
timeline.setDuration(300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit confusing how in this test, playRangeEnd corresponds with duration_ inside the timeline, and the setDuration method call corresponds with maxSegmentEndTime_ in the timeline. Feels like the two things named duration should match up.
That's out of the scope of this CL though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's weird. What's really happening is that stream.useSegmentTemplate refers to timeline's duration to decide how many references to generate. Then player.load applies the config to the timeline, and sets the duration lower.

What would be less confusing, I think, is if Timeline had a separate field for the playRangeEnd setting. But that would also require a lot more ifs or mins in Timeline, which could be confusing in a different way.

manifest = shaka.test.ManifestGenerator.generate((manifest) => {
manifest.presentationTimeline = timeline;
manifest.addVariant(0, (variant) => {
variant.addVideo(1, (stream) => {
stream.useSegmentTemplate(
'$Number$.mp4', /* segmentDuration= */ 10);
});
});
});
goog.asserts.assert(manifest, 'manifest must be non-null');

// Explicitly notify the timeline of the segment references.
const videoStream = manifest.variants[0].video;
await videoStream.createSegmentIndex();
goog.asserts.assert(videoStream.segmentIndex,
'SegmentIndex must be non-null');
const references = Array.from(videoStream.segmentIndex);
goog.asserts.assert(references.length != 0,
'Must have references for this test!');
timeline.notifySegments(references);

await player.load(fakeManifestUri, 0, fakeMimeType);
const seekRange = player.seekRange();
expect(seekRange.start).toBe(5);
expect(seekRange.end).toBe(10);
});

it('configures play and seek range after playback starts', async () => {
const timeline = new shaka.media.PresentationTimeline(300, 0);
timeline.setStatic(true);
Expand Down
9 changes: 8 additions & 1 deletion test/test/util/manifest_generator.js
Expand Up @@ -608,9 +608,16 @@ shaka.test.ManifestGenerator.Stream = class {
segmentDuration, 'Must pass a non-zero segment duration');

const shaka_ = this.manifest_.shaka_;
const totalDuration = this.manifest_.presentationTimeline.getDuration();
let totalDuration = this.manifest_.presentationTimeline.getDuration();
goog.asserts.assert(
isFinite(totalDuration), 'Must specify a manifest duration');
if (!isFinite(totalDuration)) {
// Without this, a mistake of an infinite duration would result in an
// infinite loop and a crash, which would in turn prevent you from seeing
// the above assertion fail.
totalDuration = 0;
}

const segmentCount = totalDuration / segmentDuration;
const references = [];

Expand Down