-
-
Save ThomasBurleson/7576083 to your computer and use it in GitHub Desktop.
| if (downloadRatio < 1.0) { | |
| self.debug.log("Download ratio is poor."); | |
| if (current > 0) { | |
| self.debug.log("We are not at the lowest bitrate, so switch down."); | |
| self.manifestExt.getRepresentationFor(current - 1, data).then( | |
| function (representation1) { | |
| self.manifestExt.getBandwidth(representation1).then( | |
| function (oneDownBandwidth) { | |
| self.manifestExt.getRepresentationFor(current, data).then( | |
| function (representation2) { | |
| self.manifestExt.getBandwidth(representation2).then( | |
| function (currentBandwidth) { | |
| switchRatio = oneDownBandwidth / currentBandwidth; | |
| self.debug.log("Switch ratio: " + switchRatio); | |
| if (downloadRatio < switchRatio) { | |
| self.debug.log("Things must be going pretty bad, switch all the way down."); | |
| deferred.resolve(new MediaPlayer.rules.SwitchRequest(0)); | |
| } else { | |
| self.debug.log("Things could be better, so just switch down one index."); | |
| deferred.resolve(new MediaPlayer.rules.SwitchRequest(current - 1)); | |
| } | |
| } | |
| ); | |
| } | |
| ); | |
| } | |
| ); | |
| } | |
| ); | |
| } else { | |
| self.debug.log("We are at the lowest bitrate and cannot switch down, use current."); | |
| deferred.resolve(new MediaPlayer.rules.SwitchRequest(current)); | |
| } | |
| } else { | |
| self.debug.log("Download ratio is good."); | |
| self.manifestExt.getRepresentationCount(data).then( | |
| function (max) { | |
| max -= 1; // 0 based | |
| if (current < max) { | |
| self.debug.log("We are not at the highest bitrate, so switch up."); | |
| self.manifestExt.getRepresentationFor(current + 1, data).then( | |
| function (representation1) { | |
| self.manifestExt.getBandwidth(representation1).then( | |
| function (oneUpBandwidth) { | |
| self.manifestExt.getRepresentationFor(current, data).then( | |
| function (representation2) { | |
| self.manifestExt.getBandwidth(representation2).then( | |
| function (currentBandwidth) { | |
| } | |
| ); | |
| } | |
| ); | |
| } | |
| ); | |
| } | |
| ); | |
| } else { | |
| self.debug.log("We are at the highest bitrate and cannot switch up, use current."); | |
| deferred.resolve(new MediaPlayer.rules.SwitchRequest(max)); | |
| } | |
| } | |
| ); | |
| } |
I value the many long conversations I've had with you where we've whittled away at a problem to find the most elegant solution, and there is some pretty concrete evidence above that I agree with the value of suggesting improvements. My concern just relates to the venue and manner in which one offers unsolicited advice and how that might be received.
Digital Primates is in the midst of a high profile promotional campaign for this library (which they developed in collaboration with Microsoft) that includes that blog article and a presentation in partnership with Microsoft, Google and Akamai at Streaming Media West last week. I'm excited to see people we know playing a big role in moving the web forward.
Since we know Jeff and many of the Digital Primates crew, and are used to sharing ideas amongst our tribe of Adobe Flex refugees, it's easy to miss the context here. The fact is, the last thing they need in the midst of a project launch is a bunch of their colleagues initiating an unsolicited critique of a subset of their code in the midst of their virtual press conference moment.
I wanted to address the concern that this discussion (and the manner in which it originated) might be misconstrued as a kind of "me, too" brand of self-promotion. I didn't want to contribute in publicly piling on in a critique of their code without at least acknowledging these concerns and clarifying intent.
I think I speak for both of us when I say, (in case there was any doubt) we know full well how much effort they must have put into dash.js. We share many of the same battle scars, so we're fans of their team and want them to be successful.
Returning to the topic of discussing ways we would hone this code into a polished gem:
Be careful - methods aren't typically bound to their instance scope in JavaScript.
When passing manifestExt.getBandwidth() to then(), you are really passing a reference to the getBandwidth() function which will be run in the global object scope (ex. window). That might be fine for a static function, but will quickly backfire if you call a method function that relies on instance state.
Where possible, your version does read better. This reminds me of a discussion I had earlier this year with the SpatialKey guys where a couple of them were advocating the value of including automatic instance scope binding for methods as part of a class / module factory helper; that approach eliminates an entire class of errors (no pun intended).
I like that you expanded the scope of your example to include those other conditions (upgrade, downgrade, etc.) - your new example really demonstrates how much all of these suggestions trim down the original checkIndex() method and clarify its operation.
There are only two hard things in Computer Science: cache invalidation and naming things.
-- Phil Karlton
I'm not terribly keen on the closure names here.
calculateUpgrade() implies it returns a calculation, not a SwitchRequest.
requestUpgrade() and requestDowngrade() imply they perform the action of requesting an upgrade or downgrade, but in fact return a SwitchRequest.
I still think these should be promoted from inline closures to (internal) methods of the module. There's no necessity to recreate these on every execution of the checkIndex method, and there are only downsides in doing so (increased memory usage, more work for the garbage collector, slower performance, etc.).
I also think the ternary operator use is not promoting clarity. Three nested ternary expressions are a pretty good indication that the calculation needed to initialize a variable is complex enough it should probably be extracted into its own function. Repeating the same nested ternary expression twice in immediate succession means that the same expressions have to be unnecessarily reevaluated, and makes it far less readable and obvious that they're both implementations of the same conditional branches.
I think by the end of this we're going to have some great material for a pull request.
Getting close:
MediaPlayer.rules.DownloadRatioRule = function () {
"use strict";
var self = null,
noop = function() { },
$log = null, // updated during self.checkIndex()
getRepresentation = null, // updated during self.checkIndex()
getBandwidth = null, // updated during self.checkIndex()
/**
*
*/
getIndexedBandwidth = function( index, data )
{
$log("getIndexedBandwidth( "+ index + " )");
return getRepresentation( index, data ).then( getBandwidth );
},
/**
*
*/
getMaxBandwidth = function( )
{
$log("getMaxBandwidth()");
return getRepresentation( )
.then( function(last)
{
return max - 1; // zero-index
};
},
/**
*
*/
checkRatio = function (newIdx, currentBandwidth, data)
{
$log("checkRatio()");
return getIndexedBandwidth( newIdx, data)
.then( newBandwidth )
{
return (newBandwidth / currentBandwidth);
});
},
/**
*
*/
calculateTimesAndRatios = function( metrics )
{
var DOWNLOAD_RATIO_SAFETY_FACTOR = 0.75,
httpRequests = metrics.HttpList,
numRequests = httpRequests ? httpRequests.length : 0,
lastRequest = numRequests ? httpRequests[numRequests - 1] : null,
info = {
totalTime : lastRequest ? (lastRequest.tfinish.getTime() - lastRequest.trequest.getTime()) / 1000 : 0,
downloadTime : lastRequest ? (lastRequest.tfinish.getTime() - lastRequest.tresponse.getTime()) / 1000 : 0,
totalRatio : lastRequest ? lastRequest.mediaduration / totalTime : NaN,
downloadRatio : lastRequest ? (lastRequest.mediaduration / downloadTime) * DOWNLOAD_RATIO_SAFETY_FACTOR : NaN
};
$log("calculateTimesAndRatios() - Checking download ratio rule...");
if (!metrics) {
$log("No metrics, bailing.");
info = null;
} else if ( numRequests === 0) {
$log("No requests made for this stream yet, bailing.");
info = null;
} else if (info.totalTime <= 0) {
$log("Don't know how long the download of the last fragment took, bailing.");
info = null;
} else if ( !lastRequest.mediaduration || lastRequest.mediaduration <= 0) {
$log("Don't know the duration of the last media fragment, bailing.");
info = null;
} else if ( isNaN(downloadRatio) || isNaN(totalRatio) ) {
$log("Total time: " + totalTime + "s");
$log("Download time: " + downloadTime + "s");
$log("The ratios are NaN, bailing.");
info = null;
}
$log("Total ratio: " + totalRatio);
log("Download ratio: " + downloadRatio);
return info;
},
/**
*
*/
calculateUpgradeSwitch = function( downloadRatio, current, max, data )
{
$log("calculateUpgrade()");
var calculateUpgradeIndex = function (ratios)
{
var i, len;
for ( i = 0, len = ratios.length; i < len; i += 1)
{
if (downloadRatio < ratios[i]) {
break;
}
}
return i;
}),
ratios = [ ];
while ((i += 1) < max) {
ratios.push( checkRatio( i, current, data ));
}
return Q.all( ratios )
.then( calculateUpgradeIndex )
.then( function( index )
{
$log("Calculated ideal upgrade quality index == " + index);
return new MediaPlayer.rules.SwitchRequest( index );
});
},
/**
*
*/
requestDowngradeSwitch = function (downloadRatio, currentIndex, data)
{
$log("requestDowngrade()");
return Q.all([
getIndexedBandwidth( currentIndex - 1, data ),
getIndexedBandwidth( currentIndex, data )
])
.then( Q.spread(function (previous, current)
{
var switchRatio = previous / current,
message = (downloadRatio < switchRatio) ?
"Things must be going pretty bad, switch all the way down." :
"Things could be better, so just switch down one index.",
newIndex = (downloadRatio < switchRatio) ? 0 : currentIndex - 1;
$log( "Switch ratio: ", switchRatio );
$log( message );
return new MediaPlayer.rules.SwitchRequest( newIndex );
}));
},
/**
*
*/
requestUpgradeSwitch = function(downloadRatio, currentIndex, data )
{
$log("requestUpgrade()");
return Q.all([
getIndexedBandwidth( currentIndex, data ),
getIndexedBandwidth( currentIndex + 1, data ),
getMaxBandwidth()
])
.then( Q.spread(function (current, next, max )
{
var switchRatio = next / current,
newIndex = downloadRatio < switchRatio ? current :
downloadRatio > 1000.0 ? max :
downloadRatio > 100.0 ? next : -1,
message = downloadRatio < switchRatio ? "Not enough bandwidth to switch up." :
downloadRatio > 1000.0 ? "Tons of bandwidth available, go all the way up." :
downloadRatio > 100.0 ? "Just enough bandwidth available, switch up one." :
"Not exactly sure where to go, so do some math.";
$log( "Switch ratio: ", switchRatio );
$log( message );
return (newIndex < 0) ?
calculateUpgradeSwitch( downloadRatio, current, max, data ) :
new MediaPlayer.rules.SwitchRequest( newIndex );
}));
};
// ******************************************
// Publis DownloadRatioRule instance and API
// ******************************************
return self = {
/**
* Post-construction injection to logger reference
*/
debug : undefined,
/**
* Post-construction injection to Manifest model
*/
manifestExt : undefined,
/**
* Check QoS to determine if upgrade or downgrade is warranted...
*/
checkIndex : function (current, metrics, data)
{
var deferrred = Q.defer(),
info = null;
// Dynamic update specific closure variables
$log = ( self.debug && self.debug.log ) ? self.debug.log : noop;
getRepresentation = _.bind( self.manifestExt.getRepresentationFor, self.manifestExt ),
getBandwidth = _.bind( self.manifestExt.getBandwidth, self.manifestExt ),
$log( "DownloadRatioRule::checkIndex()" );
info = calculateTimesAndRatios( metrics );
if ( !info )
{
deferred.resolve (
// @TODO - is this correct ?
new MediaPlayer.rules.SwitchRequest( );
);
} else {
if (info.downloadRatio < 1.0)
{
$log("Download ratio is poor.");
if (current > 0)
{
$log("We are not at the lowest bitrate, so switch down.");
deferred.resolve(
requestDowngradeSwitch( info.downloadRatio, current, data )
);
} else {
$log("We are at the lowest bitrate and cannot switch down, use current.");
deferred.resolve(
new MediaPlayer.rules.SwitchRequest( current )
);
}
} else {
$log("Upgrade possible since download ratio is good.");
deferred.resolve(
requestUpgradeSwitch( info.downloadRatio, current, data )
);
}
}
return deferred.promise;
}
};
};
MediaPlayer.rules.DownloadRatioRule.prototype = {
constructor: MediaPlayer.rules.DownloadRatioRule
};
Thanks for this example, Thomas. Keep up the good work, I don't think any of this was disrespectful to the people behind Dash.js or could be misconstrued as such -- I'm not sure John was a neutral party in this discussion, but his input certainly got the ball rolling.
Thanks to @johnyanarella for his provocative ideas with
Q.all( )andQ.spread( ). Here is the revised version with an alternate flattening:Obviously the secret to this solution is
deferred.resolve( )resolving itself with a promise [chain]. Nice.