fix: loss of crossOrigin value when loadMedia is called

[amtins]

Description

This PR fixes the deletion of the crossOrigin value when loadMedia is called.

Fixes: #8086

Specific Changes proposed

• stores the value of crossOrigin before reset deletes it
• restores the value of crossOrigin after reset has been called
• unit test to verify that the value of crossOrigin is restored :
  • when the crossOrigin method is called before loadMedia
  • when the crossOrigin value is passed via the options then loadMedia is called
  • when the video tag contains the crossOrigin property then loadMedia is called

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Change has been verified in an actual browser (Chrome, Firefox, IE)
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

[codecov]

Codecov Report

Merging #8085 (1b35a17) into main (1491d71) will increase coverage by 82.42%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main    #8085       +/-   ##
=========================================
+ Coverage      0   82.42%   +82.42%     
=========================================
  Files         0      112      +112     
  Lines         0     7477     +7477     
  Branches      0     1801     +1801     
=========================================
+ Hits          0     6163     +6163     
- Misses        0     1314     +1314     

Impacted Files	Coverage Δ
src/js/player.js	90.29% <100.00%> (ø)

... and 111 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[misteroneill]

Seems reasonable to me overall, but I'm not totally confident that the crossorigin setting is something we want to carry across source loads.

Perhaps an alternative would be calling player.crossOrigin() in the ready callback?

player.loadMedia({...}, () => {
  player.crossOrigin('anonymous');
});

Or would that potentially be too late?

[amtins]

@misteroneill thank you for taking the time to review this PR 👍🏽

Perhaps an alternative would be calling player.crossOrigin() in the ready callback?

TL;DR Quick answer, yes it will work.

Maybe I can add a little more context regarding this PR. This PR is the result of this issue #8068 and the comments #8068 (comment) and #8068 (comment)

According to the loadMedia documentation this function should Populate the player using a MediaObject.

However, this function does much more, it resets the player, but also deletes and rebuilds the video element. The consequence is the creation of a new playback session losing properties that are important for the developer and interesting for the user playback experience (not detailed as it's out of the scope of this PR).

The important property for the developer

videojs allows the crossOrigin parameter to be supplied as an instance option even though it's not documented. A developer can expect this option to remain unchanged for the life of the player instance or until it is intentionally changed. Refer to

video.js/src/js/player.js

Lines 776 to 777 in 1491d71

	// support both crossOrigin and crossorigin to reduce confusion and issues around the name
	this.crossOrigin(this.options_.crossOrigin || this.options_.crossorigin);

Demonstration

src and loadMedia produce two different behaviors depending on which one is used:

const player videojs('player', {crossorigin : 'anonymous'});

player.el().firstChild.crossOrigin; // -> anonymous 
player.options().crossorigin; // -> anonymous 
player.crossorigin(); // -> anonymous

player.src('...')

player.el().firstChild.crossOrigin; // -> anonymous 
player.options().crossorigin; // -> anonymous 
player.crossorigin(); // -> anonymous

player.loadMedia('...')

player.el().firstChild.crossOrigin; // -> null 
player.options().crossorigin; // -> anonymous 
player.crossorigin(); // -> null

This means that these two functions behave very differently. src keeps the current playback session while loadmedia recreates a new playback session almost as if it were a new instance of the player, but without the crossOrigin, which leans more towards to a bug.

Code references

The workflow leading to the loss of this property:

video.js/src/js/player.js

Line 1274 in 1491d71

this.tech_.dispose();

video.js/src/js/player.js

Line 1125 in 866ef24

this.unloadTech_();

The fix would have been more appropriate in techOptions, but that also involves modifying html5.js and in the end would not handle as many cases as the proposed fix.

video.js/src/js/player.js

Lines 1152 to 1169 in 866ef24

	const techOptions = {
	source,
	autoplay,
	'nativeControlsForTouch': this.options_.nativeControlsForTouch,
	'playerId': this.id(),
	'techId': `${this.id()}_${camelTechName}_api`,
	'playsinline': this.options_.playsinline,
	'preload': this.options_.preload,
	'loop': this.options_.loop,
	'disablePictureInPicture': this.options_.disablePictureInPicture,
	'muted': this.options_.muted,
	'poster': this.poster(),
	'language': this.language(),
	'playerElIngest': this.playerElIngest_ || false,
	'vtt.js': this.options_['vtt.js'],
	'canOverridePoster': !!this.options_.techCanOverridePoster,
	'enableSourceset': this.options_.enableSourceset
	};

video.js/src/js/player.js

Line 1196 in 866ef24

this.tech_ = new TechClass(techOptions);

[misteroneill]

Great explanation, I see the issue now - thank you!

[gkatsev]

While we're at it, should the object that loadMedia takes accept a crossOrigin property to force the value to something specific? This would be an additional feature and can come in a separate PR.

I think this change is fine as is.

[misteroneill]

I'm going to accept your comment as a second approval @gkatsev 😄