-
Notifications
You must be signed in to change notification settings - Fork 717
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
FaceControls
#1461
FaceControls
#1461
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e3f4365:
|
5fa25c9
to
2d39bdd
Compare
2d39bdd
to
eef118c
Compare
src/core/FaceLandmarker.tsx
Outdated
FilesetResolver.forVisionTasks(basePath) | ||
.then((vision) => FaceLandmarkerImpl.createFromOptions(vision, options)) | ||
.then((faceLandmarker) => setFaceLandmarker(faceLandmarker)) | ||
.catch((err) => console.error('error while creating faceLandmarker', err)) |
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'd consider using suspense here.
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.
but how to deal with ret.close()
cleanup with suspend
?
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, following @drcmda additional advices on that, I've now converted it to suspend, but I let you check my impl is correct
src/core/FaceLandmarker.tsx
Outdated
return () => void ret?.close() | ||
}, [basePath, options]) |
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.
If options
is an object, this will break reference every rerender. Either destructure or use an expression that is immune like JSON.stringify
-- not ideal but it works.
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've set opts = JSON.stringify(options)
and set my dep with
src/core/FaceControls.tsx
Outdated
const [stream, setStream] = useState<MediaStream>() | ||
|
||
const videoTextureApiRef = useRef<VideoTextureApi>(null) | ||
|
||
const faceControls = useFaceControls() | ||
useEffect(() => { | ||
let strm: MediaStream | ||
|
||
if (!videoTextureSrc) { | ||
navigator.mediaDevices | ||
.getUserMedia({ | ||
audio: false, | ||
video: { facingMode: 'user' }, | ||
}) | ||
.then((s) => { | ||
strm = s | ||
faceControls.dispatchEvent({ type: 'stream', stream: strm }) | ||
setStream(strm) | ||
}) | ||
.catch(console.error) | ||
} | ||
|
||
return () => strm?.getTracks().forEach((track) => track.stop()) | ||
}, [faceControls, videoTextureSrc]) |
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.
Suspense would also be helpful here. I'm pedantic about this now as it's breaking later.
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 now done ;) I let you check my impl
thanks for the review @CodyJasonBennett, will integrate them tomorrow ;) |
<color attach="background" args={['#303030']} /> | ||
<axesHelper /> | ||
|
||
<React.Suspense fallback={null}> |
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 guess React.Suspense
is now required here, since <FaceLandmarker>
provider is now suspended?
I'm not sure
@@ -284,7 +285,11 @@ export const FaceControls = forwardRef<FaceControlsApi, FaceControlsProps>( | |||
const faceBlendshapes = faces?.faceBlendshapes?.[0] | |||
return ( | |||
<FaceControlsContext.Provider value={api}> | |||
{webcam && <Webcam ref={webcamApiRef} autostart={autostart} videoTextureSrc={webcamVideoTextureSrc} />} | |||
{webcam && ( | |||
<Suspense fallback={null}> |
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.
same here: is Suspense really required? I'm not sure
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.
Your call whether suspense should go here or in user-land around the component.
@CodyJasonBennett I think I've now addressed your precious feedbacks. If you have no other ones, I think I would be ready to merge |
🎉 This PR is included in version 9.74.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
FYI this commit breaks react native builds. Use <= v9.73.4 for react native. |
@markhughes, can you create an issue? I think we'll have to remove it from the native target since iOS doesn't support JIT/WASM. |
Note
Merged, see
<FaceControls>
docWhy / What
DEMO: https://abernier.github.io/fld/ as well as the story
facecontrols-videotexture_720p.mov
Control the camera with your head/eyes:
or a custom camera, with your eyes:
This PR basically consists in:
@mediapipe/tasks-vision
Checklist