Refactored Encoded Transform management into it's own component for better support of non-redundant audio transforms#3131
Conversation
src/encodedtransformmanager/MediaMetricsEncodedTransformManager.ts
Outdated
Show resolved
Hide resolved
src/encodedtransformmanager/DefaultEncodedTransformWorkerManager.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Trigger reconnect to recreate peer connection without transforms | ||
| const status = new MeetingSessionStatus(MeetingSessionStatusCode.TaskFailed); |
There was a problem hiding this comment.
Thought: Should we add EncodedTransformManagerFailed or StartEncodedTransformWorkerTaskFailed?
|
|
||
| // Create transform managers based on disabled configuration | ||
| if (!disabledTransforms?.redundantAudio) { | ||
| this.redManager = new RedundantAudioEncodedTransformManager(this.worker!, this.logger); |
There was a problem hiding this comment.
nit: Can return Worker instance from createWorker that way we dont have to do this.worker! here.
src/encodedtransformmanager/MediaMetricsEncodedTransformManager.ts
Outdated
Show resolved
Hide resolved
…etter support of non-redundant audio transforms
hensmi-amazon
force-pushed
the
transform-metrics
branch
from
c3290a4 to
f13a94c
Compare
| @@ -124,6 +124,8 @@ export default class MeetingSessionStatus { | |||
| return 'The audio connection failed.'; | |||
| case MeetingSessionStatusCode.SignalingChannelClosedUnexpectedly: | |||
| return 'The signaling channel was closed unexpectedly. This may be due to a network change or backend detected failure.'; | |||
| case MeetingSessionStatusCode.EncodedTransformManagerFailed: | |||
There was a problem hiding this comment.
So this is not a failure code that need to reconnect?
There was a problem hiding this comment.
technically none of these need to be reconnected by the builder, since they are reconnected internally. This just clarifies that it will adjust to make the reconnection successful, which may impact behavior. Kept it pretty vague ofc.
| EncodedTransformWorker.redundantAudioEncodedTransform = new RedundantAudioEncodedTransform(); | ||
| EncodedTransformWorker.audioSenderMetricsTransform = new AudioSenderMetricsTransform(); | ||
| EncodedTransformWorker.audioReceiverMetricsTransform = new AudioReceiverMetricsTransform(); | ||
| EncodedTransformWorker.videoSenderMetricsTransform = new VideoSenderMetricsTransform(); |
There was a problem hiding this comment.
Maybe I misunderstand but its a bit confusing that a class named RedundantAudio... also has video metrics
There was a problem hiding this comment.
This class isn't called RendundantAudio :). It's meant to be the management layer on top of all transforms (since the worker is shared).
| // Handle legacy createEncodedStreams() API | ||
| if (message.msgType === 'StartEncodedTransformWorker') { | ||
| EncodedTransformWorker.log('Setting up legacy encoded streams'); | ||
| const disabledTransforms: DisabledEncodedTransformsConfiguration = |
There was a problem hiding this comment.
Is audio redundant the default option? Otherwise, feel like we could name this as EncodedTransformsConfiguration isntead.
There was a problem hiding this comment.
yup it's default on. I think all transforms would be default on in the future.
There was a problem hiding this comment.
^classic last words ofc.
5 participants
Issue #: N/A
Description of changes:
Note that given the size of this PR i am first skipping unit tests to save myself some time in case i need to do significant additional refactoring.
We would like to add transforms for metric/monitoring, and at some point E2EE. This change does that while adding a simple transform PPS metric. First packet metrics will occur in a future PR.
Testing:
Checklist:
Have you successfully run
npm run build:releaselocally?n
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
not really that public :)
n
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.