Skip to content

Commit

Permalink
Consider src= fully loaded only after first frame
Browse files Browse the repository at this point in the history 
This fixes the definition of load() to wait for a frame before
resolving the load() Promise for src= playbacks.  Now methods like
isAudioOnly can be trusted as soon as load() resolves.

This also allows load() to fail for src= playbacks if an error event
fires from the media element.

Issue #816
Issue #997

Change-Id: I0f6120d1334bbebcb78efdbbca65c7981f3ef265
  • Loading branch information
@joeyparrish
joeyparrish committed Apr 30, 2019
1 parent 8178e91 commit 831dfa6
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 43 deletions.
60 changes: 51 additions & 9 deletions lib/player.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1916,7 +1916,31 @@ shaka.Player.prototype.onSrcEquals_ = function(has, wants) {
// Dispatch a 'trackschanged' event, for the same reasons as 'streaming'.
this.onTracksChanged_();

return shaka.util.AbortableOperation.completed(undefined);
// This is fully loaded when we have loaded the first frame.
const fullyLoaded = new shaka.util.PublicPromise();
if (this.video_.readyState >= HTMLMediaElement.HAVE_CURRENT_DATA) {
// Already done!
fullyLoaded.resolve();
} else if (this.video_.error) {
// Already failed!
fullyLoaded.reject(this.videoErrorToShakaError_());
} else {
// Wait for success or failure.
this.eventManager_.listenOnce(this.video_, 'loadeddata', () => {
fullyLoaded.resolve();
});
this.eventManager_.listenOnce(this.video_, 'error', () => {
fullyLoaded.reject(this.videoErrorToShakaError_());
});
}
return new shaka.util.AbortableOperation(fullyLoaded, /* onAbort= */ () => {
const abortedError = new shaka.util.Error(
shaka.util.Error.Severity.CRITICAL,
shaka.util.Error.Category.PLAYER,
shaka.util.Error.Code.OPERATION_ABORTED);
fullyLoaded.reject(abortedError);
return Promise.resolve(); // Abort complete.
});
};

/**
Expand Down Expand Up @@ -4421,17 +4445,22 @@ shaka.Player.prototype.onRegionEvent_ = function(eventName, region) {


/**
* @param {!Event} event
* Turn the media element's error object into a Shaka Player error object.
*
* @return {shaka.util.Error}
* @private
*/
shaka.Player.prototype.onVideoError_ = function(event) {
if (!this.video_.error) return;
shaka.Player.prototype.videoErrorToShakaError_ = function() {
goog.asserts.assert(this.video_.error, 'Video error expected, but missing!');
if (!this.video_.error) {
return null;
}

let code = this.video_.error.code;
const code = this.video_.error.code;
if (code == 1 /* MEDIA_ERR_ABORTED */) {
// Ignore this error code, which should only occur when navigating away or
// deliberately stopping playback of HTTP content.
return;
return null;
}

// Extra error information from MS Edge and IE11:
Expand All @@ -4446,13 +4475,26 @@ shaka.Player.prototype.onVideoError_ = function(event) {
}

// Extra error information from Chrome:
let message = this.video_.error.message;
const message = this.video_.error.message;

this.onError_(new shaka.util.Error(
return new shaka.util.Error(
shaka.util.Error.Severity.CRITICAL,
shaka.util.Error.Category.MEDIA,
shaka.util.Error.Code.VIDEO_ERROR,
code, extended, message));
code, extended, message);
};


/**
* @param {!Event} event
* @private
*/
shaka.Player.prototype.onVideoError_ = function(event) {
const error = this.videoErrorToShakaError_();
if (!error) {
return;
}
this.onError_(error);
};


Expand Down
70 changes: 36 additions & 34 deletions test/player_integration.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,8 @@ describe('Player Manifest Retries', function() {
// This test suite focuses on how the player moves through the different load
// states.
describe('Player Load Path', () => {
const SMALL_MP4_CONTENT_URI = '/base/test/test/assets/small.mp4';

/** @type {!HTMLVideoElement} */
let video;
/** @type {shaka.Player} */
Expand Down Expand Up @@ -618,17 +620,17 @@ describe('Player Load Path', () => {

it('attach and initialize media source when constructed with media element',
async () => {
expect(video.src).toBeFalsy();
expect(video.src).toBeFalsy();

createPlayer(/* attachedTo= */ video);
createPlayer(/* attachedTo= */ video);

// Wait until we enter the media source state.
await new Promise((resolve) => {
whenEnteringState('media-source', resolve);
});
// Wait until we enter the media source state.
await new Promise((resolve) => {
whenEnteringState('media-source', resolve);
});

expect(video.src).toBeTruthy();
});
expect(video.src).toBeTruthy();
});

it('does not set video.src when no video is provided', async function() {
expect(video.src).toBeFalsy();
Expand All @@ -644,43 +646,43 @@ describe('Player Load Path', () => {

it('attach + initializeMediaSource=true will initialize media source',
async () => {
createPlayer(/* attachedTo= */ null);
createPlayer(/* attachedTo= */ null);

expect(video.src).toBeFalsy();
await player.attach(video, /* initializeMediaSource= */ true);
expect(video.src).toBeTruthy();
});
expect(video.src).toBeFalsy();
await player.attach(video, /* initializeMediaSource= */ true);
expect(video.src).toBeTruthy();
});

it('attach + initializeMediaSource=false will not intialize media source',
async () => {
createPlayer(/* attachedTo= */ null);
createPlayer(/* attachedTo= */ null);

expect(video.src).toBeFalsy();
await player.attach(video, /* initializeMediaSource= */ false);
expect(video.src).toBeFalsy();
});
expect(video.src).toBeFalsy();
await player.attach(video, /* initializeMediaSource= */ false);
expect(video.src).toBeFalsy();
});

it('unload + initializeMediaSource=false does not initialize media source',
async () => {
createPlayer(/* attachedTo= */ null);
createPlayer(/* attachedTo= */ null);

await player.attach(video);
await player.load('test:sintel');
await player.attach(video);
await player.load('test:sintel');

await player.unload(/* initializeMediaSource= */ false);
expect(video.src).toBeFalsy();
});
await player.unload(/* initializeMediaSource= */ false);
expect(video.src).toBeFalsy();
});

it('unload + initializeMediaSource=true initializes media source',
async () => {
createPlayer(/* attachedTo= */ null);
createPlayer(/* attachedTo= */ null);

await player.attach(video);
await player.load('test:sintel');
await player.attach(video);
await player.load('test:sintel');

await player.unload(/* initializeMediaSource= */ true);
expect(video.src).toBeTruthy();
});
await player.unload(/* initializeMediaSource= */ true);
expect(video.src).toBeTruthy();
});

// There was a bug when calling unload before calling load would cause
// the load to continue before the (first) unload was complete.
Expand Down Expand Up @@ -1083,15 +1085,15 @@ describe('Player Load Path', () => {
// Normally the player would load content like this with the media source
// path, but since we don't have media source support, it should use the
// src= path.
player.load('test:sintel');
player.load(SMALL_MP4_CONTENT_URI);

const event = await spyIsCalled(stateIdleSpy);
expect(event.state).toBe('src-equals');
});

it('unloading ignores init media source flag', async () => {
await player.attach(video, /* initMediaSource= */ false);
await player.load('test:sintel');
await player.load(SMALL_MP4_CONTENT_URI);

// Normally the player would try to go to the media source state because
// we are saying to initialize media source after unloading, but since we
Expand Down Expand Up @@ -1378,7 +1380,7 @@ describe('Player Load Path', () => {
})
.set('src-equals', async () => {
await player.attach(video, /* initMediaSource= */ false);
await player.load('test:sintel', 0, 'video/mp4');
await player.load(SMALL_MP4_CONTENT_URI, 0, 'video/mp4');
});

const action = actions.get(state);
Expand Down Expand Up @@ -1472,7 +1474,7 @@ describe('Player Load Path', () => {
return player.load('test:sintel');
})
.set('src-equals', () => {
return player.load('test:sintel', 0, 'video/mp4');
return player.load(SMALL_MP4_CONTENT_URI, 0, 'video/mp4');
});

const action = actions.get(state);
Expand Down

0 comments on commit 831dfa6

Please sign in to comment.