feat(virtual background): Add virtual background flow. #8750

tudor-phd · 2021-03-08T15:35:34Z

No description provided.

saghul

Can't wait for this to land! Great work so far! Left some comments, PTAL.

saghul · 2021-03-09T13:21:28Z

lang/main.json

@@ -809,8 +812,8 @@
        "tileViewToggle": "Toggle tile view",
        "toggleCamera": "Toggle camera",
        "videomute": "Start / Stop camera",
-        "startvideoblur": "Blur my background",
-        "stopvideoblur": "Disable background blur"
+        "startvideoblur": "Enable background",


Let's change the keys to match please.

saghul · 2021-03-09T13:22:05Z

lang/main.json

-        "startvideoblur": "Blur my background",
-        "stopvideoblur": "Disable background blur"
+        "startvideoblur": "Enable background",
+        "stopvideoblur": "Change background"


Don't we just need a single entry now? The user will be able to choose between a background, blur or nothing, right?

Case 1: The user has already set a background => Text will be: "Change background"
Case 2: The user has no background set, or has pressed the None button => Text will be: "Enable background"

I don't think we need 2. "Enable background" sounds very weird. I'd go with a si gel entry with something like "Select background"

saghul · 2021-03-09T13:23:21Z

react/features/blur/actions.js


-        return Promise.resolve();
+        return getBlurEffect()
+            .then(blurEffectInstance =>


Please make this an async function and remove the .then "ladder" here :-)

saghul · 2021-03-09T13:24:04Z

react/features/blur/actions.js

+        return getBlurEffect()
+            .then(blurEffectInstance =>
+                jitsiTrack
+                    .setEffect(enabled ? blurEffectInstance : undefined)


We are creating an effect object just to discard it when disabling, let's avoid that.

saghul · 2021-03-09T13:25:20Z

react/features/blur/components/VideoBlurButton.js

@@ -36,7 +36,7 @@ type Props = AbstractButtonProps & {
 */
 class VideoBlurButton extends AbstractButton<Props, *> {


Please rename the button and the feature itself to match the new behavior. This does now a lot more than blur!

saghul · 2021-03-09T13:34:59Z

react/features/virtual-background/components/VirtualBackgroundDialog.js

+
+    const addImageBackground = image => {
+        dispatch(setVirtualBackground(image, true));
+        dispatch(toggleBlurEffect(true));


We need a new name for this, since it now turns on the virtual background, not just blur.

saghul · 2021-03-09T13:36:15Z

react/features/virtual-background/components/VirtualBackgroundDialog.js

+        tooltip: 'Image 1',
+        name: 'background-1.jpg',
+        id: 1,
+        src: '../../../../images/virtual-background/background-1.jpg'


you should be able to use images/foo because the static assets will be part of the CDN anyway.

saghul · 2021-03-09T13:37:40Z

react/features/virtual-background/components/VirtualBackgroundDialog.js

+                    position = { 'top' }>
+                    <div
+                        className = 'virtual-background-none'
+                        // eslint-disable-next-line react/jsx-no-bind


Please put this at the top of the file and remove it for the entire file.

We may need to rethink if having it enabled is a good idea going forward, but that's outside the scope of this PR.

react/features/virtual-background/components/VirtualBackgroundDialog.js

saghul · 2021-03-09T13:40:48Z

react/features/virtual-background/components/VirtualBackgroundDialog.js

+                            src = { image.src } />
+                    </Tooltip>
+                ))}
+                <div className = 'virtual-video-preview'>


Is this a real preview? Looks like the effect is being applied to the actual track anyway not to a different one.

To be discussed...

Removed the preview.

saghul

Left some comments, PTAL! Nothing major, mostly related to consistency / naming.

saghul · 2021-03-10T15:30:37Z

react/features/blur/actions.js

-*/
-export function toggleBlurEffect(enabled: boolean) {
-    return function(dispatch: (Object) => Object, getState: () => any) {
+ * Signals the local participant is switching between blurred or non blurred video.


Please reword this so it better matches what it does now.

saghul · 2021-03-10T15:30:44Z

react/features/blur/actions.js

+        try {
+            if (enabled) {
+                await jitsiTrack.setEffect(await getBlurEffect(virtualBackground));
+                dispatch(blurEnabled());


saghul · 2021-03-10T15:30:53Z

react/features/blur/actions.js

+
+        try {
+            if (enabled) {
+                await jitsiTrack.setEffect(await getBlurEffect(virtualBackground));


saghul · 2021-03-10T15:31:07Z

react/features/blur/components/VideoBackgroundButton.js

@@ -28,31 +28,28 @@ type Props = AbstractButtonProps & {
     * The redux {@code dispatch} function.
     */
    dispatch: Function
-
 };

 /**
 * An abstract implementation of a button that toggles the video blur effect.


saghul · 2021-03-10T15:31:54Z

react/features/blur/functions.js

@@ -7,16 +7,17 @@ let filterSupport;
 /**
 * Returns promise that resolves with the blur effect instance.
 *
+ * @param {Object} virtualBackground - The virtual object.


Please describe the object.

saghul · 2021-03-10T15:36:58Z

react/features/virtual-background/components/VirtualBackgroundDialog.js

+                    </div>
+                </Tooltip>
+                <Tooltip
+                    content = { 'Enable Blur' }


Ditto, translate.

saghul · 2021-03-10T15:38:41Z

react/features/stream-effects/blur/JitsiStreamBlurEffect.js

@@ -38,6 +39,12 @@ export default class JitsiStreamBlurEffect {
     * @param {Object} options - Segmentation dimensions.
     */
    constructor(model: Object, options: Object) {
+        this._options = options;


Please rename this class too, it now does more than blur.

saghul · 2021-03-10T15:40:09Z

react/features/stream-effects/blur/index.js

@@ -57,5 +58,7 @@ export async function createBlurEffect() {

    const options = wasmCheck.feature.simd ? segmentationDimensions.model144 : segmentationDimensions.model96;

+    options.virtualBackground = virtualBackground;
+
    return new JitsiStreamBlurEffect(tflite, options);


Please rename this class.

saghul · 2021-03-10T15:42:07Z

react/features/virtual-background/components/VirtualBackgroundDialog.js

+    /**
+     * The JitsiLocalTrack to display.
+     */
+    videoTrack: ?Object


saghul · 2021-03-10T15:42:41Z

react/features/virtual-background/reducer.js

+/**
+ * Initial state.
+ */
+const DEFAULT_STATE = {


Both of these are falsey when undefined, you don't need to define them explicitly, do you?

If I don't define them -> I get this console error: "The initial state may not be undefined."

Doesn't this do it?

ReducerRegistry.register('features/virtual-background', (state = {}, action) => {

saghul

Did another round, PTAL!

saghul · 2021-03-11T13:07:25Z

react/features/base/tracks/loadEffects.web.js

@@ -14,8 +14,8 @@ import logger from './logger';
 export default function loadEffects(store: Object): Promise<any> {
    const state = store.getState();

-    const blurPromise = state['features/blur'].blurEnabled
-        ? getBlurEffect()
+    const blurPromise = state['features/virtual-backgrounds'].backgroundEnabled


backgroundPromose? also adjust the log line please

saghul · 2021-03-11T13:09:41Z

react/features/stream-effects/blur/index.js

+    model96: {
+        height: 96,
+        width: 160,
+        virtualBackground: {}


Please create a new object then instead of augmenting this one. Like so:

const options = { ...(wasmCheck.feature.simd ? segmentationDimensions.model144 : segmentationDimensions.model96), virtualBackground }; return new JitsiStreamBackgroundEffect(tflite, options);

react/features/toolbox/components/web/Toolbox.js

react/features/virtual-background/components/VirtualBackgroundDialog.js

saghul · 2021-03-11T13:11:55Z

react/features/virtual-background/reducer.js

+/**
+ * Initial state.
+ */
+const DEFAULT_STATE = {


Doesn't this do it?

ReducerRegistry.register('features/virtual-background', (state = {}, action) => {

saghul · 2021-03-11T13:14:32Z

react/features/virtual-backgrounds/components/VideoBackgroundButton.js

@@ -92,10 +89,9 @@ function _mapStateToProps(state): Object {
    const tracks = state['features/base/tracks'];

    return {
-        _isVideoBlurred: Boolean(state['features/blur'].blurEnabled),
+        _isVideoBlurred: Boolean(state['features/virtual-backgrounds'].backgroundEnabled),


_isBackgroundEnabled

saghul · 2021-03-11T13:14:56Z

react/features/virtual-backgrounds/functions.js

    const ns = getJitsiMeetGlobalNS();

    if (ns.effects && ns.effects.createBlurEffect) {
-        return ns.effects.createBlurEffect();


createVirtualBackgroundEffect

saghul · 2021-03-11T13:15:14Z

react/features/virtual-backgrounds/reducer.js

+
+
+ReducerRegistry.register('features/virtual-backgrounds', (state = {}, action) => {
+


drop the extra newline

saghul · 2021-03-11T13:15:26Z

react/features/virtual-backgrounds/reducer.js

+import { BACKGROUND_ENABLED, BACKGROUND_DISABLED } from './actionTypes';
+
+
+ReducerRegistry.register('features/virtual-backgrounds', (state = {}, action) => {


some docs please

saghul · 2021-03-11T13:17:34Z

react/features/virtual-backgrounds/reducer.js

+ReducerRegistry.register('features/virtual-backgrounds', (state = {}, action) => {
+
+    switch (action.type) {
+    case BACKGROUND_ENABLED: {


Why is this not part of the virtual-background (no S) feature? Please put all of this in that one feature, why would we want 2, with such similar names even?

saghul

Almost there! PTAL.

saghul · 2021-03-12T09:11:54Z

react/features/base/icons/svg/virtual-background.svg

@@ -0,0 +1,3 @@
+<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
+<path fill-rule="evenodd" clip-rule="evenodd" d="M16.6666 1.66667H3.33329C2.41282 1.66667 1.66663 2.41286 1.66663 3.33334V16.6667C1.66663 17.5871 2.41282 18.3333 3.33329 18.3333H3.63257H8.56767H9.14753H15.6942H16.6666C17.5871 18.3333 18.3333 17.5871 18.3333 16.6667V3.33334C18.3333 2.41286 17.5871 1.66667 16.6666 1.66667ZM7.57977 12.3005L9.3687 14.893L12.0223 8.76728C12.2052 8.34496 12.6959 8.15091 13.1182 8.33385C13.2964 8.41106 13.4421 8.54811 13.53 8.72131L16.6666 14.9002V3.33334H3.33329V16.4665L6.208 12.3005C6.4694 11.9217 6.98838 11.8265 7.36718 12.0879C7.45035 12.1453 7.52238 12.2174 7.57977 12.3005ZM7.49996 10C6.11925 10 4.99996 8.88072 4.99996 7.5C4.99996 6.11929 6.11925 5.00001 7.49996 5.00001C8.88067 5.00001 9.99996 6.11929 9.99996 7.5C9.99996 8.88072 8.88067 10 7.49996 10ZM8.33329 7.5C8.33329 7.96024 7.9602 8.33334 7.49996 8.33334C7.03972 8.33334 6.66663 7.96024 6.66663 7.5C6.66663 7.03977 7.03972 6.66667 7.49996 6.66667C7.9602 6.66667 8.33329 7.03977 8.33329 7.5ZM12.8466 11.0572L15.6942 16.6667H10.4167L12.8466 11.0572ZM6.89389 14.2411L8.56767 16.6667H5.2201L6.89389 14.2411Z" fill="white"/>


Please remove the fill="white"

This is not solved.

saghul · 2021-03-12T09:13:43Z

react/features/toolbox/components/web/Toolbox.js

-            this._shouldShowButton('videobackgroundblur')
-                && <VideoBlurButton
-                    key = 'videobackgroundblur'
+            this._shouldShowButton('select-background')


Add some backwards compatibility check here. It should still work with the old button name.

saghul · 2021-03-12T09:15:30Z

react/features/stream-effects/blur/index.js

@@ -2,35 +2,37 @@



Please rename this folder from blur to virtual-background too.

saghul · 2021-03-12T09:17:38Z

react/features/virtual-background/index.js

@@ -1,3 +1,5 @@
 export * from './actions';
 export * from './components';
 export * from './functions';
+
+import './reducer';


Import the reducer in the app's reducers.web.js, not here.

saghul · 2021-03-12T09:18:46Z

react/features/virtual-background/reducer.js

+    case BACKGROUND_ENABLED: {
+        return {
+            ...state,
+            backgroundEnabled


when the action is created this is called backgroundEffectEnabled, so this is broken.

saghul · 2021-03-12T09:19:17Z

react/features/virtual-background/actionTypes.js

+ * is enabled.
+ *
+ * {
+ *      type: BACKGROUND_ENABLED


Document the parameter.

saghul requested changes Mar 9, 2021

View reviewed changes

tudor-phd force-pushed the virtual-background-feature branch from 09370ad to d5a2348 Compare March 9, 2021 16:57

saghul requested changes Mar 10, 2021

View reviewed changes

tudor-phd force-pushed the virtual-background-feature branch from ad190a3 to e37a31c Compare March 11, 2021 10:25

saghul requested changes Mar 11, 2021

View reviewed changes

saghul requested changes Mar 12, 2021

View reviewed changes

saghul force-pushed the virtual-background-feature branch from cae762b to 334a240 Compare March 12, 2021 13:17

feat(virtual-backgrounds) add virtual background support

21f9122

saghul force-pushed the virtual-background-feature branch from 334a240 to 21f9122 Compare March 12, 2021 13:18

fix(virtual-background) use tighter edge smoothing

612e70b

saghul approved these changes Mar 12, 2021

View reviewed changes

saghul merged commit 31ace26 into jitsi:master Mar 12, 2021

QuentinPi mentioned this pull request May 17, 2021

Virtual Backgrounds webcamoid/webcamoid#635

Open

		@@ -36,7 +36,7 @@ type Props = AbstractButtonProps & {
		*/
		class VideoBlurButton extends AbstractButton<Props, *> {



		ReducerRegistry.register('features/virtual-backgrounds', (state = {}, action) => {

		import { BACKGROUND_ENABLED, BACKGROUND_DISABLED } from './actionTypes';


		ReducerRegistry.register('features/virtual-backgrounds', (state = {}, action) => {

		@@ -0,0 +1,3 @@
		<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
		<path fill-rule="evenodd" clip-rule="evenodd" d="M16.6666 1.66667H3.33329C2.41282 1.66667 1.66663 2.41286 1.66663 3.33334V16.6667C1.66663 17.5871 2.41282 18.3333 3.33329 18.3333H3.63257H8.56767H9.14753H15.6942H16.6666C17.5871 18.3333 18.3333 17.5871 18.3333 16.6667V3.33334C18.3333 2.41286 17.5871 1.66667 16.6666 1.66667ZM7.57977 12.3005L9.3687 14.893L12.0223 8.76728C12.2052 8.34496 12.6959 8.15091 13.1182 8.33385C13.2964 8.41106 13.4421 8.54811 13.53 8.72131L16.6666 14.9002V3.33334H3.33329V16.4665L6.208 12.3005C6.4694 11.9217 6.98838 11.8265 7.36718 12.0879C7.45035 12.1453 7.52238 12.2174 7.57977 12.3005ZM7.49996 10C6.11925 10 4.99996 8.88072 4.99996 7.5C4.99996 6.11929 6.11925 5.00001 7.49996 5.00001C8.88067 5.00001 9.99996 6.11929 9.99996 7.5C9.99996 8.88072 8.88067 10 7.49996 10ZM8.33329 7.5C8.33329 7.96024 7.9602 8.33334 7.49996 8.33334C7.03972 8.33334 6.66663 7.96024 6.66663 7.5C6.66663 7.03977 7.03972 6.66667 7.49996 6.66667C7.9602 6.66667 8.33329 7.03977 8.33329 7.5ZM12.8466 11.0572L15.6942 16.6667H10.4167L12.8466 11.0572ZM6.89389 14.2411L8.56767 16.6667H5.2201L6.89389 14.2411Z" fill="white"/>

feat(virtual background): Add virtual background flow. #8750

feat(virtual background): Add virtual background flow. #8750

Conversation

tudor-phd commented Mar 8, 2021

saghul left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul left a 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

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

saghul left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment