Save the redundant audio worker code during build time so that the worker code stays intact and is able to be loaded #2772

dinmin-amzn · 2023-10-05T01:38:25Z

Issue #: #2771
Description of changes:
The redundant audio worker code string may vary based on the build system of different projects. Because of this, certain build systems will cause invalid worker code to be generated and the redundant audio worker will not be able to run. Now, the redundant audio worker code is saved at build time so that all clients can run the redundant audio worker with the same exact code, regardless of the client's build system.

This issue was originally found in #2771.

Testing:

Can these tested using a demo application? Please provide reproducible step-by-step instructions.
Reproduction steps for the original issue are mentioned in #2771. Following the same steps with this change should fix the issue.

Checklist:

Have you successfully run npm run build:release locally?
y
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
n
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
n

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

src/redundantaudioencoder/RedundantAudioEncoder.ts

pracheth · 2023-10-05T16:55:33Z

src/redundantaudioencoder/RedundantAudioEncoder.ts

+ * This function is only meant to be called within the RED worker to initialize and start RED functionality.
+ */
+function startRedWorker(): void {
+  RedundantAudioEncoder.shouldLogDebug = true;


This should be false, yes?

We want the RED worker to be able to emit logs, so this should be true

Does the worker have any high frequency logs? If yes, then this will lead to a lot of messages being sent from the worker to the main thread. Probably just a quick scrub of the encoder file should be good.

The encoder file only has low frequency logs at the moment, so this shouldn't overload the main thread with too many messages

Can we just based on the Log level defined ?

Updated to configure this based on log level

ltrung · 2023-10-06T19:20:34Z

config/tsconfig.red.json

+    "outDir": "../build",
+    "rootDir": "../src"
+  },
+  "include": [


Why do we need a separate config for just RED? Can you put a comment in the package.json explaining it?

Same question. Using the existing tsconfig file I see RedundantAudioEncoder.js being generated in the build folder? Doesn't that mean we don't need this extra tsconfig file?

I assume this is because we need to convert ts -> js for this file first and put it in the source folder so when we transpile for the whole folder they can refer to this file

The idea for generating and building the worker code was:

Build the RedundantAudioEncoder.js file

Use the RedundantAudioEncoder.js file to generate RedundantAudioEncoderWorkerCode.ts

Now build everything using npm run tsc:src so that the generated RedundantAudioEncoderWorkerCode.ts file also gets built and transpiled

So yes, we don't need this extra tsconfig file, but using the normal tsconfig.json would generate all of the built files for Step 1, then the RedundantAudioEncoderWorkerCode.ts file would be generated in Step 2, and then all of the files would be re-built again in Step 3. Having the separate config for RED was just a minor build time optimization so that we wouldn't be building all of the code twice during each npm run build.

pracheth

Wondering if we should make these lines INFO rather than DEBUG while we're at it?

[DEBUG] ChimeLogger - [AudioRed] Initializing RedundantAudioEncoder
[DEBUG] ChimeLogger - [AudioRed] Setting up sender RED transform
[DEBUG] ChimeLogger - [AudioRed] Setting up receiver RED transform
[DEBUG] ChimeLogger - [AudioRed] red payload type set to 63
[DEBUG] ChimeLogger - [AudioRed] opus payload type set to 111

ltrung · 2023-10-06T19:29:00Z

src/redundantaudioencoder/RedundantAudioEncoderWorkerCode.ts

+ * This file was generated with the `generate-red-worker-code.js` script.
+ */
+
+export default class RedundantAudioEncoderWorkerCode {


nit: Can we rename the folder to match with the file name (..workercode)?

I will make this change

script/generate-red-worker-code.js

dinmin-amzn · 2023-10-06T19:39:41Z

Wondering if we should make these lines INFO rather than DEBUG while we're at it?

[DEBUG] ChimeLogger - [AudioRed] Initializing RedundantAudioEncoder
[DEBUG] ChimeLogger - [AudioRed] Setting up sender RED transform
[DEBUG] ChimeLogger - [AudioRed] Setting up receiver RED transform
[DEBUG] ChimeLogger - [AudioRed] red payload type set to 63
[DEBUG] ChimeLogger - [AudioRed] opus payload type set to 111

Yes, we should make these INFO level

dinmin-amzn · 2023-10-07T00:15:18Z

script/generate-red-worker-code.js

+  ]
+}
+`;
+fs.writeFileSync(`${configDir}/${redTsconfig}`, redTsconfigContent);


sure, let me add it

ltrung · 2023-10-07T00:16:29Z

script/generate-red-worker-code.js

+fs.writeFileSync(`${configDir}/${redTsconfig}`, redTsconfigContent);
+
+// Transpile `RedundantAudioEncoder.ts`.
+const command = 'npx tsc --build config/tsconfig.red.json';


nit: Can we not depend on npx and just use the local node_module path to tsc? otherwise you would need to update READM to install it.

Updated to use local tsc from node_modules in latest push

…rker code stays intact and is able to be loaded The redundant audio worker code string may vary based on the build system of different projects. Because of this, certain build systems will cause invalid worker code to be generated and the redundant audio worker will not be able to run. Now, the redundant audio worker code is saved at build time so that all clients can run the redundant audio worker with the same exact code, regardless of the client's build system. This issue was originally found in #2771.

…rker code stays intact and is able to be loaded (#2772) The redundant audio worker code string may vary based on the build system of different projects. Because of this, certain build systems will cause invalid worker code to be generated and the redundant audio worker will not be able to run. Now, the redundant audio worker code is saved at build time so that all clients can run the redundant audio worker with the same exact code, regardless of the client's build system. This issue was originally found in #2771.

dinmin-amzn requested a review from a team as a code owner October 5, 2023 01:38

dinmin-amzn force-pushed the avoid-red-regex branch 2 times, most recently from 3a863b8 to 3b4e765 Compare October 5, 2023 01:48

hensmi-amazon previously approved these changes Oct 5, 2023

View reviewed changes

ltrung reviewed Oct 5, 2023

View reviewed changes

src/redundantaudioencoder/RedundantAudioEncoder.ts Outdated Show resolved Hide resolved

ltrung mentioned this pull request Oct 5, 2023

Error when start audioVideo after 3.18.1 update #2771

Closed

4 tasks

pracheth reviewed Oct 5, 2023

View reviewed changes

dinmin-amzn dismissed hensmi-amazon’s stale review via 0218702 October 6, 2023 00:09

dinmin-amzn force-pushed the avoid-red-regex branch from 3b4e765 to 0218702 Compare October 6, 2023 00:09

dinmin-amzn changed the title ~~Avoid use of regex to create RED worker code~~ Save the redundant audio worker code during build time so that the worker code stays intact and is able to be loaded Oct 6, 2023

dinmin-amzn force-pushed the avoid-red-regex branch 2 times, most recently from 3cb7500 to fbd8292 Compare October 6, 2023 07:45

ltrung reviewed Oct 6, 2023

View reviewed changes

pracheth reviewed Oct 6, 2023

View reviewed changes

ltrung reviewed Oct 6, 2023

View reviewed changes

script/generate-red-worker-code.js Show resolved Hide resolved

dinmin-amzn force-pushed the avoid-red-regex branch from fbd8292 to 57beef2 Compare October 6, 2023 22:26

pracheth previously approved these changes Oct 6, 2023

View reviewed changes

dinmin-amzn dismissed pracheth’s stale review via c54a1fd October 6, 2023 23:24

dinmin-amzn force-pushed the avoid-red-regex branch from 57beef2 to c54a1fd Compare October 6, 2023 23:24

pracheth previously approved these changes Oct 6, 2023

View reviewed changes

dinmin-amzn commented Oct 7, 2023

View reviewed changes

ltrung reviewed Oct 7, 2023

View reviewed changes

dinmin-amzn dismissed pracheth’s stale review via b442d9b October 7, 2023 00:19

dinmin-amzn force-pushed the avoid-red-regex branch from c54a1fd to b442d9b Compare October 7, 2023 00:19

dinmin-amzn force-pushed the avoid-red-regex branch from b442d9b to 596b50f Compare October 7, 2023 00:35

ltrung approved these changes Oct 7, 2023

View reviewed changes

pracheth approved these changes Oct 7, 2023

View reviewed changes

dinmin-amzn merged commit 096edac into main Oct 9, 2023
10 checks passed

dinmin-amzn deleted the avoid-red-regex branch October 9, 2023 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save the redundant audio worker code during build time so that the worker code stays intact and is able to be loaded #2772

Save the redundant audio worker code during build time so that the worker code stays intact and is able to be loaded #2772

dinmin-amzn commented Oct 5, 2023 •

edited

Loading

pracheth Oct 5, 2023

dinmin-amzn Oct 5, 2023

pracheth Oct 5, 2023

dinmin-amzn Oct 5, 2023

ltrung Oct 5, 2023

dinmin-amzn Oct 6, 2023

ltrung Oct 6, 2023 •

edited

Loading

pracheth Oct 6, 2023

ltrung Oct 6, 2023

dinmin-amzn Oct 6, 2023 •

edited

Loading

pracheth left a comment

ltrung Oct 6, 2023

dinmin-amzn Oct 6, 2023

dinmin-amzn commented Oct 6, 2023

dinmin-amzn Oct 7, 2023

ltrung Oct 7, 2023

dinmin-amzn Oct 7, 2023

Save the redundant audio worker code during build time so that the worker code stays intact and is able to be loaded #2772

Save the redundant audio worker code during build time so that the worker code stays intact and is able to be loaded #2772

Conversation

dinmin-amzn commented Oct 5, 2023 • edited Loading

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

ltrung Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinmin-amzn Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

pracheth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinmin-amzn commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinmin-amzn commented Oct 5, 2023 •

edited

Loading

ltrung Oct 6, 2023 •

edited

Loading

dinmin-amzn Oct 6, 2023 •

edited

Loading