From 1d1927d0e441f1023e7f9ebd0c821945487b1603 Mon Sep 17 00:00:00 2001 From: Michael Hyun Date: Mon, 9 Jan 2023 13:02:58 -0800 Subject: [PATCH] Revert changes to Background Blur and Background Replacement to fix bug --- CHANGELOG.md | 2 ++ .../BackgroundBlurProvider/index.tsx | 8 +++---- .../BackgroundReplacementProvider/index.tsx | 8 +++---- .../BackgroundBlurProvider/index.test.tsx | 22 +++++++++++++++++-- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba0903723..bcde8699b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,11 +17,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Updated `VoiceFocusProvider` to destroy the Voice Focus worker thread on unmount. +- Reverted changes to `BackgroundBlurProvider` and `BackgroundReplacemenProvider` to fix bug related to `isBackgroundBlurSupported` and `isBackgroundReplacementSupported` returning false. ### Fixed * Update the documentation to use `GlobalStyles` along with `ThemeProvider`. + ## [3.4.0] - 2022-09-13 ### Added diff --git a/src/providers/BackgroundBlurProvider/index.tsx b/src/providers/BackgroundBlurProvider/index.tsx index 692c72810..cc85447f9 100644 --- a/src/providers/BackgroundBlurProvider/index.tsx +++ b/src/providers/BackgroundBlurProvider/index.tsx @@ -88,9 +88,12 @@ const BackgroundBlurProvider: FC = ({ spec, options, children }) => { ); useEffect(() => { + // One reason we need to initialize first, even though we'll destroy this background blur processor when we create a new device + // is because we need to check if background blur is supported by initializing the background blur processor to see if the browser supports + initializeBackgroundBlur(); return () => { logger.info( - 'Specs or options were changed. Destroying background blur processor.' + 'Specs or options were changed. Destroying and re-initializing background blur processor.' ); backgroundBlurProcessor?.destroy(); }; @@ -147,9 +150,6 @@ const BackgroundBlurProvider: FC = ({ spec, options, children }) => { selectedDevice )}` ); - // TODO: We don't need to intialize a new processor every time we create a background blur device - // We could potentially check for if a processor exists already AND that the processor isn't destroyed. - // If both of those statements are true, then chooseNewInnerDevice instead of creating a new processor const currentProcessor = await initializeBackgroundBlur(); try { const logger = diff --git a/src/providers/BackgroundReplacementProvider/index.tsx b/src/providers/BackgroundReplacementProvider/index.tsx index 2434a2d4f..e62084ec8 100644 --- a/src/providers/BackgroundReplacementProvider/index.tsx +++ b/src/providers/BackgroundReplacementProvider/index.tsx @@ -95,9 +95,12 @@ const BackgroundReplacementProvider: FC = ({ ); useEffect(() => { + // One reason we need to initialize first, even though we'll destroy this background replacement processor when we create a new device + // is because we need to check if background replacement is supported by initializing the background replacement processor to see if the browser supports + initializeBackgroundReplacement(); return () => { logger.info( - 'Specs or options were changed. Destroying background replacement processor.' + 'Specs or options were changed. Destroying and re-initializing background replacement processor.' ); backgroundReplacementProcessor?.destroy(); }; @@ -155,9 +158,6 @@ const BackgroundReplacementProvider: FC = ({ selectedDevice )}` ); - // TODO: We don't need to intialize a new processor every time we create a background replacement device - // We could potentially check for if a processor exists already AND that the processor isn't destroyed. - // If both of those statements are true, then chooseNewInnerDevice instead of creating a new processor const currentProcessor = await initializeBackgroundReplacement(); try { const logger = diff --git a/tst/providers/BackgroundBlurProvider/index.test.tsx b/tst/providers/BackgroundBlurProvider/index.test.tsx index f580ded06..d9253b418 100644 --- a/tst/providers/BackgroundBlurProvider/index.test.tsx +++ b/tst/providers/BackgroundBlurProvider/index.test.tsx @@ -59,9 +59,27 @@ describe.only('BackgroundBlurProvider', () => { // happens when the component remounts. If it is happening more than // once, that means some dependency or parent is changing the parameters // erroneously. - expect(loggerInfoMock).toHaveBeenCalledTimes(1); + expect(loggerInfoMock).toHaveBeenCalledTimes(4); expect(loggerInfoMock).toHaveBeenCalledWith( - 'Specs or options were changed. Destroying background blur processor.' + `Initializing background blur processor with, spec: ${JSON.stringify( + undefined + )}, options: ${JSON.stringify(blurOptions)}` + ); + expect(loggerInfoMock).toHaveBeenCalledWith( + 'Specs or options were changed. Destroying and re-initializing background blur processor.' + ); + + // Even though we are using a NoOpVideoFrameProcessor, the input specs + // and options that are passed to the amazon-chime-sdk-js are still + // run through `resolveOptions` and `resolveSpec` in the + // `BackgroundBlurVideoFrameProcessor` constructor so any invalid API + // boundary changes to those are still validated by this test. The + // first warning call is output by `BackgroundBlurVideoFrameProcessor` + // and we don't validate it strictly because there's a non-deterministic + // timestamp. Verifying the call count should be good enough. + expect(loggerWarnMock).toHaveBeenCalledTimes(2); + expect(loggerWarnMock).toHaveBeenLastCalledWith( + expect.stringContaining('Initialized NoOpVideoFrameProcessor') ); // No errors should happen.