Skip to content

Commit

Permalink
Revert changes to Background Blur and Background Replacement to fix bug
Browse files Browse the repository at this point in the history
  • Loading branch information
michhyun1 committed Jan 9, 2023
1 parent b8242bd commit 1d1927d
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 10 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions src/providers/BackgroundBlurProvider/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,12 @@ const BackgroundBlurProvider: FC<Props> = ({ 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();
};
Expand Down Expand Up @@ -147,9 +150,6 @@ const BackgroundBlurProvider: FC<Props> = ({ 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 =
Expand Down
8 changes: 4 additions & 4 deletions src/providers/BackgroundReplacementProvider/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,12 @@ const BackgroundReplacementProvider: FC<Props> = ({
);

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();
};
Expand Down Expand Up @@ -155,9 +158,6 @@ const BackgroundReplacementProvider: FC<Props> = ({
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 =
Expand Down
22 changes: 20 additions & 2 deletions tst/providers/BackgroundBlurProvider/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 1d1927d

Please sign in to comment.