Provide better fix for the set{Video,Audio}Bitrate issue #1271
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I fixed an issue which led to manual bitrate setting performed by calling the
setVideoBitrate
orsetAudioBitrate
RxPlayer API still overwritable by our buffer-based adaptive logic in #1269.However I wasn't too happy with that fix because going from a manual bitrate setting to an adaptive one (e.g. when going back to that mode) would necessitate to await that a new segment be pushed before relying on the buffer-based logic, whereas it could theoretically switch to it right away (leading to a potentially better quality right away, as we always take the best quality between what the different adaptive algorithms in place estimate).
Here, I improve the code so going back into adaptive mode leads directly to the estimated best quality.
The idea is to continue producing buffer-based estimates even when a bitrate is forced manually (this is actually what we already did for bandwidth estimates), but to only rely on them when the bitrate isn't forced. To do that, I simply moved the code actually returning the wanted manual bitrate setting in the same code which regularly produces estimates (it was handled much earlier before, thus short-circuiting much more adaptive-related code).
This was done like this because it leads IMO to very simple to follow code: when a new estimate may be produced, we just check if the bitrate is forced manually and if it is, we just return it. When doing this that way, all the rest of the adaptive logic still runs, even when we don't really need it, though it doesn't seem like it should be an issue.
I found it much easier to follow and reason about than the previous implementation which considered a manual bitrate as a special case where some things run and some things don't.
Even better, it just mirrors the implementation of other API such as
maxVideoBitrate
orminVideoBitrate
- which I would guess would be considered close technically to amanualVideoBitrate
setting, at least to an RxPlayer developer.I also updated another thing which I didn't need to keep in the end, but I still did: the
BufferBasedChooser
(module implementing the buffer-based adaptive algorithm) before had one method,getCurrentEstimate
, to which you both communicated information on a just-pushed segment and got the current estimate deduced from the resulting current buffer's size.I found it more understandable as an API to decorrelate the two. Now there are two methods exposed by the
BufferBasedChooser
:onAddedSegment
: to call when a segment has just been pushed. This also better communicates that this method is supposed to be called directly after pushing a segment, as it contains timing-related code.getLastEstimate
: which returns the current last estimate produced by theBufferBasedChooser
. This means that it is now theBufferBasedChooser
's role to store it - which makes IMO the central adaptive code which uses it more legible.