From a975c6a64417f6455e57262a656f77dae49c4a1b Mon Sep 17 00:00:00 2001 From: Riley Shaw Date: Tue, 10 Jan 2023 12:02:44 -0800 Subject: [PATCH 1/2] Improve resize listener with the `loadedmetadata` event in `DailyVideo` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current `onResize` callback may not fire on initial load. Since it relies on React’s render timing (using a combination of `useEffect` and `requestAnimationFrame`) instead of events on the video, the video stream may not be loaded in time for the initial `handleResize()` call. React has built-in support for `onResize` and `onLoadedMetadata` on `video` elements. This commit ditches the custom resize listener defined in `useEffect` in favor of these supported properties. --- src/components/DailyVideo.tsx | 58 ++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/components/DailyVideo.tsx b/src/components/DailyVideo.tsx index aaf112e..8f04357 100644 --- a/src/components/DailyVideo.tsx +++ b/src/components/DailyVideo.tsx @@ -1,4 +1,10 @@ -import React, { forwardRef, useEffect, useMemo, useRef } from 'react'; +import React, { + forwardRef, + useCallback, + useEffect, + useMemo, + useRef, +} from 'react'; import { useLocalSessionId } from '../hooks/useLocalSessionId'; import { useMediaTrack } from '../hooks/useMediaTrack'; @@ -161,35 +167,30 @@ export const DailyVideo = forwardRef( * Add optional event listener for resize event so the parent component * can know the video's native aspect ratio. */ - useEffect( - function reportVideoDimensions() { - const video = videoEl.current; - if (!onResize || !video) return; - - let frame: ReturnType; - const handleResize = () => { - if (!video) return; - if (frame) cancelAnimationFrame(frame); - frame = requestAnimationFrame(() => { - if (document.hidden) return; - const videoWidth = video?.videoWidth; - const videoHeight = video?.videoHeight; - if (videoWidth && videoHeight) { - onResize({ - aspectRatio: videoWidth / videoHeight, - height: videoHeight, - width: videoWidth, - }); - } + const handleVideoResolutionChange = useCallback( + (e: Event) => { + if (document.hidden) return; + const { videoWidth, videoHeight } = e.target as HTMLVideoElement; + if (videoWidth && videoHeight) { + onResize?.({ + aspectRatio: videoWidth / videoHeight, + width: videoWidth, + height: videoHeight, }); - }; - - handleResize(); - video?.addEventListener('resize', handleResize); - - return () => video?.removeEventListener('resize', handleResize); + } }, - [onResize, videoTrack] + [onResize] + ); + + const resizeHandlers = useMemo( + () => + onResize + ? { + onLoadedMetadata: handleVideoResolutionChange, + onResize: handleVideoResolutionChange, + } + : {}, + [onResize, handleVideoResolutionChange] ); return ( @@ -211,6 +212,7 @@ export const DailyVideo = forwardRef( ...style, ...(isPlayable ? playableStyle : {}), }} + {...resizeHandlers} {...props} /> ); From 12177ca339f0b09b5ec78e2a7d87a266508e6764 Mon Sep 17 00:00:00 2001 From: Riley Shaw Date: Wed, 11 Jan 2023 14:52:36 -0800 Subject: [PATCH 2/2] Use custom event handlers rather than React synthetic events This preserves support for React versions below v18. --- src/components/DailyVideo.tsx | 59 ++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/src/components/DailyVideo.tsx b/src/components/DailyVideo.tsx index 8f04357..c079150 100644 --- a/src/components/DailyVideo.tsx +++ b/src/components/DailyVideo.tsx @@ -1,10 +1,4 @@ -import React, { - forwardRef, - useCallback, - useEffect, - useMemo, - useRef, -} from 'react'; +import React, { forwardRef, useEffect, useMemo, useRef } from 'react'; import { useLocalSessionId } from '../hooks/useLocalSessionId'; import { useMediaTrack } from '../hooks/useMediaTrack'; @@ -167,32 +161,42 @@ export const DailyVideo = forwardRef( * Add optional event listener for resize event so the parent component * can know the video's native aspect ratio. */ - const handleVideoResolutionChange = useCallback( - (e: Event) => { - if (document.hidden) return; - const { videoWidth, videoHeight } = e.target as HTMLVideoElement; - if (videoWidth && videoHeight) { - onResize?.({ - aspectRatio: videoWidth / videoHeight, - width: videoWidth, - height: videoHeight, + useEffect( + function reportVideoDimensions() { + const video = videoEl.current; + if (!onResize || !video) return; + + let frame: ReturnType; + function handleResize() { + if (frame) cancelAnimationFrame(frame); + frame = requestAnimationFrame(() => { + const video = videoEl.current; + if (!video || document.hidden) return; + const videoWidth = video.videoWidth; + const videoHeight = video.videoHeight; + if (videoWidth && videoHeight) { + onResize({ + aspectRatio: videoWidth / videoHeight, + height: videoHeight, + width: videoWidth, + }); + } }); } + + handleResize(); + video.addEventListener('loadedmetadata', handleResize); + video.addEventListener('resize', handleResize); + + return () => { + if (frame) cancelAnimationFrame(frame); + video.removeEventListener('loadedmetadata', handleResize); + video.removeEventListener('resize', handleResize); + }; }, [onResize] ); - const resizeHandlers = useMemo( - () => - onResize - ? { - onLoadedMetadata: handleVideoResolutionChange, - onResize: handleVideoResolutionChange, - } - : {}, - [onResize, handleVideoResolutionChange] - ); - return (