feat: Allow Opt in to Codec Switching #4766

tjohnson-nbcuni · 2022-11-28T15:15:48Z

Reference implementation for codec switching.

Resolves: #1528
Related: #4379

Extended StreamingConfig with new enumerated property codecSwitchingStrategy.
Allows for 3 strategies as referenced in original conversation with @theRealRobG .
-- DISABLED is current Shaka behavior.
--SMOOTH behavior allows for use of SourceBuffer.changeType on environments which support it.
--RELOAD behavior replaces MediaSource instances.

google-cla · 2022-11-28T15:15:53Z

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.

avelad · 2022-11-28T15:24:17Z

@tjohnson-nbcuni Thanks for your PR! Can your sign the CLA? Can you rebase and resolve the conflicts?

tjohnson-nbcuni · 2022-11-28T15:58:05Z

@avelad - Yep, jumping through the CLA hoops now. Updated and fixed merged conflicts previously.

tjohnson-nbcuni · 2022-12-01T13:51:32Z

@avelad - CLA signed. Rebased and merge conflicts resolved.

avelad · 2022-12-01T15:10:46Z

externs/shaka/player.js

@@ -1304,7 +1308,6 @@ shaka.extern.LcevcConfiguration;
 */
 shaka.extern.OfflineConfiguration;

-


Remove this change

Looks like spacing due to merge w LCEVC changes. Has been resolved.

Please, remove this change

externs/shaka/player.js

lib/media/media_source_engine.js

lib/media/streaming_engine.js

lib/offline/storage.js

lib/media/media_source_engine.js

lib/media/streaming_engine.js

externs/shaka/player.js

lib/player.js

lib/util/stream_utils.js

lib/media/streaming_engine.js

github-actions · 2022-12-02T06:41:27Z

Incremental code coverage: 52.34%

theodab · 2022-12-02T07:25:01Z

lib/media/streaming_engine.js

-  switchVariant(
-      variant, clearBuffer = false, safeMargin = 0, force = false,
-      adaptation = false) {
+  async switchVariant(variant, clearBuffer = false,


Also, for reference, I spent a while considering this particular change.
The fact that this method was made async, but none of the callsites for it were made to await the result, made me kind of nervous.
But all of the asynchronous operations inside this method are just methods on the MediaSourceEngine, which uses a queue to protect against race conditions and other concurrency errors. So it should probably be safe.
Also, actually awaiting this every time would require changing the public interface of the player, which would make this a good bit more complicated to put into a feature release.

lib/media/media_source_engine.js

…rning on text usage per request.

avelad · 2022-12-07T10:32:37Z

externs/shaka/player.js

@@ -1304,7 +1308,6 @@ shaka.extern.LcevcConfiguration;
 */
 shaka.extern.OfflineConfiguration;

-


Please, remove this change

externs/sourcebuffer.js

lib/media/media_source_engine.js

lib/media/streaming_engine.js

avelad · 2022-12-07T10:46:08Z

lib/player.js

+      shaka.config.CodecSwitchingStrategy.DISABLED) {
+      this.filterManifestByCurrentVariant_();
+    } else {
+      shaka.log.debug('Skipping re-filter. Not necessary' +


Here I think it is necessary to do a more complex filtering...

Imagine the next stream:

Video:
360p - h264 - 500kbps
720p - h264 - 1000kbps
720p - h265 - 800kbps
1080p - h265 - 1500kbps

Audio:
en - aac - 2 channels - 128kbps
es - ac3 - 2 channels - 128kbps
es - ac3 - 6 channels - 320kbps
es - aac - 6 channels - 256kbps

What would be expected to return (I would like @joeyparrish to confirm)

Video:
360p - h264 - 500kbps
720p - h265 - 800kbps
1080p - h265 - 1500kbps

Audio:
en - aac - 2 channels - 128kbps
es - ac3 - 2 channels - 128kbps
es - aac - 6 channels - 256kbps

Going to discuss this with @sky-hugolima in the morning. Looking to close out remaining change requests; before re-addressing the filtration logic.

Hey @avelad, sorry for the delay replying on this comment, we had to spend some time refactoring the actual switching logic to make it work correctly for the reload strategy.

Looking at your comment above, it makes sense, and while we actually don't experience those scenarios on our particular Peacock streams, we do understand that's a valid concern.

I think where these changes affect the intended behaviour tho, is not specifically on not calling filterManifestByCurrentVariant_ because that method's single purpose is to filter-out variants that have codecs incompatible with the currently selected variant, and that logic doesn't apply if we can switch codecs, but instead the place you mention bellow chooseCodecsAndFilterManifest is where I think we need to make some other changes.

It's going to be a somewhat more complex refactoring to the chooseCodecsAndFilterManifest logic to output the intended result, and filter-out the variants that conflict at the same audio/video specs, but not variants where the non optimal codecs are the only ones available for that particular audio/video spec, but I guess that's what we need to do.

Please advise if the above assumption is incorrect.

That makes sense to me. I haven't finished reviewing the entire PR, but filtering by current variant should be skipped AFAICT. Removal of redundant streams is a separate issue, I think.

avelad · 2022-12-07T10:47:15Z

lib/util/player_configuration.js

@@ -180,6 +181,7 @@ shaka.util.PlayerConfiguration = class {
      observeQualityChanges: false,
      maxDisabledTime: 30,
      parsePrftBox: false,
+      codecSwitchingStrategy: shaka.config.CodecSwitchingStrategy.DISABLED,


I think the default behavior should be SMOOTH since it is a feature that we want to have enabled by default.

avelad · 2022-12-07T10:47:42Z

lib/util/stream_utils.js

+    // If enforcing a codec switch strategy skip filtration
+    if (codecSwitchingStrategy && codecSwitchingStrategy !==
+      shaka.config.CodecSwitchingStrategy.DISABLED) {
+      shaka.log.debug('Skipping chooseCodecsAndFilterManifest');


See my previous comment about the filtering

I agree. I don't think the filtering here is correct, and I think more work is required.

Codec-switching means we don't have to choose one specific codec, but some care should still be taken to consider codec preferences and to remove redundant streams available in multiple codecs.

This also breaks the default codec selection when preferredVideoCodecs and preferredAudioCodecs are provided.

lib/util/stream_utils.js

avelad · 2022-12-07T10:55:16Z

The PR should also mention that it resolves #1528

sky-hugolima · 2022-12-09T09:47:10Z

Hey guys !

I've been thinking about this PR and also testing it, and I think the decision we took to initiate/control the codec switching from inside the streaming engine, doesn't feel right, 1st because needing special logic to switch codecs should be an implementation detail of the media source, and I think only the media source engine, and the main player flow (because of filtering) should be aware of it, and 2nd because of the complexity and async nature of the streaming engine, it's really hard to refactor it so it doesn't break when there's an ongoing media source reset operation originating from a call to variantSwitch, and 3rd, there's a lot of spooky calls we're doing in the streaming engine on the reload strategy (like calling initStreams_ again).

I think we should revisit this, to see if we can isolate the code switching logic inside the media source engine, specifically at the call to append by looking at the reference's codec. The media source already has protective measures for blocking operations and we can make the append only return after the media source & buffers are re-created. Doing the switch at the append stage also makes more sense because it will already be part of the expected sequence of events, and not require weird logic inside the streaming engine.

joeyparrish · 2022-12-09T21:43:11Z

I have not reviewed this PR yet, but @sky-hugolima's comment sounds reasonable to me.

…gEngine into mediaSourceEngine

…setStreamProperties

joeyparrish · 2023-01-23T20:27:56Z

lib/media/streaming_engine.js


          throw error;
        }
      };
-      operations.push(setProperties());
+      await setProperties();


Why did you change this? With this change, we will strictly wait for setStreamProperties() to complete before beginning to fetch the init segment. I don't see any reason those can't happen in parallel.

joeyparrish · 2023-01-23T20:31:38Z

lib/player.js

+      shaka.config.CodecSwitchingStrategy.DISABLED) {
+      this.filterManifestByCurrentVariant_();
+    } else {
+      shaka.log.debug('Skipping re-filter. Not necessary' +


That makes sense to me. I haven't finished reviewing the entire PR, but filtering by current variant should be skipped AFAICT. Removal of redundant streams is a separate issue, I think.

joeyparrish · 2023-01-23T20:32:38Z

lib/config/codec_switching_strategy.js

+ * @export
+ */
+shaka.config.CodecSwitchingStrategy = {
+  // No codec switching allowed (i.e. current Shaka behaviour).


nit: "current" should say "default". Also, you should move these comments into jsdoc syntax so that they end up in the public API docs for these values.

joeyparrish · 2023-01-23T20:32:58Z

lib/config/codec_switching_strategy.js

+  // and attempt to use this first, but fall back to reloading `MediaSource` if
+  // not available / fails.
+  //
+  // This case can be implemented as a future improvement once #1528 is


Is this accurate? It seems like you're implementing it.

joeyparrish · 2023-01-23T20:34:03Z

externs/sourcebuffer.js

 /** @type {string} */
 SourceBuffer.prototype.mode;
+
+// TODO: need to figure out how to correctly override this


Why does this need to be overridden? It appears to be in the compiler's built-ins already: https://github.com/google/closure-compiler/blob/a4329316423d8478452e6330c6b887dd97ddef00/externs/browser/mediasource.js#L155

joeyparrish · 2023-01-23T20:45:06Z

lib/media/media_source_engine.js

+      const sourceBufferAdded = new shaka.util.PublicPromise();
+      const sourceBuffers =
+        /** @type {EventTarget} */(this.mediaSource_.sourceBuffers);
+
+      this.eventManager_.listenOnce(sourceBuffers, 'addsourcebuffer', () => {
+        sourceBufferAdded.resolve();
+      });


This only waits for a single source buffer to be added, but you add up to two. Perhaps you should track the number of events fired instead, and resolve the promise when the count matches what will be added in the loop below.

joeyparrish · 2023-01-23T20:46:16Z

lib/media/media_source_engine.js

+        sourceBufferAdded.resolve();
+      });
+
+      for (const contentType of [ContentType.AUDIO, ContentType.VIDEO]) {


I don't think this loop is correct. Some content may be audio-only or video-only. You should not assume all content is audio+video.

joeyparrish · 2023-01-23T20:47:31Z

lib/media/media_source_engine.js

+        }
+
+        const type = mimeType + this.config_.sourceBufferExtraFeatures;
+        const sourceBuffer = this.mediaSource_.addSourceBuffer(type);


This duplicates a lot of what init() does, but with less flexibility. I think you should look into factoring out what you need from init() instead, and using something like this.sourceBufferTypes_ to drive the loop.

joeyparrish · 2023-01-23T20:49:19Z

lib/media/media_source_engine.js

+    shaka.log.debug(
+        `${this.sourceBufferTypes_[contentType]} -> ${newFullMimeType}`);
+    if (shaka.media.Capabilities.isChangeTypeSupported()) {
+      this.sourceBuffers_[contentType].changeType(newFullMimeType);


It is not guaranteed that any call to changeType will work merely because the old and new types are both supported separately. In particular, hardware decoders and EME can have an impact on this capability. You need to use MediaCapabilities to determine which changeType() calls will work, and ideally incorporate that into the filtering logic.

joeyparrish · 2023-01-23T20:52:01Z

lib/util/stream_utils.js

+    // If enforcing a codec switch strategy skip filtration
+    if (codecSwitchingStrategy && codecSwitchingStrategy !==
+      shaka.config.CodecSwitchingStrategy.DISABLED) {
+      shaka.log.debug('Skipping chooseCodecsAndFilterManifest');


I agree. I don't think the filtering here is correct, and I think more work is required.

Codec-switching means we don't have to choose one specific codec, but some care should still be taken to consider codec preferences and to remove redundant streams available in multiple codecs.

joeyparrish · 2023-01-23T20:55:30Z

Codec-switching should also impact logic in period-flattening. Currently, multi-period DASH content uses codec compatibility rules to decide what to connect across periods. If codec-switching is allowed, that means many more connections are possible (and consequently that we either need much more removal of redundant streams or much earlier removal of redundancies during the period-processing stage).

Addressing that may turn out to be much uglier or more performance-impacting than anything else I've left feedback on, but I think we need to understand how we will address that before this PR can be merged.

Thanks so much for your work on this feature, though. I think this is moving in the right direction.

sky-hugolima · 2023-01-25T15:29:49Z

Codec-switching should also impact logic in period-flattening. Currently, multi-period DASH content uses codec compatibility rules to decide what to connect across periods. If codec-switching is allowed, that means many more connections are possible (and consequently that we either need much more removal of redundant streams or much earlier removal of redundancies during the period-processing stage).

Addressing that may turn out to be much uglier or more performance-impacting than anything else I've left feedback on, but I think we need to understand how we will address that before this PR can be merged.

Thanks so much for your work on this feature, though. I think this is moving in the right direction.

Hey @joeyparrish !

I think I understand what you mean... so, if we had for example:

Period 1:
- Audio adaptation 1: en - main - 2 channels - aac

Period 2:
-  Audio adaptation 1: en - main - 2 channels - ec3

should the player consider period 2 audio adaption 1, an extension of audio adaption 1 from the 1st period ?

If it should, then the period combining logic would need significant changes...

The thing is we don't really expect this scenario to be possible on our streams, but instead we expect all streams to have a matching configuration including the same codec across all periods. This codec switching feature instead for us is mostly to provide a solution for streams that have some audio languages that are encoded in a single codec, different from the main audio but still supported by the decoder.

Could you still consider accepting the logic on this PR without support for that more lenient combination of streams ?
I suggest we change the default codec switching strategy to NO_SWITCHING as default, allowing explicit opt-in if desired, until we decide how to cater for that problematic scenario.

Thank you for the review.

Thanks to #4766 This PR adds support for codec hot-switching in MediaSourceEngine. There are two ways to change codec: - Using `Sourcebuffer.changeType` on supported browsers - Reloading the `MediaSource` instance with the new codecs This change does not change the current filter logic, so this code is not used yet. The change in the filtering will be done in a subsequent PR in order to simplify the changes of this PR.

sky-hugolima · 2023-06-13T11:20:55Z

Hey !

Sorry for letting this go stale for so long... we've been testing this feature across our supported devices and are now getting ready to release it to prod using the RELOAD strategy enabled by default, the reasoning was to support the lowest denominator 1st, and then after we assess KPIs, proceed with testing the SMOOTH strategy across more devices to build a support whitelist.

There were a couple of device/models where we saw unexpected behaviour with the RELOAD tho, for example on Xbox, we found that sometimes when resetting the media source, the video would freeze while audio playback continued playing, and on Tizen 3 we observed that the player would sometimes enter an intermitted seeking behaviour, so we ended up disabling the feature on those until we can investigate it further.

Now, about this pull request, since we've moved on by splitting it into two and already merged the codec switching logic inside the media source and streaming engines (#5217), I think we should close this one.

Meanwhile, not sure what're your plans for the filtering logic changes, but we've opted for after the manifest is already filtered of variants unsupported by the device (through media capabilities), not do any further filtering of variants when the codec switching logic is enabled, even on scenarios where both codecs are available for the same lang & role. The reasoning being we do the filtering on the application side based on our rules that give preference to a higher channel count and only then to codecs based on a set order of codec preference, but we're not sure this would be a desirable behaviour for every app.

The main issue with our approach, is that the initial track selection logic done by the preference based adaptation set criteria, must match the filtering logic on the app, otherwise that track can potentially be outside the options we provide the user with.

A potentially better alternative would be to still run filtering of multiple supported audio tracks that share the same lang & role inside the player, but somehow let the app customize the decisioning logic.

For example, by default the player could opt between duplicate tracks, based of the following order:

channel count pref
codec pref
bandwitdh

but if the app provides a custom audio track selection method through config, for example:
isPreferredAudio(track: shaka.extern.Track, candidate: shaka.extern.Track):boolean then the player would use this function instead to filter the manifest of duplicate audio tracks.

Thoughts ?

avelad · 2023-07-05T07:38:21Z

I'm going to close this PR because the internal codec switching logic is already done, only the variant filtering part is missing to leave variants with different codecs.

If anyone wants to work on this, there are several files that need to be modified:

For the ABR you have to modify https://github.com/shaka-project/shaka-player/blob/main/lib/media/adaptation_set.js , but keep in mind that the ABR should only do SMOOTH transitions (isChangeTypeSupported () == true)
It would be necessary to modify the logic to maintain variants, this logic is in https://github.com/shaka-project/shaka-player/blob/main/lib/util/stream_utils.js (chooseCodecsAndFilterManifest method)
We should review the logic of the filterManifestWithStreamUtils_ method (at https://github.com/shaka-project/shaka-player/blob/main/lib/player.js) to avoid filtering variants that do not have the same codec.

Point 1 and 3 are easy to implement. Point 2 is where all the logic is.

tjohnson-nbcuni added 2 commits November 26, 2022 20:55

config update

dddc751

working smooth

a2338c4

tjohnson-nbcuni changed the title ~~Allow Opt in to Codec Switching~~ Feat: Allow Opt in to Codec Switching Nov 28, 2022

tjohnson-nbcuni changed the title ~~Feat: Allow Opt in to Codec Switching~~ feat: Allow Opt in to Codec Switching Nov 28, 2022

avelad requested review from joeyparrish, theodab, avelad, JulianDomingo and littlespex November 28, 2022 15:22

avelad added type: enhancement New feature or request priority: P2 Smaller impact or easy workaround labels Nov 28, 2022

avelad added this to the v4.4 milestone Nov 28, 2022

working reload

3de4943

tjohnson-nbcuni force-pushed the codec-switching branch from 9faefb3 to 3de4943 Compare November 28, 2022 15:41

Merge branch 'main' into codec-switching

3ab5032

tjohnson-nbcuni added 2 commits November 30, 2022 12:35

test fixes

9d437ef

merge fix

757dd97

fix jsdoc comment from merge

cf72812

avelad requested changes Dec 1, 2022

View reviewed changes

update with fixes for build & spacing issues

0ee96ec

theodab requested changes Dec 2, 2022

View reviewed changes

theodab reviewed Dec 2, 2022

View reviewed changes

theodab requested changes Dec 2, 2022

View reviewed changes

lib/media/media_source_engine.js Outdated Show resolved Hide resolved

tjohnson-nbcuni added 2 commits December 4, 2022 22:55

demo fixes and added support for video codec switching

6ce4b3d

updates for changeType extern

379ac17

tjohnson-nbcuni added 7 commits December 6, 2022 08:28

build fixes, typo spacing

d1dfdb4

build fix, null type supported for prevVariant. updated change for wa…

345cbc7

…rning on text usage per request.

whitespace revert per request

71792a4

udpate jsdoc, remove bind and use arrow func for reset

846739b

update log messaging per request.

55c7a68

revert externs to fix build. update to use blocking enqueue.

8f9bca7

update demo locales

7745bcb

avelad requested changes Dec 7, 2022

View reviewed changes

update misspellings per request

a5a2035

tjohnson-nbcuni and others added 6 commits December 9, 2022 18:52

update codecs comparison

ed38556

merge with updated main, fix merge issues

a1ef8f9

rebase against main, fix merge issues, initial refactor from Streamin…

2deaafe

…gEngine into mediaSourceEngine

wip - move config property into media config. move switch logic into …

89abad2

…setStreamProperties

some changes to make reloading work reliably

dc2bc69

merge w resiliency branch. merge fixes.

ccd6942

FireMasterK mentioned this pull request Dec 20, 2022

Only 360p available on Piped, 1080p+ on YouTube TeamPiped/Piped#1901

Closed

1 task

joeyparrish requested changes Jan 23, 2023

View reviewed changes

avelad mentioned this pull request May 8, 2023

feat: Add support for changing codecs in MediaSourceEngine #5217

Merged

avelad closed this Jul 5, 2023

dave-nicholas mentioned this pull request Aug 8, 2023

feat: Enable codec switching #5470

Merged

shaka-bot added the status: archived Archived and locked; will not be updated label Sep 3, 2023

shaka-project locked as resolved and limited conversation to collaborators Sep 3, 2023

		@@ -1304,7 +1308,6 @@ shaka.extern.LcevcConfiguration;
		*/
		shaka.extern.OfflineConfiguration;

feat: Allow Opt in to Codec Switching #4766

feat: Allow Opt in to Codec Switching #4766

Conversation

tjohnson-nbcuni commented Nov 28, 2022 • edited

google-cla bot commented Nov 28, 2022

avelad commented Nov 28, 2022

tjohnson-nbcuni commented Nov 28, 2022

tjohnson-nbcuni commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sky-hugolima Dec 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avelad commented Dec 7, 2022

sky-hugolima commented Dec 9, 2022 • edited

joeyparrish commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparrish commented Jan 23, 2023

sky-hugolima commented Jan 25, 2023

sky-hugolima commented Jun 13, 2023 • edited

avelad commented Jul 5, 2023

tjohnson-nbcuni commented Nov 28, 2022 •

edited

github-actions bot commented Dec 2, 2022 •

edited

sky-hugolima Dec 16, 2022 •

edited

sky-hugolima commented Dec 9, 2022 •

edited

sky-hugolima commented Jun 13, 2023 •

edited