-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Media Recording: Add Background Blur #12067
Conversation
Plugin builds for 5878aaa are ready 🛎️!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works really well, but it's hard to review with the mixed code from multiple PR's. Let's have @felipebochehin87 quickly QA #11988, so we can merge #12022 and rebase this PR.
#12022 has been merged, so this can be updated to main to better reflect that actual changes. |
# Conflicts: # package-lock.json
Summary of the latest updates: I also fixed #12033 |
packages/story-editor/src/app/highlights/quickActions/useQuickActions.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Will continue the review on Monday, couldn't go through everything)
packages/story-editor/src/components/mediaRecording/settingsModal.js
Outdated
Show resolved
Hide resolved
packages/story-editor/src/components/mediaRecording/settingsModal.js
Outdated
Show resolved
Hide resolved
@@ -331,6 +340,7 @@ const useQuickActions = () => { | |||
muteAudio: actions.muteAudio, | |||
unMuteAudio: actions.unMuteAudio, | |||
startTrim: actions.startTrim, | |||
setVideoEffect: actions.setVideoEffect, | |||
})); | |||
|
|||
const enableMediaRecordingTrimming = useFeature('recordingTrimming'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 thought:
Not related to this PR directly but should these actions be disabled during recording? Currently it seems that it's possible to click on it during recording which results in the recording just stopping. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, okay, I'm now trying again and seeing these are disabled generally. Unfortunately I don't know how to reproduce it but somehow I ended up (twice) in a situation where I could click on the Disable Background Blur quick-action while recording -> recording stopped -> when I opened the recorder again, I saw this with neither of the buttons (Pause/Stop) doing anything:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I tried again and the following seems to break it eventually:
- Open the recorder
- Enable background blur
- Start recording
- Wait for a couple of seconds -- when testing, the quick-actions became enabled when the blur option was turned on.
- Now disable the blur while recording. And now the video stops recording.
- Click X to close the recorder tool.
When I repeat the process a few times, the following occurs:
- If the video stopped recording because of using quick-actions during recording, and then when opening the recorder tool from the Library again, it starts recording right away when opening.
- After a few times of doing this, the video starts recording but doesn't really show the video and the Pause/Stop buttons don't do anything (as above on the screenshot).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be just a flag change (incorrect status/inputStatus/isReady), but it turned out that I was trying to fix something that was impossible to fix with the current version of the library. It was not possible to use the custom media recorder more than once per instance. Luckily, when I was about to go crazy I checked the library repo and saw this one-liner 🪄
Note to self: always check if you are using the latest version of the library.
Noticed another weird issues with the settings dialog. See video. Screen.Recording.2022-09-13.at.11.14.03.mov |
What browser is this? any errors in the console? |
Chrome 104. Nope. |
Seeing slow down of image in safari. And other issues in firefox. |
That is expected. It will never match the native blur or get close to it, tho it looks better on my Macbook Air.
|
Not seeing device list issue anymore. |
Not seeing firefox issues now. 🎉 |
Yes, it's WASM and some of its memory issues. I added a try/catch there that's the best we can do. |
More testing, I am seeing the slow version in safari. Blur background works but it is very slow in safari. Screen.Recording.2022-09-14.at.17.28.05.mov |
Summary
This version of Background Blur is based on the latest version of the new Trimming flow PR.
The Media Pipe object is very difficult to cooperate and it often throws errors while switching between no effect and blur effect. In this PR I managed to make it work consistently. The recording part is not yet merged into this version (two media recorders one used to get the camera feed, the second for canvas recording when the blur effect is active).
Edges are now smooth so it looks a lot better than the first version:
Relevant Technical Choices
To-do
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZ
label to the PRFixes #11909
Fixes #12033
Fixes #12245
Fixes #12249
Fixes #12246