Camera Control Component with websocket h264 stream provider#20
Camera Control Component with websocket h264 stream provider#20
Conversation
NoxHarmonium
left a comment
There was a problem hiding this comment.
Looks pretty good, just some refactoring suggestions.
How can I test it? Can I spin up the h264-websocket-stream and it will access it via localhost?
| startX: number; | ||
| startY: number; | ||
| width: number; | ||
| height: number; |
There was a problem hiding this comment.
Since this type definition is duplicated on line onZoom it might be good to extract to a type alias.
There was a problem hiding this comment.
Are you happy with what I have done in d997fa4
| if (typeof source === "object" && "video" in source && source.video) { | ||
| return { w: source.video.videoWidth, h: source.video.videoHeight }; | ||
| if (!image) return { w: 0, h: 0 }; | ||
| if (typeof image === "object" && "video" in image && image.video) { |
There was a problem hiding this comment.
If image can only be { video: HTMLVideoElement | null; frameId: number } | ImageBitmap | null if we rearrange the guards like this can we avoid this check?
// Rules out `null`
if (!image) return { w: 0, h: 0 };
// Rules out `ImageBitmap`
if (image instanceof ImageBitmap) {
return { w: image.width, h: image.height };
}
// This must be `{ video: HTMLVideoElement | null; frameId: number }`
return { w: image.video.videoWidth, h: image.video.videoHeight };
I think that last statement return { w: 0, h: 0 }; is effectively dead code since it can never not be one of those types. If it can be, then I guess the types are wrong.
Alternatively, we could turn typeof image === "object" && "video" in image && image.video into a type guard function that can be reused in other parts of this file.
There was a problem hiding this comment.
I think rearranging things that way works, but the type checker isn't happy:
'image.video' is possibly 'null'.ts(18047)
const image: {
video: HTMLVideoElement | null;
frameId: number;
}
Which is how I ended up with that weird guard statement. I will try the solution mentioned in the latter comment.
| lastY: number; | ||
| } | null>(null); | ||
| const [spaceHeld, setSpaceHeld] = useState<boolean>(false); | ||
| const [cursorDisplay, setCursorDisplay] = useState<string>("crosshair"); | ||
| const [frameCount, setFrameCount] = useState<number>(0); | ||
| const [startTime, setStartTime] = useState<number>(performance.now()); | ||
|
|
||
| // Helper to get dimensions safely | ||
| const getDimensions = () => { |
There was a problem hiding this comment.
If this uses the return type of other hooks (e.g. image comes from useContext) should it be wrapped in useCallback with a dependency list?
There was a problem hiding this comment.
Please correct me if I'm wrong, but I don't think the fact that image comes from useContext necessarily implies that we should use useCallback. It is more if we want to memoize this function if it is causing un-necessary re-renders. The only dependency in this function is image - so we don't gain anything by using useCallback with image in the dependency list.
|
|
||
| // Report the size back to the provider | ||
| reportSize(Math.floor(width), Math.floor(height)); | ||
| }, 100); |
There was a problem hiding this comment.
This 100 might be better as a constant that describes what it does
There was a problem hiding this comment.
Made the default 100 ms in the debounce function instead.
| ctx.drawImage(source.video, 0, 0); | ||
| } else if (source instanceof ImageBitmap) { | ||
| ctx.drawImage(source, 0, 0); | ||
| if (typeof image === "object" && "video" in image && image.video) { |
There was a problem hiding this comment.
As mentioned above, this check is in multiple places so maybe could be a type guard function:
const isVideo = (image: unknown): image is { video: HTMLVideoElement | null; frameId: number } => image !== null && typeof image === "object" && "video" in image && image.video
or bet yet I guess { video: HTMLVideoElement | null; frameId: number } could have a type alias defined in image-context.tsx
type VideoFrame = { video: HTMLVideoElement | null; frameId: number };
const isVideo = (image: unknown): image is VideoFrame => image !== null && typeof image === "object" && "video" in image && image.video
| function CameraControlWebsocketH264Demo() { | ||
| const [mousePos, setMousePos] = useState<{ x: number, y: number, intensity: number } | null>(null); | ||
| const [clickPos, setClickPos] = useState<{ x: number, y: number, intensity: number } | null>(null); | ||
| const api = useMemo(() => h264FetchApi("localhost:9999"), []); |
There was a problem hiding this comment.
How will this work in the docs if you don't have something running at localhost?
There was a problem hiding this comment.
Yeah it won't. I wonder if there is a way to fake it for the docs.
There was a problem hiding this comment.
Maybe we can implement a mock context that just emits some test images in a loop or something. If we mock out the API, we would still need a mock websocket that emits h264 I guess which sounds hard.
| configuredRef.current = false; // force reconfigure | ||
| void tryConfigure(); | ||
| } else { | ||
| /**/ |
| } | ||
|
|
||
| export const WebsocketH264Provider: React.FC<WebsocketH264ProviderProps> = ({ children, wsUrl }) => { | ||
|
|
||
| export const WebsocketH264Provider: React.FC<WebsocketH264ProviderProps> = ({ |
There was a problem hiding this comment.
Might be good to have just a short doc explaining that this should be used with the CameraControl component
| width: number; | ||
| height: number; | ||
| }) => void; | ||
| sizeFollowsImage?: boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
Might be good if this had a short doc explaining what this component is
Yes. Or if you use the Eiger stream I think you need to use localhost/test |
This has turned into a lot of code and needs review. I'm expecting to need to make changes but need others to start looking at it.