Skip to content
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

Provide better fix for the set{Video,Audio}Bitrate issue #1271

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Aug 22, 2023

I fixed an issue which led to manual bitrate setting performed by calling the setVideoBitrate or setAudioBitrate 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 or minVideoBitrate - which I would guess would be considered close technically to a manualVideoBitrate 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 the BufferBasedChooser. This means that it is now the BufferBasedChooser's role to store it - which makes IMO the central adaptive code which uses it more legible.

I fixed an issue which lead to manual bitrate setting performed by
calling the `setVideoBitrate` or `setAudioBitrate` 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) would necessitate to
await that a new segment be pushed before relying on the buffer-based
logic, whereas it could 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` or `minVideoBitrate` - which I would guess would be
considered close technically to a `manualVideoBitrate` 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 the `BufferBasedChooser`. This means that it is now the
    `BufferBasedChooser`'s role to store it - which makes IMO the
    central adaptive code which uses it more legible.
@peaBerberian peaBerberian added ABR Relative to adaptive streaming (Adaptive BitRate) Priority: 1 (High) This issue or PR has a high priority. labels Aug 22, 2023
@peaBerberian peaBerberian added this to the 3.31.1 milestone Aug 22, 2023
@peaBerberian peaBerberian force-pushed the fix/setVideoAudioBitrate-better-fix branch 2 times, most recently from 7fe2d18 to 99f6695 Compare August 22, 2023 15:57
@peaBerberian peaBerberian force-pushed the fix/setVideoAudioBitrate-better-fix branch from 99f6695 to f2cf1bb Compare August 22, 2023 15:58
@peaBerberian peaBerberian merged commit e8bf1ab into master Aug 23, 2023
4 checks passed
@peaBerberian peaBerberian modified the milestones: 3.31.1, 3.32.0 Sep 12, 2023
@peaBerberian peaBerberian mentioned this pull request Oct 2, 2023
@peaBerberian peaBerberian deleted the fix/setVideoAudioBitrate-better-fix branch February 7, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABR Relative to adaptive streaming (Adaptive BitRate) Priority: 1 (High) This issue or PR has a high priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant