Major Refactor reduces complication, increases dependability and solve critical bugs. #1523

dsparacio · 2016-08-02T01:11:48Z

This is a big change that just all keep creeping due to the original task; which was to solve issue #1365 and remove the Virtual Buffer. This task is complete!

That lead to multi-track changes since multi-track depended on VB. This lead to a refactor of buffer controller and schedule controller. But.....

While doing this @spiterikevin noticed duplicate loading in certain cases so I set out to fix that as well. This changed solved the duplicate issue and should solve #1205 as well!!

Then issue #1514 was reported and this solution for this fit into the current work for fragmentModel and multitrack... so It is also fixed and part of this PR.

Some of the line changes are really just es6 conversion. E.g use of const to improve readability and efficiency.

I have tested this code and reviewed it extensively.! Please review if you have time!

… store init segments for switching.

…priority for events with same type

…a request, simplifed buffercontroller and schedulecontroller post VB removal

…ntModel to es6 syntax

…a type so I may persist the model through track and period changes. Major es5 cleanup and some notes.

dsparacio · 2016-08-02T01:16:34Z

@bbcrddave @LloydW93 or anyone else, Maybe you can check this out would love to get some eyes on it. I am going to run BBC browserstack test soon to see if results are same or better...

davemevans · 2016-08-02T07:49:50Z

src/streaming/controllers/BufferController.js

@@ -131,16 +129,15 @@ function BufferController(config) {
        eventBus.on(Events.PLAYBACK_RATE_CHANGED, onPlaybackRateChanged, this);
        eventBus.on(Events.PLAYBACK_SEEKING, onPlaybackSeeking, this);
        eventBus.on(Events.WALLCLOCK_TIME_UPDATED, onWallclockTimeUpdated, this);
-        eventBus.on(Events.CURRENT_TRACK_CHANGED, onCurrentTrackChanged, this);
+        eventBus.on(Events.CURRENT_TRACK_CHANGED, onCurrentTrackChanged, this, 0);


EventBus should probably export some consts rather than using magic numbers - it isn't clear what priority 0 is.

If we do something like states of priority CONST the question is, is that scalable? What happens if there are five preset states of priority but then the use of an event type grows to 6 listeners and you need all six in a certain order. I opted for a pure int based system which is limitless so I want to see how we can make this more clear as I do agree this needs some help. However in most other event systems I have used it is the reverse, Higher number holds more priority.

It may be nice to have a few preset priorities that the library uses with plenty of space between them. Then third party developers and others in the library can have flexibility to adjust as needed.

Something like:

const priorities = { low: 0, normal: 200, high: 500 }

Obviously if we have need for more than those we can adjust, but at least that gives people some indication of where we're at.

This also has the benefit of resolving the "is 0 low or high" question by adding some labels to at least a few numbers.

davemevans · 2016-08-02T12:37:48Z

Great effort Dan!

dsparacio · 2016-08-02T16:42:46Z

Thanks Dave! Thanks for the great code review. I’ll fix all the commented items but we should discuss event priority and how to make it most useful and clear. I do want to keep that in. Thanks again for taking the time!

Dan

From: David Evans notifications@github.com
Reply-To: "Dash-Industry-Forum/dash.js" reply@reply.github.com
Date: Tuesday, August 2, 2016 at 5:37 AM
To: "Dash-Industry-Forum/dash.js" dash.js@noreply.github.com
Cc: Daniel Sparacio dsparaci@akamai.com, Author author@noreply.github.com
Subject: Re: [Dash-Industry-Forum/dash.js] Major Refactor reduces complication, increases dependability and solve critical bugs. (#1523)

Great effort Dan!

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Dash-2DIndustry-2DForum_dash.js_pull_1523-23issuecomment-2D236891107&d=DQMCaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=HMIPFyLErj4M8VUwg3x36W02236DJoh_A_hxovpfrCc&m=s7JjvEt3PFsDEuRHFF1yrprFNcGC5_n33zfM256ip2c&s=VjRnX8fxMFOOCRz-opGdRqF7zoVye5gbkFPouPmLyyI&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AHgAjsHfG4RkyD6EiubrfhHwHBD1r9Mmks5qbzokgaJpZM4JaJih&d=DQMCaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=HMIPFyLErj4M8VUwg3x36W02236DJoh_A_hxovpfrCc&m=s7JjvEt3PFsDEuRHFF1yrprFNcGC5_n33zfM256ip2c&s=GNrnnjfxb0jIojjxTsFQwfVS6pylwSjlVXfM-qt63zs&e=.

boushley · 2016-08-02T17:29:40Z

src/streaming/StreamProcessor.js

-        if (fragmentModel) {
-            fragmentModel.reset();
-            fragmentModel = null;
-        }


Why does the fragmentModel no longer need to be reset or nulled out? It's still being setup in the initialize method.

It is cleaned up in FragmentController and persisted over new creations of schedule controller. It is using the same one if it exists in the init of schedule controller.

Gotcha fair enough. I saw the .create for RepresentationController above and below this and assume we were creating FragmentModel... but we're not. So disregard

boushley · 2016-08-02T18:01:46Z

Gotta love when you remove > 2x the code added and things get better 👍

… for priority added default param, refactored a bit and added off for new listener in schedule controller.

dsparacio · 2016-08-04T02:45:30Z

@bbcrddave @boushley if you get a chance check out 97673f7

boushley · 2016-08-04T13:56:44Z

This looks good. Much clearer what priority is low and what is high.

Looking good 👍

dsparacio · 2016-08-04T16:38:44Z

Thanks guys. I am going to do some deep browsers os testing today and make sure all is well then merge by end of week hopefully!

…le controller. Data change / Stream init handlers would allow media type to cross and pollute the representation info usually of audio. Fixed buffer target access for metrics in buffer controller. Removed unused items in buffer controller and fixed in proper init of objects.

boushley · 2016-08-05T18:32:30Z

👏

…rum#1514 Dash-Industry-Forum#1523)

Dan Sparacio added 7 commits July 26, 2016 14:32

Removed all calls and use of Virtual buffer and added InitCache.js to…

b20199f

… store init segments for switching.

removed virtual buffer completly and replaced with InitCache. - adds …

86941ce

…priority for events with same type

minor cleanup

dfda08c

some updates

223c1b4

Fixes issue with fragments loading init out of sync, in parallel medi…

636f4f9

…a request, simplifed buffercontroller and schedulecontroller post VB removal

fixes issue with segmenttimeline duplicated loading. Refactors Fragme…

8966b3f

…ntModel to es6 syntax

Change fragmentModel storage context from schedule controller to medi…

8d10efe

…a type so I may persist the model through track and period changes. Major es5 cleanup and some notes.

This was referenced Aug 2, 2016

Last segment doesn't get appended at times if we seek #1205

Closed

ThroughputRule wrong calculation and duplicate loads of same fragment #1514

Closed

Refactor Virtual Buffer #1365

Closed

delete virtual buffer

d01fdea

davemevans reviewed Aug 2, 2016
View reviewed changes

boushley reviewed Aug 2, 2016
View reviewed changes

Made priority more clear with const, explicit insert and removed sort…

97673f7

… for priority added default param, refactored a bit and added off for new listener in schedule controller.

dsparacio mentioned this pull request Aug 4, 2016

Playing unexpectedly stops before end #1258

Closed

dsparacio merged commit 9150dd2 into Dash-Industry-Forum:development Aug 5, 2016

dsparacio deleted the #1365 branch August 9, 2016 17:07

spiterikevin added a commit to spiterikevin/dash.js that referenced this pull request Sep 27, 2016

check for replacement using startTime not index (see Dash-Industry-Fo…

f4fcb9c

…rum#1514 Dash-Industry-Forum#1523)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major Refactor reduces complication, increases dependability and solve critical bugs. #1523

Major Refactor reduces complication, increases dependability and solve critical bugs. #1523

dsparacio commented Aug 2, 2016

dsparacio commented Aug 2, 2016

davemevans Aug 2, 2016

dsparacio Aug 2, 2016

boushley Aug 2, 2016

boushley Aug 2, 2016

davemevans commented Aug 2, 2016

dsparacio commented Aug 2, 2016

boushley Aug 2, 2016

dsparacio Aug 2, 2016

boushley Aug 2, 2016

boushley commented Aug 2, 2016

dsparacio commented Aug 4, 2016

boushley commented Aug 4, 2016

dsparacio commented Aug 4, 2016

boushley commented Aug 5, 2016

Major Refactor reduces complication, increases dependability and solve critical bugs. #1523

Major Refactor reduces complication, increases dependability and solve critical bugs. #1523

Conversation

dsparacio commented Aug 2, 2016

dsparacio commented Aug 2, 2016

davemevans Aug 2, 2016

Choose a reason for hiding this comment

dsparacio Aug 2, 2016

Choose a reason for hiding this comment

boushley Aug 2, 2016

Choose a reason for hiding this comment

boushley Aug 2, 2016

Choose a reason for hiding this comment

davemevans commented Aug 2, 2016

dsparacio commented Aug 2, 2016

boushley Aug 2, 2016

Choose a reason for hiding this comment

dsparacio Aug 2, 2016

Choose a reason for hiding this comment

boushley Aug 2, 2016

Choose a reason for hiding this comment

boushley commented Aug 2, 2016

dsparacio commented Aug 4, 2016

boushley commented Aug 4, 2016

dsparacio commented Aug 4, 2016

boushley commented Aug 5, 2016