[Code refactoring] remove circular dependencies #3256
Conversation
bbert
force-pushed
the
refactoring-circular-deps
branch
from
db9b083
to
1f6a3b9
Compare
- rename BufferController.switchInitData() by appendInitSegment() - add method BufferController.replaceBuffer() for replacing buffer when switching track (replace resetBufferInProgress mechanism)
…Controller (to update fragmentDuration for example in case of SegmentTimeline)
| } else { | ||
| const seekTarget = playbackController.getStreamStartTime(false); | ||
| bufferController.setSeekStartTime(seekTarget); | ||
| scheduleController.setCurrentRepresentation(getRepresentationInfo()); |
There was a problem hiding this comment.
Before this was also set for livestreams. Might need to move this up to also be applied to livestreams
There was a problem hiding this comment.
This is done in setLiveEdgeSeekTarget() method.
This is the way it was done before in ScheduleController
| addDVRMetric(); | ||
| if (!e.error) { | ||
| scheduleController.setCurrentRepresentation(adapter.convertDataToRepresentationInfo(e.currentRepresentation)); | ||
| } else if (e.error.code !== Errors.SEGMENTS_UPDATE_FAILED_ERROR_CODE) { |
There was a problem hiding this comment.
Compared to previous check, addDVRMetric should only be called if e.error.code === Errors.SEGMENTS_UPDATE_FAILED_ERROR_CODE
There was a problem hiding this comment.
Previous check was:
if (e.sender.getType() !== getType() || e.sender.getStreamId() !== getStreamInfo().id || !e.error || e.error.code !== Errors.SEGMENTS_UPDATE_FAILED_ERROR_CODE) return;
| let representationInfo = getRepresentationInfo(e.newQuality); | ||
| scheduleController.setCurrentRepresentation(representationInfo); | ||
| dashMetrics.pushPlayListTraceMetrics(new Date(), PlayListTrace.REPRESENTATION_SWITCH_STOP_REASON); | ||
| dashMetrics.createPlaylistTraceMetrics(representationInfo.id, playbackController.getTime() * 1000, playbackController.getPlaybackRate()); |
There was a problem hiding this comment.
Might need to add (playbackRate !== null ? playbackRate.toString() : null)
There was a problem hiding this comment.
This check is done in DashMetrics.createPlaylistTraceMetrics()
| if (e.sender.getStreamProcessor() !== instance) return; | ||
| let manifest = manifestModel.getValue(); | ||
| if (!manifest.doNotUpdateDVRWindowOnBufferUpdated) { | ||
| if (e.streamId !== streamInfo.id || e.mediaType !== type) return; |
There was a problem hiding this comment.
We could add "dashMetrics.addBufferLevel()" at this point and remove dashMetrics from the BufferController.
There was a problem hiding this comment.
Yes, good point. I'll do it this way
| @@ -296,7 +400,7 @@ function StreamProcessor(config) { | |||
| } | |||
|
|
|||
| function setMediaSource(mediaSource) { | |||
| bufferController.setMediaSource(mediaSource, getMediaInfo()); | |||
| bufferController.setMediaSource(mediaSource, getMediaInfoArr()); | |||
There was a problem hiding this comment.
I suggest to rename the variable in BufferController.setMediaSource(value,mediaInfo) to mediaInfoArr as well
There was a problem hiding this comment.
Additional suggestion: Use mediaInfoArr instead of calling the getter function
| const currentRepresentation = getRepresentationInfo(quality); | ||
|
|
||
| // Update current representation info (to update fragmentDuration for example in case of SegmentTimeline) | ||
| scheduleController.setCurrentRepresentation(currentRepresentation); |
There was a problem hiding this comment.
Question: Is it possible that the fragmentDuration changed at this point? Why does the representation info change when a fragment has been loaded?
There was a problem hiding this comment.
Yes, for SegmentTimeline the segmentDuration (and thus fragmentDuration) is set and available only once a fragment has been requested (in TimelineSegmentsGetter)
| } | ||
| } | ||
|
|
||
| function onQualityChanged(e) { | ||
| if (requiredQuality === e.newQuality || type !== e.mediaType || streamProcessor.getStreamInfo().id !== e.streamInfo.id) return; | ||
| if (e.streamInfo.id != streamInfo.id || e.mediaType !== type || requiredQuality === e.newQuality) return; |
There was a problem hiding this comment.
Use !== for comparing streamInfo ids
| resetInitialSettings(); | ||
| } | ||
|
|
||
| function initialize() { | ||
| function initialize(hasVideoTrack) { | ||
| hasVideoTrack = hasVideoTrack; |
There was a problem hiding this comment.
Rename either the outer variable of the payload of the function. Otherwise we overload the name of the variable.
There was a problem hiding this comment.
Yes, I missed it. It's fixed
| const initCache = InitCache(context).getInstance(); | ||
|
|
||
| describe('BufferController', function () { | ||
| describe.only('BufferController', function () { |
to/from StreamController and StreamProcessor, as illustrated in following figure (remove red relations):
Thanks to this PR, responsabilities are better distributed between the classes.
It especially required to refactor the sheduling mechanism, as illustrated in the following sequence diagrams:
Also, the NextFragmentRule is removed and its logic direclty integrate in the StreamProcessor, as IMO this is not really a rule.