Improvement ideas for evaluateQueueSize
- Dominant language
- Java
- Stars
- 21.9k
- Forks
- 6k
- PR merge metrics
- No merged PRs in 30d
Description
### [REQUIRED] Issue description
In testing of the code to discard samples on upshift, from this pull request: https://github.com/google/ExoPlayer/pull/7244 I've discovered what I believe are several issues with the implementation.
One of the issues is an exception that gets triggered due to an index out of bounds, which I filed in a separate bug: https://github.com/google/ExoPlayer/issues/7580
In this issue report I'd like to discuss some of the other things I've found and whether there's a planned solution or if there are thoughts of how to address the problem. I've looked into some possibilities and I've implemented one which seems to work OK, though has a drawback of delaying the upshift by one segment.
I'll document the various issues here in this bug report - if it's preferred to have separate issue reports, I'd be happy to split this report up and file additional reports.
ISSUE 1)
I believe the core issue is that AdaptiveTrackSelection::evaluateQueueSize() and AdaptiveTrackSelection::updateSelectedTrack() are called at different times and use different algorithms to determine if an upshift is going to occur. The result is that there are times when evaluateQueueSize() determines that it thinks an upshift is going to occur and proceeds to effectively dump chunks (by reducing the returned queue size), but then when updateSelectedTrack() runs it decides not to perform an upshift. This results in discarding samples and then re-downloading the same samples which were discarded. And in a related vein, while I still need to evaluate the algorithms more closely, it appears that the act of discarding samples ends up impacting the updateSelectedTrack() selection, making it less likely to upshift.
As a temporary fix/test for this issue, what I did was I changed updateSelectedTrack such that when it detects it should upshift, it just sets a flag indicating an upshift should occur and what track it plans to switch to, but does not immediately upshift. Then in the evaluteQueueSize(), if there is a pending upshift, it goes through its discard logic, but otherwise just returns the normal queue size. When updateSelectedTrack() runs again, if there's a pending upshift, it executes it and resets the flag. This seems to work pretty well, though with the obvious impact of delaying the upshift.
ISSUE 2)
I believe there are a couple issues in evaluateQueueSize() which can generally/broadly be categorized as issues with when to evaluate the samples and which samples to remove.
The main issue here, I believe is that it doesn't check whether the idealSelectedIndex is actually an upshift as compared to the current track selection. Instead it goes through all of the chunks and anything lower than the idealSelected triggers a discard. What can happen (and what I've seen happen) is that the idealSelected is either the same or lower quality than the current track selection, but there is at least one chunk in the queue that's of lower quality than ideal, and that triggers a discard. Admittedly I'm more likely to see this because of the stress scenario I've set up. As I'll describe in more detail later, I've set a bandwidth limit on my access point that causes the algorithm to oscillate pretty regularly and frequently between different tracks. In this scenario, I've often seen cases with chunk[0]=track1 chunk[1]=track3 chunk[2]=track2 chunk[3]=track2 chunk[4]=track1. Where track1 is high resolution, track2 medium and track3 low. So when we go into evaluateQueueSize() if it decides the ideal is track2(medium), even though we're currently on track1(high), which means we're doing a downshift, we'll still go through the queue. And in this case, since chunk[1] is low (and thus less than medium) it indicates we should discard everything from chunk[1] to the end.
The simple/easy fix here is to only evaluate the chunks in the queue if ideal is > current. This of course also gets "fixed" using the delay-upshift algorithm I described above, but since that has the cost of delaying the upshift, it sure would be nice to have a better solution.
ISSUE2.1)
I also believe it would be better to evaluate the queue starting from the end and working toward the beginning. Even if we are legitimately upshifting tracks it's possible, with a queue that contains a mix of different qualities, that we'll end up dumping samples we probably don't want to dump. Whereas if we evaluate starting at the end of the queue we should only be dumping samples that we really would like to dump.
ISSUE 3)
In the grand scheme of things, this is really a minor nit and is somewhat more on the coding style side of things. In updateSelectedTrack(), the selectedIndex class variable is directly manipulated/changed even in cases where it may be changed back by the end of the method. Assuming no multi-threading/protection issues, this is "OK" but seems like it's just begging for someone to add code somewhere, assuming that selectedIndex is pointing to the currently-selected track when it may not actually be. It seems it would be far safer to have the new track selection be a local variable and only update the class variable once we're sure that's going to be the selection.
### [REQUIRED] Reproduction steps
This shows up primarily on live streams when playing at the live point. This is because updateSelectedTrack() goes through some additional buffer health checks after getting the index from determineIdealSelectedIndex() and these health checks are more likely to reject the upshift when playing at the live point. This results in the two sections of code making different decisions on whether an upshift is indicated.
To exacerbate the problem, and make it more common and visible, artificially limit the bandwidth to the device in a way that it will tend to upshift/downshift frequently. In my particular setup, I've set a bandwidth limit of 10Mbps while viewing a stream that has 6Mbps, 4Mbps and 2Mbps variants. In this scenario, it often oscillates between the 6 and 4, but sometimes also goes down to the 2 for short periods.
Also, to help exercise the evaluateQueueSize() code and the discarding, I've disabled the resolution checks and only base it on the bitrate.
Environment:
- client device: TiVo Stream 4k
- wireless connection is intentionally set to be close to the threshold for the stream. In this particular case, the stream (a live stream) has 6Mbps, 4Mbps and 2Mbps variants and the access point is limiting the connection to 10Mbps
- CODE CHANGE: in part because this stream uses the same resolution, and also because we'd like to eventually have the discard be evaluated for every shift to a higher bitrate (not just higher resolution) I disabled the checks for resolution and only evaluate bitrate when determining whether to discard samples. i.e., in AdaptiveTrackSelection.java:
// If the chunks contain video, discard from the first SD chunk beyond
// minDurationToRetainAfterDiscardUs whose resolution and bitrate are both lower than the ideal
// track.
for (int i = 0; i < queueSize; i++) {
MediaChunk chunk = queue.get(i);
Format format = chunk.trackFormat;
long mediaDurationBeforeThisChunkUs = chunk.startTimeUs - playbackPositionUs;
long playoutDurationBeforeThisChunkUs =
Util.getPlayoutDurationForMediaDuration(mediaDurationBeforeThisChunkUs, playbackSpeed);
if (playoutDurationBeforeThisChunkUs >= minDurationToRetainAfterDiscardUs
&& format.bitrate < idealFormat.bitrate
/* && format.height != Format.NO_VALUE && format.height < 720
&& format.width != Format.NO_VALUE && format.width < 1280
&& format.height < idealFormat.height*/) {
### [REQUIRED] Link to test content
At the moment we are unable to make our video feed public. We are working on ways to address this.
### [REQUIRED] A full bug report captured from the device
I will capture the bug report and email it.
### [REQUIRED] Version of ExoPlayer being used
Branch of dev-v2 at: commit dac3dde7bb8d79e785f65b0c0c7b7425f41dffa6 (dev-v2)
### [REQUIRED] Device(s) and version(s) of Android being used
Primary device used for reproduction and testing is a Tivo Stream 4K running android:
dwc$ adb shell getprop ro.build.version.release
9
dwc$ adb shell getprop ro.build.version.sdk
28
Contributor guide
Assessment
This issue has not been assessed yet.