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

fix: only append init segment on change #1128

Merged
merged 6 commits into from
May 17, 2021
Merged

fix: only append init segment on change #1128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a12e38a
fix: only append init segment on change
brandonocasey May 11, 2021
172c7d8
add tests
brandonocasey May 14, 2021
c6d9ee8
Merge branch 'main' into fix/only-append-changed-init
brandonocasey May 14, 2021
e9b73cb
test comment
brandonocasey May 17, 2021
a61d7f2
comment change
brandonocasey May 17, 2021
f7aab4d
Update src/segment-loader.js
brandonocasey May 17, 2021
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
18 changes: 15 additions & 3 deletions src/segment-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2096,9 +2096,8 @@ export default class SegmentLoader extends videojs.EventTarget {
// we can re-append the init segment in the event that we get data from a new
// playlist. Discontinuities and track changes are handled in other sections.
this.playlistOfLastInitSegment_[type] = playlist;
// we should only be appending the next init segment if we detect a change, or if
// the segment has a map
this.appendInitSegment_[type] = map ? true : false;
// Disable future init segment appends for this type. Until a change is necessary.
this.appendInitSegment_[type] = false;

// we need to clear out the fmp4 active init segment id, since
// we are appending the muxer init segment
Expand Down Expand Up @@ -2368,6 +2367,19 @@ export default class SegmentLoader extends videojs.EventTarget {

this.logger_(`Requesting ${segmentInfoString(segmentInfo)}`);

// If there's an init segment associated with this segment, but it is not cached (identified by a lack of bytes),
// then this init segment has never been seen before and should be appended.
//
// At this point the content type (audio/video or both) is not yet known, but it should be safe to set
// both to true and leave the decision of whether to append the init segment to append time.
if (simpleSegment.map && !simpleSegment.map.bytes) {
this.logger_('going to request init segment.');
this.appendInitSegment_ = {
video: true,
audio: true
};
Comment on lines +2377 to +2380
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be dependent on the type that this loader is handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it should because if we get an audio or video init segment on this segment loader, we should re-append it. It won't likely happen for a video only loader or an audio only loader, but at this point we won't actually know what type of content we are requesting, as we get that in trackinfo.

}

segmentInfo.abortRequests = mediaSegmentRequest({
xhr: this.vhs_.xhr,
xhrOptions: this.xhrOptions_,
Expand Down
45 changes: 43 additions & 2 deletions test/segment-loader.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3079,7 +3079,7 @@ QUnit.module('SegmentLoader', function(hooks) {
origAppendToSourceBuffer(config);
};

const playlist = playlistWithDuration(30);
const playlist = playlistWithDuration(40);

playlist.segments[0].map = {
resolvedUri: 'init.mp4',
Expand All @@ -3096,6 +3096,11 @@ QUnit.module('SegmentLoader', function(hooks) {
byterange: { length: Infinity, offset: 0 }
};

playlist.segments[3].map = {
resolvedUri: 'init.mp4',
byterange: { length: Infinity, offset: 0 }
};

loader.playlist(playlist);
loader.load();
this.clock.tick(1);
Expand Down Expand Up @@ -3142,6 +3147,9 @@ QUnit.module('SegmentLoader', function(hooks) {
appends[1].initSegment,
'appended a different init segment'
);
// force init segment append to prove that init segments are not
// re-requested, but will be re-appended when needed.
loader.appendInitSegment_.audio = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were relying on re-appending init segments, which shoudn't happen here, but I think these tests are still valuable.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth adding those details to the comment (here and below) to ensure we don't forget why we force it in the future.


// no init segment request, as it should be the same (and cached) segment
standardXHRResponse(this.requests.shift(), mp4AudioSegment());
Expand All @@ -3150,6 +3158,7 @@ QUnit.module('SegmentLoader', function(hooks) {
loader.one('error', reject);
});
}).then(() => {
this.clock.tick(1);

assert.equal(appends.length, 3, 'one more append');
assert.equal(appends[2].type, 'audio', 'appended to audio buffer');
Expand All @@ -3159,6 +3168,17 @@ QUnit.module('SegmentLoader', function(hooks) {
appends[2].initSegment,
'reused the init segment'
);

// no init segment request, as it should be the same (and cached) segment
standardXHRResponse(this.requests.shift(), mp4AudioSegment());
return new Promise((resolve, reject) => {
loader.one('appended', resolve);
loader.one('error', reject);
});
}).then(() => {
assert.equal(appends.length, 4, 'one more append');
assert.equal(appends[3].type, 'audio', 'appended to audio buffer');
assert.notOk(appends[3].initSegment, 'did not append audio init segment');
});
});

Expand All @@ -3176,7 +3196,7 @@ QUnit.module('SegmentLoader', function(hooks) {
origAppendToSourceBuffer(config);
};

const playlist = playlistWithDuration(30);
const playlist = playlistWithDuration(40);

playlist.segments[0].map = {
resolvedUri: 'init.mp4',
Expand All @@ -3193,6 +3213,11 @@ QUnit.module('SegmentLoader', function(hooks) {
byterange: { length: Infinity, offset: 0 }
};

playlist.segments[3].map = {
resolvedUri: 'init.mp4',
byterange: { length: Infinity, offset: 0 }
};

loader.playlist(playlist);
loader.load();
this.clock.tick(1);
Expand Down Expand Up @@ -3229,13 +3254,18 @@ QUnit.module('SegmentLoader', function(hooks) {
'appended a different init segment'
);

// force init segment append to prove that init segments are not
// re-requested, but will be re-appended when needed.
loader.appendInitSegment_.video = true;

// no init segment request, as it should be the same (and cached) segment
standardXHRResponse(this.requests.shift(), mp4VideoSegment());
return new Promise((resolve, reject) => {
loader.one('appended', resolve);
loader.one('error', reject);
});
}).then(() => {
this.clock.tick(1);

assert.equal(appends.length, 3, 'one more append');
assert.equal(appends[2].type, 'video', 'appended to video buffer');
Expand All @@ -3245,6 +3275,17 @@ QUnit.module('SegmentLoader', function(hooks) {
appends[2].initSegment,
'reused the init segment'
);

// no init segment request, as it should be the same (and cached) segment
standardXHRResponse(this.requests.shift(), mp4VideoSegment());
return new Promise((resolve, reject) => {
loader.one('appended', resolve);
loader.one('error', reject);
});
}).then(() => {
assert.equal(appends.length, 4, 'one more append');
assert.equal(appends[3].type, 'video', 'appended to video buffer');
assert.notOk(appends[3].initSegment, 'did not append video init segment');
});
});

Expand Down