Add custom format comparator support to AdaptiveTrackSelection#3068
Add custom format comparator support to AdaptiveTrackSelection#3068AndyWangLYN wants to merge 3 commits intoandroidx:releasefrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
AndyWangLYN
force-pushed
the
codex/heyu_wang-adaptive-format-comparator-fallback
branch
from
df1bdf0 to
d29702f
Compare
|
hey guys can I get some feedback on this PR? Any feedback can do. Very much appreciated! @tonihei @oceanjules |
|
Also raised a question regarding this: #3077 |
| previousReason = Iterables.getLast(queue).trackSelectionReason; | ||
| } | ||
| int newSelectedIndex = determineIdealSelectedIndex(nowMs, chunkDurationUs); | ||
| int newSelectedIndex = determineIdealSelectedIndexForEffectiveBitrate(nowMs, effectiveBitrate); |
There was a problem hiding this comment.
This method is still determining the ideal selected index by going through the ordered list of options and returning the first one where canSelectFormat returns true. Doesn't that mean we can keep the existing name of the method?
There was a problem hiding this comment.
Make sense, determineIdealSelectedIndex has been kept as-is now
| newSelectedIndex = previousSelectedIndex; | ||
| } else if (selectedFormat.bitrate < currentFormat.bitrate | ||
| } else if (newSelectedIndex > previousSelectedIndex | ||
| && (isUsingDefaultFormatComparator() |
There was a problem hiding this comment.
Can you add more details why you'd like to add this additional constraint? It seems the generic logic would still apply maxDurationForQualityDecreaseUs without further conditions.
Related to that - I think we should't have a method isUsingDefaultFormatComparator. If there is a different behavior needed, it would need to be in a method that describes the actual behavior and can be customized by everyone (not just the default logic).
There was a problem hiding this comment.
You're right on both points.
The deferral logic applies uniformly regardless of the comparator, so no special-casing is needed. I’ve removed isUsingDefaultFormatComparator entirely.
I also updated the switch-direction logic: With a custom comparator, the higher-priority track may not have a higher bitrate, so bitrate comparison isn’t reliable. Using index comparison makes the logic correct for any comparator while preserving the same semantics in the default case.
| return i; | ||
| } else { | ||
| } | ||
| int formatBitrate = format.bitrate == Format.NO_VALUE ? 0 : format.bitrate; |
There was a problem hiding this comment.
Similar to my other comment above, I'd assume this logic can be kept as it was before? If no format returns true for canSelectFormat, we just fall back to the lowest quality format that isn't excluded. The only thing that changes is the definition of 'quality'.
There was a problem hiding this comment.
Agreed, reverted to the original pattern
| } | ||
|
|
||
| @Test | ||
| public void initial_updateSelectedTrack_withInvalidComparator_fallsBackToDefaultOrdering() { |
There was a problem hiding this comment.
There is no reason to account for invalid Comparators. If custom app code provides an implementation of a well-defined interface, we should be able to assume they implemented the interface correctly.
| } | ||
|
|
||
| @Test | ||
| public void initial_updateSelectedTrack_usesFactoryProvidedTrackFormatComparatorOrder() { |
There was a problem hiding this comment.
Thanks for providing tests for your change!
There was a problem hiding this comment.
For sure! Thank you for recognizing!
AndyWangLYN
changed the title
2 participants
Summary
Adds optional custom format ordering for
AdaptiveTrackSelectionso apps can influenceABR selection priority beyond bitrate-only ordering. While default behavior remain unchanged
This PR is refined version of the PR: #3064 in which I received comments about track format sorting. Intend to close above PR if this one is deemed good eventually.
What changed
AdaptiveTrackSelection.Factory#setTrackFormatComparator(Comparator<Format>)to allow custom format ordering via the factory.
protected final getTrackFormatComparator()onFactoryso subclasses thatoverride
createAdaptiveTrackSelection()can access the configured comparator.protectedconstructor onAdaptiveTrackSelectionaccepting aComparator<Format>, enabling subclasses to pass a comparator directly.updateSelectedTrackto use indexcomparison instead of bitrate comparison, so it works correctly with any comparator
(lower index = higher priority).
DEFAULT_FORMAT_COMPARATORas a named constant inBaseTrackSelection.TrackSelectionand field Javadoc to say "selection order" instead of"decreasing bandwidth order".
Why
The current implementation always prioritizes higher bitrate. This makes it difficult to
express developer-defined rules (e.g. a quality score ranking) without subclassing and
duplicating ABR logic. These hooks allow custom ordering without touching core ABR
switching behavior.
Testing Done