feat: content steering switching

wseymour15 commented 1 year ago

Description

The changes include the logic for switching pathways regarding content steering. All specifications for both DASH and HLS can be found in the resources below. The function of these changes are to ensure that we are communicating with the content steering server, handling the responses, switching playlists based on this data, and ensuring that we follow the requirements of the spec when deciding when/where to switch.

Specific Changes proposed

Logic to determine which pathway/serviceLocation should be selected based on priority.
Logic to switch media in cases where we want to switch playlists.
Logic to exclude playlists when necessary according to the spec.
Logic to set timers to retry certain content steering requests.
Tests to cover specification

Example Manifests to Test Locally

DASH: https://fastly.content-steering.com/bbb/playlist_steering_fastly_https_cdn-a_cdn-c_cdn-b.mpd HLS: https://fastly.content-steering.com/bbb_hls/master_steering_fastly_https.m3u8

Specs

https://dashif.org/docs/DASH-IF-CTS-00XX-Content-Steering-Community-Review.pdf https://developer.apple.com/streaming/HLSContentSteeringSpecification.pdf

Requirements Checklist

[x] Feature implemented / Bug fixed
[ ] If necessary, more likely in a feature request than a bug fix
- [x] Unit Tests updated or fixed
- [ ] Docs/guides updated
- [ ] Example created (starter template on JSBin)
[ ] Reviewed by Two Core Contributors

codecov[bot] commented 1 year ago

Codecov Report

Merging #1427 (72af63a) into main (86d5327) will increase coverage by 0.17%. Report is 1 commits behind head on main. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1427      +/-   ##
==========================================
+ Coverage   85.75%   85.93%   +0.17%     
==========================================
  Files          42       42              
  Lines       10284    10414     +130     
  Branches     2374     2409      +35     
==========================================
+ Hits         8819     8949     +130     
  Misses       1465     1465

Files Changed	Coverage Δ
src/media-groups.js	`98.65% <ø> (-0.01%)`	:arrow_down:
src/content-steering-controller.js	`99.42% <100.00%> (+0.20%)`	:arrow_up:
src/dash-playlist-loader.js	`90.54% <100.00%> (+0.02%)`	:arrow_up:
src/playlist-controller.js	`95.80% <100.00%> (+0.44%)`	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

dzianis-dashkevich commented 1 year ago

Curious about the usage of throughput from segmentLoader:

Are we sure we want to use this value?

throughput is the time since all segment-related downloads are done till the time the segment is appended to the buffer (so basically, it is all about decrypt, transmux, and browser append time)

It is not related to network

dzianis-dashkevich commented 1 year ago

Also curious about the usage of segmentLoader in the contentSteeringController:

Do we need segmentLoader as a dependency? As far as I see, we are using throughput and xhr; both entities can be provided separately in the constructor.

I believe it should be vhs.bandwidth(pure network metric), or vhs.systemBandwidth(network + cpu) instead of segmentLoader.throughput

wseymour15 commented 1 year ago

Do we need segmentLoader as a dependency? As far as I see, we are using throughput and xhr; both entities can be provided separately in the constructor.

@dzianis-dashkevich I believe we need to pass the entire segment loader here because throughput will be changing. If we just pass throughput, it will be a static value. Would you agree with that @adrums86?

adrums86 commented 1 year ago

@wseymour15 Thanks for addressing this, yeah that is correct. We need a reference to the actual bandwidth property on the segmentLoader as it's updated during segment requests. Since the segmentLoader also has an xhr reference we can get both by just passing in the segmentLoader.

dzianis-dashkevich commented 1 year ago

@dzianis-dashkevich I believe we need to pass the entire segment loader here because throughput will be changing. If we just pass throughput, it will be a static value. Would you agree with that @adrums86?

@wseymour15, @adrums86

Yes, I understand that :)

Sorry, I didn't express my thoughts fully in my previous messages... I was referring to a famous OOP quote:

You wanted a banana, but you got a gorilla holding the banana

My point was to provide a bare minimum interface required for a controller to work.

This is a common approach when designing a new entity, you are using a DI for injecting dependencies, but instead of relying on concrete implementation, you invert control by expecting some interface: Check the Dependency Inversion Principle from SOLID: https://en.wikipedia.org/wiki/Dependency_inversion_principle

So, in this particular use case: Instead of providing the whole segmentLoader, we can provide: { get bandwidth() { return segmentLoader.bandwith } }

or something like that.

It is a minor thing, and I am ok to keep things as is.

adrums86 commented 1 year ago

@dzianis-dashkevich Yeah I see what you mean, and I 100% agree. The only reason it was done this way was because we needed xhr as well, sacrificing a bit of principle for less code. I'm cool with passing a getter for the segment loader bandwidth and passing just that and xhr separately.

dzianis-dashkevich commented 1 year ago

+1 from me when all checks will be successful

adrums86 commented 1 year ago

@dzianis-dashkevich we decided to remove the segmentLoader parameter and just pass xhr and a bandwidth function, I think it's much better. Thanks for the suggestion.

videojs / http-streaming