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 all commits
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
15 changes: 12 additions & 3 deletions lib/media/presentation_timeline.js
Expand Up @@ -354,14 +354,23 @@ shaka.media.PresentationTimeline = class {
* starting after this time should be assumed to be unavailable.
*
* @return {number} The current segment availability end time, in seconds,
* relative to the start of the presentation. Always returns the
* presentation's duration for video-on-demand.
* relative to the start of the presentation. For VOD, the availability
* end time is the content's duration. If the Player's playRangeEnd
* configuration is used, this can override the duration.
* @export
*/
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)
return this.maxSegmentEndTime_ || this.duration_;
if (this.maxSegmentEndTime_) {
// If we know segment times, use the min of that and duration.
// 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_);
} else {
// If we don't have segment times, use duration.
return this.duration_;
}
}
// Can be either live or "in-progress recording" (live with known duration)
return Math.min(this.getLiveEdge_() + this.availabilityTimeOffset_,
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
41 changes: 40 additions & 1 deletion test/player_unit.js
Expand Up @@ -687,17 +687,56 @@ describe('Player', () => {
});

it('configures play and seek range for VOD', async () => {
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) => {
variant.addVideo(1);
});
});
goog.asserts.assert(manifest, 'manifest must be non-null');

player.configure({playRangeStart: 5, playRangeEnd: 10});
await player.load(fakeManifestUri, 0, fakeMimeType);

const seekRange = player.seekRange();
expect(seekRange.start).toBe(5);
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 () => {
const timeline = new shaka.media.PresentationTimeline(300, 0);
timeline.setStatic(true);
// This duration is used by useSegmentTemplate below to decide how many
// references to generate.
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);

player.configure({playRangeStart: 5, playRangeEnd: 10});
await player.load(fakeManifestUri, 0, fakeMimeType);

const seekRange = player.seekRange();
expect(seekRange.start).toBe(5);
expect(seekRange.end).toBe(10);
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