Skip to content
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
77e0d97
Added background option to More Menu, selection dialog, and unit tests
Jun 10, 2021
51eb265
added background icon svg, background and chat window no longer opena…
Jun 14, 2021
3cbf1ef
reinstall video-processors-sdk using rc release
Jun 14, 2021
c0855e7
worked on thumbnail
Jun 15, 2021
ab13b81
update VIDEO-5732
Jun 17, 2021
32335c6
Added background selection UI and unit tests
Jun 21, 2021
438cb04
code revision and image compression
Jun 23, 2021
91ba438
added smaller thumbnails, compressed images, used img elements, fixed…
Jun 24, 2021
066fd43
updated thumbnail images to correct dimensions
Jun 28, 2021
ca56a40
used themes in css and updated css
Jun 28, 2021
eb67e4a
added blur background
Jun 30, 2021
62e5e8c
more blur bg
Jun 30, 2021
1ab582f
resolved master merge conflicts
Jul 1, 2021
976e954
Implemented blur background and fixed failing tests
Jul 8, 2021
3d4a940
added useBackgroundSettings hook tests
Jul 10, 2021
94f218b
moved room variable inside useCallback
Jul 10, 2021
ad52f8c
refactored useBackgroundSettings implementation
Jul 14, 2021
8adde08
refactored useBackgroundSettings hook, still need to implement tests
Jul 16, 2021
30a9f59
Fixed implementation, need to fix tests
Jul 19, 2021
4faab69
implemented tests for useBackgroundSettings hook
Jul 19, 2021
ceae6d0
added check for local participant
Jul 20, 2021
5c5a8b8
remove processor from prejoin screen and completed test suite
Jul 20, 2021
f0a2997
cleaned up tests
Jul 20, 2021
eb0a31a
remove calls to updateSettings
Jul 20, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43,767 changes: 19,086 additions & 24,681 deletions package-lock.json

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import React from 'react';
import BackgroundThumbnail from './BackgroundThumbnail';
import BlurIcon from '@material-ui/icons/BlurOnOutlined';
import NoneIcon from '@material-ui/icons/NotInterestedOutlined';
import { shallow } from 'enzyme';
import useVideoContext from '../../../hooks/useVideoContext/useVideoContext';

Expand Down Expand Up @@ -50,24 +52,25 @@ describe('The BackgroundThumbanil component', () => {
index: 1,
},
setBackgroundSettings: mockSetBackgroundSettings,
updateBackgroundSettings: jest.fn(),
}));
const wrapper = shallow(<BackgroundThumbnail thumbnail={'image'} index={5} />);
expect(wrapper.find('.selected').exists()).toBe(false);
});

it("should contain the NoneIcon when thumbnail is set to 'none'", () => {
const wrapper = shallow(<BackgroundThumbnail thumbnail={'none'} />);
expect(wrapper.find('NotInterestedOutlinedIcon').exists()).toBe(true);
expect(wrapper.containsMatchingElement(<NoneIcon />)).toBe(true);
});

it("should contain the BlurIcon when thumbnail is set to 'blur'", () => {
const wrapper = shallow(<BackgroundThumbnail thumbnail={'blur'} />);
expect(wrapper.find('BlurOnOutlinedIcon').exists()).toBe(true);
expect(wrapper.containsMatchingElement(<BlurIcon />)).toBe(true);
});

it("should not have any icons when thumbnail is set to 'image'", () => {
const wrapper = shallow(<BackgroundThumbnail thumbnail={'image'} />);
expect(wrapper.find('BlurOnOutlinedIcon').exists()).toBe(false);
expect(wrapper.find('NotInterestedOutlinedIcon').exists()).toBe(false);
expect(wrapper.containsMatchingElement(<BlurIcon />)).toBe(false);
expect(wrapper.containsMatchingElement(<NoneIcon />)).toBe(false);
});
});
2 changes: 2 additions & 0 deletions src/components/Buttons/EndCallButton/EndCallButton.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const mockVideoContext = {
room: {
disconnect: jest.fn(),
},
removeProcessor: jest.fn(),
};

jest.mock('../../../hooks/useVideoContext/useVideoContext', () => () => mockVideoContext);
Expand All @@ -15,6 +16,7 @@ describe('End Call button', () => {
it('should disconnect from the room when clicked', () => {
const wrapper = shallow(<EndCallButton />);
wrapper.simulate('click');
expect(mockVideoContext.removeProcessor).toHaveBeenCalled();
expect(mockVideoContext.room.disconnect).toHaveBeenCalled();
});
});
9 changes: 7 additions & 2 deletions src/components/Buttons/EndCallButton/EndCallButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,15 @@ const useStyles = makeStyles((theme: Theme) =>

export default function EndCallButton(props: { className?: string }) {
const classes = useStyles();
const { room } = useVideoContext();
const { room, removeProcessor } = useVideoContext();

const handleDisconnect = (): void => {
removeProcessor();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized the background is also applied in the prejoin screen when I refresh. I think instead of calling the remove processor here, maybe we should call it in the prejoin screen.

room!.disconnect();
};

return (
<Button onClick={() => room!.disconnect()} className={clsx(classes.button, props.className)} data-cy-disconnect>
<Button onClick={handleDisconnect} className={clsx(classes.button, props.className)} data-cy-disconnect>
Disconnect
</Button>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,10 @@ exports[`the messageList component should render correctly 1`] = `
</div>
</div>
<div
class="makeStyles-messageContainer-45"
class="makeStyles-messageContainer-9"
>
<div
class="makeStyles-iconContainer-46"
class="makeStyles-iconContainer-10"
>
<svg
fill="none"
Expand Down Expand Up @@ -212,15 +212,15 @@ exports[`the messageList component should render correctly 1`] = `
</svg>
</div>
<div
class="makeStyles-mediaInfo-47"
class="makeStyles-mediaInfo-11"
>
<p
class="makeStyles-filename-48"
class="makeStyles-filename-12"
>
test1.txt
</p>
<p
class="makeStyles-size-49"
class="makeStyles-size-13"
>
120.56 KB
- Click to open
Expand All @@ -243,7 +243,6 @@ exports[`the messageList component should render correctly 1`] = `
aria-hidden="true"
class="MuiSvgIcon-root"
focusable="false"
role="presentation"
viewBox="0 0 24 24"
>
<path
Expand Down
4 changes: 4 additions & 0 deletions src/components/MenuBar/Menu/Menu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,26 @@ import useChatContext from '../../../hooks/useChatContext/useChatContext';
import useFlipCameraToggle from '../../../hooks/useFlipCameraToggle/useFlipCameraToggle';
import useMediaQuery from '@material-ui/core/useMediaQuery';
import useIsRecording from '../../../hooks/useIsRecording/useIsRecording';
import useLocalVideoToggle from '../../../hooks/useLocalVideoToggle/useLocalVideoToggle';

jest.mock('../../../hooks/useFlipCameraToggle/useFlipCameraToggle');
jest.mock('@material-ui/core/useMediaQuery');
jest.mock('../../../state');
jest.mock('../../../hooks/useVideoContext/useVideoContext', () => () => ({ room: { sid: 'mockRoomSid' } }));
jest.mock('../../../hooks/useIsRecording/useIsRecording');
jest.mock('../../../hooks/useChatContext/useChatContext');
jest.mock('../../../hooks/useLocalVideoToggle/useLocalVideoToggle');

const mockUseFlipCameraToggle = useFlipCameraToggle as jest.Mock<any>;
const mockUseMediaQuery = useMediaQuery as jest.Mock<boolean>;
const mockUseAppState = useAppState as jest.Mock<any>;
const mockUseIsRecording = useIsRecording as jest.Mock<boolean>;
const mockUseChatContext = useChatContext as jest.Mock<any>;
const mockUseLocalVideoToggle = useLocalVideoToggle as jest.Mock<any>;

const mockToggleChatWindow = jest.fn();
mockUseChatContext.mockImplementation(() => ({ setIsChatWindowOpen: mockToggleChatWindow }));
mockUseLocalVideoToggle.mockImplementation(() => [true, () => {}]);

describe('the Menu component', () => {
let mockUpdateRecordingRules: jest.Mock<any>;
Expand Down
6 changes: 3 additions & 3 deletions src/components/Snackbar/__snapshots__/Snackbar.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ exports[`the Snackbar component should render correctly with "error" variant 1`]
onClick={[Function]}
size="small"
>
<CloseIcon
<Memo(ForwardRef(CloseIcon))
fontSize="small"
/>
</WithStyles(ForwardRef(IconButton))>
Expand Down Expand Up @@ -103,7 +103,7 @@ exports[`the Snackbar component should render correctly with "info" variant 1`]
onClick={[Function]}
size="small"
>
<CloseIcon
<Memo(ForwardRef(CloseIcon))
fontSize="small"
/>
</WithStyles(ForwardRef(IconButton))>
Expand Down Expand Up @@ -159,7 +159,7 @@ exports[`the Snackbar component should render correctly with "warning" variant 1
onClick={[Function]}
size="small"
>
<CloseIcon
<Memo(ForwardRef(CloseIcon))
fontSize="small"
/>
</WithStyles(ForwardRef(IconButton))>
Expand Down
16 changes: 16 additions & 0 deletions src/components/VideoProvider/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ jest.mock('./useLocalTracks/useLocalTracks', () =>
jest.mock('./useHandleRoomDisconnection/useHandleRoomDisconnection');
jest.mock('./useHandleTrackPublicationFailed/useHandleTrackPublicationFailed');
jest.mock('./useRestartAudioTrackOnDeviceChange/useRestartAudioTrackOnDeviceChange');
jest.mock('@twilio/video-processors', () => {
return {
GaussianBlurBackgroundProcessor: jest.fn().mockImplementation(() => {
return {
loadModel: jest.fn(),
};
}),
};
});

describe('the VideoProvider component', () => {
it('should correctly return the Video Context object', () => {
Expand All @@ -34,6 +43,10 @@ describe('the VideoProvider component', () => {
</VideoProvider>
);
const { result } = renderHook(useVideoContext, { wrapper });
const expectedSettings = {
type: 'none',
index: 0,
};
expect(result.current).toMatchObject({
isConnecting: false,
localTracks: [{ name: 'mockTrack' }],
Expand All @@ -47,6 +60,9 @@ describe('the VideoProvider component', () => {
toggleScreenShare: expect.any(Function),
isBackgroundSelectionOpen: false,
setIsBackgroundSelectionOpen: expect.any(Function),
backgroundSettings: expectedSettings,
setBackgroundSettings: expect.any(Function),
removeProcessor: expect.any(Function),
});
expect(useRoom).toHaveBeenCalledWith([{ name: 'mockTrack' }], expect.any(Function), {
dominantSpeaker: true,
Expand Down
7 changes: 6 additions & 1 deletion src/components/VideoProvider/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export interface IVideoContext {
setIsBackgroundSelectionOpen: (value: boolean) => void;
backgroundSettings: BackgroundSettings;
setBackgroundSettings: (settings: BackgroundSettings) => void;
removeProcessor: () => void;
}

export const VideoContext = createContext<IVideoContext>(null!);
Expand Down Expand Up @@ -79,9 +80,12 @@ export function VideoProvider({ options, children, onError = () => {} }: VideoPr
);
useHandleTrackPublicationFailed(room, onError);
useRestartAudioTrackOnDeviceChange(localTracks);
console.log('tracks: ');
console.log(localTracks);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove


const [isBackgroundSelectionOpen, setIsBackgroundSelectionOpen] = useState(false);
const [backgroundSettings, setBackgroundSettings] = useBackgroundSettings();
const videoTrack = localTracks.find(track => track.name.includes('camera')) as LocalVideoTrack | undefined;
const [backgroundSettings, setBackgroundSettings, removeProcessor] = useBackgroundSettings(videoTrack);

return (
<VideoContext.Provider
Expand All @@ -102,6 +106,7 @@ export function VideoProvider({ options, children, onError = () => {} }: VideoPr
setIsBackgroundSelectionOpen,
backgroundSettings,
setBackgroundSettings,
removeProcessor,
}}
>
<SelectedParticipantProvider room={room}>{children}</SelectedParticipantProvider>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { act, renderHook } from '@testing-library/react-hooks';
import useBackgroundSettings, { BackgroundSettings } from './useBackgroundSettings';
const mockLoadModel = jest.fn();

jest.mock('@twilio/video-processors', () => {
return {
GaussianBlurBackgroundProcessor: jest.fn().mockImplementation(() => {
return {
loadModel: mockLoadModel,
};
}),
};
});

const defaultSettings = {
type: 'none',
index: 0,
};

const blurSettings = {
type: 'blur',
};

describe('The useBackgroundSettings hook ', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add test for the following use cases:

  • removeProcessor
    • should call videoTrack.removeProcessor if videoTrack and videoTrack.processor exists
    • should not call videoTrack.removeProcessor if videoTrack.processor doesn't exists
    • should not call videoTrack.removeProcessor if videoTrack doesn't exists
  • setBackgroundSettings
    • should call videoTrack.removeProcessor if videoTrack and videoTrack.processor exists
    • should not call videoTrack.removeProcessor if videoTrack.processor doesn't exists
    • should not call videoTrack.removeProcessor if videoTrack doesn't exists
    • should not call videoTrack.addProcessor with a param of blurProcessor if backgroundSettings.type is not equal to 'blur'
    • should not call videoTrack.addProcessor if backgroundSettings.type is 'blur' but videoTrack is missing
    • should set localStorage item for each of the use cases above

let mockVideoTrack: any;
beforeEach(() => {
mockVideoTrack = {
kind: 'video',
processor: '',
addProcessor: jest.fn(),
removeProcessor: jest.fn(),
};
});

it('should return the backgroundsettings and update function.', () => {
const { result } = renderHook(() => useBackgroundSettings(mockVideoTrack as any));
expect(result.current).toEqual([defaultSettings, expect.any(Function), expect.any(Function)]);
});

it('should set the background settings correctly and set the video processor when "blur" is selected', async () => {
const { result } = renderHook(() => useBackgroundSettings(mockVideoTrack as any));
await act(async () => {
result.current[1](blurSettings as BackgroundSettings);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 41 seems to be duplicated on all tests. I think this can be in the beforeEach hook.
Also, it's better to assign the results into variables for readability, instead of accessing them directly from the array. I think this can also live in a beforeEach hook. Something like below.

  let backgroundSettings: any;
  let setBackgroundSettings: any;
  let removeProcessor: any;
  let renderResult: any;

  beforeEach(() => {
    const { result } = renderHook(() => useBackgroundSettings(mockVideoTrack as any));
    renderResult = result;
    [backgroundSettings, setBackgroundSettings, removeProcessor] = renderResult.current;
  });

Then your tests would look something like

  it('should return the backgroundsettings and update function.', () => {
    expect(renderResult.current).toEqual([defaultSettings, expect.any(Function), expect.any(Function)]);
  });

  it('should set the background settings correctly and set the video processor when "blur" is selected', async () => {
    await act(async () => {
      setBackgroundSettings(blurSettings as BackgroundSettings);
    });
    expect(backgroundSettings.type).toEqual('blur');
    expect(mockVideoTrack.addProcessor).toHaveBeenCalled();
  });

});
expect(result.current[0].type).toEqual('blur');
expect(mockVideoTrack.addProcessor).toHaveBeenCalled();
});

it('should set the background settings correctly and remove the video processor when "none" is selected', async () => {
const { result } = renderHook(() => useBackgroundSettings(mockVideoTrack as any));
// set video processor to non-null value
await act(async () => {
result.current[1](blurSettings as BackgroundSettings);
});
// set video processor to none
await act(async () => {
result.current[1](defaultSettings as BackgroundSettings);
});
expect(mockVideoTrack.addProcessor).toHaveBeenCalled();
expect(result.current[0].type).toEqual('none');
});

it('should set the background settings correctly and set the video processor when "image" is selected', () => {
// TODO add test after implementing virtual background feature/logic
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { useState } from 'react';
import { LocalVideoTrack } from 'twilio-video';
import { useState, useEffect } from 'react';
import { SELECTED_BACKGROUND_SETTINGS_KEY } from '../../../constants';
import { GaussianBlurBackgroundProcessor } from '@twilio/video-processors';
import AbstractThumb from '../../../images/thumb/Abstract.jpg';
import BohoHomeThumb from '../../../images/thumb/BohoHome.jpg';
import BookshelfThumb from '../../../images/thumb/Bookshelf.jpg';
Expand Down Expand Up @@ -65,13 +68,44 @@ export const backgroundConfig = {
images,
};

// TODO : Add video processing logic after backgroundSettings change
// useEffect hooks, etc ...
const virtualBackgroundAssets = '/virtualbackground';
let blurProcessor: GaussianBlurBackgroundProcessor;

export default function useBackgroundSettings() {
const [backgroundSettings, setBackgroundSettings] = useState({
type: 'none',
index: 0,
} as BackgroundSettings);
return [backgroundSettings, setBackgroundSettings] as const;
export default function useBackgroundSettings(videoTrack: LocalVideoTrack | undefined) {
const [backgroundSettings, setBackgroundSettings] = useState<BackgroundSettings>(() => {
const localStorageSettings = window.localStorage.getItem(SELECTED_BACKGROUND_SETTINGS_KEY);
return localStorageSettings ? JSON.parse(localStorageSettings) : { type: 'none', index: 0 };
});

// Specifically used for handling room disconnection
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this comment. You can reuse this function below.

Suggested change
// Specifically used for handling room disconnection

const removeProcessor = (): void => {
if (videoTrack && videoTrack.processor) {
videoTrack.removeProcessor(videoTrack.processor);
}
};

useEffect(() => {
if (!blurProcessor) {
blurProcessor = new GaussianBlurBackgroundProcessor({
assetsPath: virtualBackgroundAssets,
});
blurProcessor.loadModel();
Comment on lines +82 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this needs to be in a useEffect hook. Would it work if we moved it outside of useBackgroundSettings?

}
}, []);

useEffect(() => {
if (videoTrack) {
if (videoTrack.processor) {
videoTrack.removeProcessor(videoTrack.processor);
}
Comment on lines +93 to +95
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reuse the method you created above

Suggested change
if (videoTrack.processor) {
videoTrack.removeProcessor(videoTrack.processor);
}
removeProcessor();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Charlie, I made it a separate function and didn't use it inside the useEffect hook since I was getting warnings about missing removeProcessor as a dependency, but when I include it, I get another warning about how removeProcessor is updated on every render. What would be the best step/approach to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a way to make this work, but I wonder if we need to worry about removing the processor when we disconnect from a room? When we disconnect from a room, track.stop() is automatically called because of this hook here (this is to turn off the camera light on laptops when the user disconnects from a room). Then when the user goes back to the Join Room screens, new tracks are acquired.

@charliesantos Since we are stopping the tracks on disconnect, do we need to remove the processor as well?

If we do, I think we can just handle that in the removeVideoTrack function in the useLocalTracks hook here. Something like this maybe (but only if needed):

  const removeLocalVideoTrack = useCallback(() => {
    if (videoTrack) {
      if (videoTrack.processor) {
        videoTrack.removeProcessor(videoTrack.processor)
      }
      videoTrack.stop();
      setVideoTrack(undefined);
    }
  }, [videoTrack]);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we need to remove the processor that way it won't show in the join screen. On @timmydoza 's suggestion, it doesn't seem to save us anything 😄 . I was hoping we don't have to duplicate the code. I think I'm leaning more towards keeping our current implementation. We will probably remove this later if we decide to launch the virtual background settings UI in the join screen page.

if (backgroundSettings.type === 'blur') {
videoTrack.addProcessor(blurProcessor);
} else if (backgroundSettings.type === 'image') {
// TODO implement image background replacement logic
}
}
window.localStorage.setItem(SELECTED_BACKGROUND_SETTINGS_KEY, JSON.stringify(backgroundSettings));
}, [backgroundSettings, videoTrack]);

return [backgroundSettings, setBackgroundSettings, removeProcessor] as const;
}
3 changes: 3 additions & 0 deletions src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,6 @@ export const DEFAULT_VIDEO_CONSTRAINTS: MediaStreamConstraints['video'] = {
export const SELECTED_AUDIO_INPUT_KEY = 'TwilioVideoApp-selectedAudioInput';
export const SELECTED_AUDIO_OUTPUT_KEY = 'TwilioVideoApp-selectedAudioOutput';
export const SELECTED_VIDEO_INPUT_KEY = 'TwilioVideoApp-selectedVideoInput';

// This is used to store the current background settings in localStorage
export const SELECTED_BACKGROUND_SETTINGS_KEY = 'TwilioVideoApp-selectedBackgroundSettings';
Loading