If beforeRequest is set, reuse it on source changes #983
Conversation
src/videojs-contrib-hls.js
Outdated
| @@ -674,12 +674,23 @@ const HlsSourceHandler = function(mode) { | |||
|
|
|||
| let settings = videojs.mergeOptions(options, {hls: {mode}}); | |||
|
|
|||
| let previousBeforeRequest; | |||
|
|
|||
| if (tech.hls && tech.hls.xhr && tech.hls.xhr.beforeRequest) { | |||
There was a problem hiding this comment.
You don't need to check for tech.hls.xhr.beforeRequest here. Even if beforeRequest is undefined, the next line won't cause an exception.
src/videojs-contrib-hls.js
Outdated
| let previousBeforeRequest; | ||
|
|
||
| if (tech.hls && tech.hls.xhr && tech.hls.xhr.beforeRequest) { | ||
| previousBeforeRequest = tech.hls.xhr.beforeRequest; |
There was a problem hiding this comment.
Could change this to previousBeforeRequest = tech.hls.xhr.beforeRequest || videojs.Hls.xhr.beforeRequest; This could remove the need for the if statements below to instead just always set tech.hls.xhr.beforeRequest = previousBeforeRequest;. However, the code in its current state may be more understandable, so I'd only consider this comment if you think its an improvement there
|
Many of the people in the issue you referenced mention that using the global override |
|
Good call @mjneil , changed it so that they can be changed anytime. |
src/videojs-contrib-hls.js
Outdated
| @@ -674,13 +674,20 @@ const HlsSourceHandler = function(mode) { | |||
|
|
|||
| let settings = videojs.mergeOptions(options, {hls: {mode}}); | |||
|
|
|||
| let previousBeforeRequest; | |||
|
|
|||
| if (tech.hls && tech.hls.xhr) { | |||
There was a problem hiding this comment.
Not sure how I feel about this. We should be cleaning up tech.hls on dispose anyway. That we don't is actually a bug.
There was a problem hiding this comment.
On dispose is fine, but what about on source changes? Since the hls object is per player, shouldn't it continue to exist and not be disposed?
There was a problem hiding this comment.
No, the source handler is disposed of on each source change.
There was a problem hiding this comment.
In fact, the HLS property on the player is for compatibility reasons. David and I were considering removing the object all together if HLS is used with Video.js 6+
gesinger
force-pushed
the
fix-source-change-before-request
branch
from
102cfd0 to
7986734
Compare
Description
handleSource would replace any preexisting beforeRequest.
Reported in #665
Example of bug: http://jsbin.com/qijehan/2/edit?html,console,output
Specific Changes proposed
Now handleSource checks for a preexisting beforeRequest, and reuses it if it is there.
Requirements Checklist