-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#745 Video playback quality watcher #907
Conversation
…hing switch_ calls upon lower device capacity detected.
Note that there are a few debug logs in there, and no docs yet, linting/compile will surely fail as well. Only tested in uncompiled mode. This is to give a base, to see if this approach gets positive resonance, and if yes, docs and format will be added. |
in order to test this and cause some frame-dropping, i can recommend the |
In general, this is a good idea; thanks for the contribution. But I don't think we should be guessing the capabilities of the device explicitly. Since a number of factors can influence dropped frames, it would be better to treat this the same as the network. Namely, if we have too many dropped frames, we downgrade. If we don't have any dropped frames for a while, we can allow upgrades again. You may want to use the You could also use the |
@TheModMaker quoting you: In case I did understand what you mean, it is in line with what we are doing in this PR. More precisely, how do you suggest should we abstract away from the specific device capabilities? I understand we should not set fixed max-pixel values. The defaults in this PR were kind of an p-o-c example. How about one would allow to lib clients to input as configuration a set of pixel-values, over which we up and downgrad as you suggest. For this we already have added here the concept of Generally, the dropped-frames detection and the handling should be disabled by default, it might cause unexpected behavior - but when running this with specific content like full-HD and 4K streams on a range of devices that may have issues with decoding, it would be great to be able to configure the behavior to ones needs and enable the watch-interval. The idea of using a moving average for smoothing the dropped-frames measurements is a good one I agree! That's exactly what I meant by adding more advanced means of generating a decision-driving signal. Also, was thinking the same about |
What I mean is that we should not have fixed pixel sizes at all. It would be better to adapt based on the device rather than just dropping all UHD streams. So rather than having explicit regions, I suggest adjusting to the next lower track. I would suggest a type to track an estimate for what the device supports, similar to how the bandwidth estimate works. If we start dropping frames, then the estimate should go down below what is being played. If we are not dropping frames, increase the estimate some (being sure to remember old data to avoid switching back and forth). This will result in a dynamic estimate for what the device supports. Then Also, I don't think we should give it |
Yes that makes much more sense and will universally work from a control systems pov. We then also need to have the exponential smoothing in there to make it stabilize. Also agreeing with all your other suggestions. Thanks for the valuable input on the solution here. |
i've fixed the linter and test issues and i've tested as compiled mode, as far as i test i works as expected. i'm still continuing to test, i will try to inform you about updates |
yes but we should move this towards the direction as discussed with @TheModMaker here. his comments make more sense than my initial approach here. I will soon modify it in the direction of not restricting on preset resolutions, but rather have a control-system that switches bitrate up/down upon extended period of frame-drops detected by the exponential moving average. @TheModMaker I figured the smoothing filter will also be good to cancel out high short spikes that might happen on any machine when one moves around windows or other stuff that happens in the background. we wouldn't want to switch down on that but only if there is a prolonged dropped fps rate over a certain time that does not go away. @hyurtseven81 great if you fixed linter things! could you commit that please? :D 👍 |
…dropped-frames-detector
…redundant definition of criteria
… to optimize quality
Just wanted to add a note for this dropped frames ABR feature. In Edge it seems like dropped and total frames from |
@forbesjo interesting. that is something we could eventually fix via the polyfill (which we already use in this library to enable webkit-prefixed dropped/decoded frame-count) by keeping a count in a reserved property in the video element itself. but then again, we may not necessarily need it, there should be some kind of differential sampling of these counters going on, so that a reset should not matter to the overall estimate. especially if there is a seek happening, potentially we are starting on a flushed buffer and can assume different payload properties yielding different decoder behavior. |
generally about the solution to this: the previous approach here is not yet what I think is what we should do. there will in fact rather be a scalar dimension for "video quality" that should be proportional to the number of decoded frames and the total pixels decoded per frame. when frames are dropped, the quality estimate will drop (smoothed by the ewma, just like for the bandwidth). then the ABR algorithm should optimize for highest quality possible, just like it already optimizes for highest bandwidth possible. to do so it will have to choose resolutions such that they fulfill a condition against the quality estimate (anologous to how we choose variants by the bandwidth estimate). we are currently working on a such a direction for a solution to this. |
Are there any plans to include this functionality into release in the near future ? |
Hi all, When i'm waiting this feature added to core, i implemented a drop frame solution but i need your help and advise about the idea and implementation, i'm sending the code to sum, i create a 5 seconds interval, and check the current period drop frame percentage if this period exceeds the threshold i limit the max resolution, i tried the cool down period implementation but when i test on a low-end machine i saw the following behaviour: Player starts playing on high bitrate drop frames start and the following function limits the max height and the bitrate the lower than max bitrate player can play video without any drop frame and cooldown period starts after that drop frames starts again. so my opinion cooldown period can be useless. ` var droppedFramesHistory_ = [];
} catch (e) { ` |
@hyurtseven81 thank you for sharing. I would just change one thing:
into:
This way player do buffer clean automatically and also do not drop other variants from its list, but only do not take them into consideration during abr selection (so user still can force to select higher resolution). I'll check this, test in some A/B scenarios and share some results. |
@bitactive Thank you for your reply, i'll try to change the line that you suggested and i'm waiting your test results. |
I'm sorry that we have fallen behind in reviewing pull requests. I promise to take a look at this as soon as I can make time. Please accept my apologies for keeping you waiting for so long. |
Hi @joeyparrish - i think this is a critical issue especially in chrome DRM when it not using the GPU we see stalls due to drop frames. |
@itaykinnrot +1 exactly what we observed with many HD+ and 4K streams, CDM processing creates a bottle-neck on certain hardware configs... It would be good if Shaka could detect this, and generally solve the problem of "hardware capabilities adaptation" - while browser API that tells us better about CPU / GPU resources still needs to be spec'd :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still something you are interested in? If so, please rebase on the latest master
and fix any linter errors for our new style guidelines. Note that all the new files need to have copyright headers and need to be added to one of the files in build/types
(probably core
).
I think having the EwmaVideoQualityEstimator
is a good idea (maybe squash the public interface into a single getSmoothness
method?), but it may be better as part of SimpleAbrManager's suggestStreams_ logic. So that method would determine the current smoothness or expected highest quality and use that to adjust the possible choices.
* @exportDoc | ||
*/ | ||
shakaExtern.AbrManager.prototype.init = function(switchCallback) {}; | ||
shakaExtern.AbrManager.prototype.init = function(switchCallback, videoEl) {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to not require passing in the video element to AbrManager. What the app can do is pass a closure to player.configure
that will capture the current video element and pass it to their own constructor. This is what we do for the TextDisplayer.
For the Player's default abrFactory
, you could set it to shaka.abr.SimpleAbrManager.bind(null, this.video_)
. See how we do this for textDisplayFactory.
/** | ||
* A fast-moving average. | ||
* Half of the estimate is based on the last `FAST_WINDOW` seconds of sample history. | ||
* @private {!Ewma} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs the full type shaka.abr.Ewma
.
goog.require('shaka.abr.Ewma'); | ||
goog.require('shaka.log'); | ||
|
||
var FAST_WINDOW = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please namespace global constants. (you also can't use var under our new style rules)
shaka.abr.EwmaVideoQualityEstimator.FAST_WINDOW = 1;
@@ -0,0 +1,71 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs a copyright header.
/** @private {shaka.abr.EwmaVideoQualityEstimator} */ | ||
this.videoPlaybackQualityWatcher_ = null; | ||
|
||
this.videoElem_ = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fields need a type using either @type
for public fields or @private
for private ones.
@@ -103,18 +107,32 @@ shaka.abr.SimpleAbrManager.prototype.stop = function() { | |||
|
|||
// Don't reset |startupComplete_|: if we've left the startup interval then we | |||
// can start using bandwidth estimates right away if init() is called again. | |||
|
|||
clearInterval(this.videoQualitySampleTimer_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a utility class for this. shaka.util.Timer
initialBandwidth, samplingPeriodMs, onUpdate) { | ||
|
||
if (typeof videoEl.getVideoPlaybackQuality !== "function") { | ||
throw new Error("Video element has no getVideoPlaybackQuality function"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, you should only throw shaka.util.Error
(the compiler will yell at you). Second, it would be better to use feature detection and avoid using this type instead of throwing.
/* This is needed because MediaElement.seeking is non-standard prop */ | ||
this.seeking_ = false; | ||
|
||
this.videoEl_.addEventListener("seeking", function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a utility class for event listeners, shaka.util.EventManager
.
@TheModMaker Hey, yes, this is an actual issue, and I would like to continue to work on a solution for it. I hope thanks to your reviewing we can get it to an actual patch to master this time 👍 I had interrupted these efforts as there was a lack of response from the maintainers side here from my pov. |
This is badly out of date, which is our fault for not reviewing it in a timely manner. I'm going to close it now, but please feel free to rebase and/or reimplement on the latest code and send a new PR. Thanks! |
@joeyparrish understood & agreed :) it is also due on our side: we haven't seen the need to pursue the idea as far and/or merge it into upstream Shaka version at the time (in the end). which is really why back then i may have not continued to provide necessary changes to progress in the review. no worries and thanks for the feedback provided so far, and the clear communication right now! best regards!! |
Taking note of #745, I'd like to give a first shot at how we could deal with adaptation related to device capabilities.
We have added such a thing to our product, but it would be great to contribute this upstream to have it in a great shape and well maintained. Also I think the more general purpose we make this solution, the better it will probably work for us as well.
We added an object called
DroppedFramesDetector
. It takes aStatsProvider
to call upon thegetStats
function of thePlayer
or get dropped-frames in some way. It periodically checks the current dropped-frames-per-seconds average on a given time-window.A lot of magic constants and opinions can be found in there, all of this could be made configurable i.e is subject to feedback. It's kept very simple intentionally. One could specify more complex detection algorithms and override it in the future, in the same way as it is done for bandwidth estimation elsewhere.
The key is to quantify somehow the device capacity to decode frames in time with a scalar.
Then we map this scalar back to a maximum pixel height, which is then applied to the ABR manager's restrictions.
In case a level-down is detected, we are doing a flushing switch to immediatly help the resources catch up with the workload and eventually lower the total pixels to decode.
The way the mapping is done, and how the levels are determined could be customizable. But I guess it would be important to find a good ground that generally will work.
Now, also one might want to have a cool-down period as sometimes a CPU might get overflowed with work and frames will get dropped, even if it's a device with good resources. Therefore to optimize the QoE one would want to switch up again after the average dropped-fps have not exceeded the heat-up threshold for the given cooldown period.
Similar to other ABR settings for bandwidth estimation, one can choose a low cooldown period and may have a better average quality, but might risk to switch up too high qualities for the CPU capacity, as an analogy to bandwidth estimation where one may smooth more or less the estimation.
Finally, I would say it is important to deactivate the cooldown when playback is buffering or paused!
And especially stop the whole detection of average dropped frames when paused or buffering in fact.
Otherwise it will take a decision based on decoding zero fps, which would be wrong :)
For the last point, we need an interface to the ABR manager where it detects playhead state (buffering/playing/paused...) in order to ensure this.