Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

Feature/ex 110 use camera and mic hook #39

Merged
merged 30 commits into from
Aug 31, 2023

Conversation

kamil-stasiak
Copy link
Contributor

No description provided.

examples/readme.md Outdated Show resolved Hide resolved
examples/use-camera-and-microphone-example/README.md Outdated Show resolved Hide resolved
examples/use-camera-and-microphone-example/public/vite.svg Outdated Show resolved Hide resolved
examples/use-camera-and-microphone-example/src/App.tsx Outdated Show resolved Hide resolved
examples/use-camera-and-microphone-example/src/App.tsx Outdated Show resolved Hide resolved
src/create.tsx Outdated Show resolved Hide resolved
@Crackhoff
Copy link
Contributor

Crackhoff commented Aug 30, 2023

Bug: When I connect and stop streaming either audio or video sometimes whole room crashes

@Crackhoff
Copy link
Contributor

When I have auto streaming on and I connect to the server, on dashboard I see two video tracks. What's the cause? If one sends both audio and video, why there is another? If they're both video-only, what happens with audio and why there are two of them?

Copy link
Contributor

@Crackhoff Crackhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the hook itself, but I have some questions about the example

examples/use-camera-and-microphone-example/src/App.css Outdated Show resolved Hide resolved
examples/use-camera-and-microphone-example/src/App.tsx Outdated Show resolved Hide resolved
examples/use-camera-and-microphone-example/src/App.tsx Outdated Show resolved Hide resolved
</div>
</div>
<div>
Streaming
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add colons to this and "Local" and make bigger gap between the local video/audio and this. For a while I thought this is other way round and was a bit confused. This would prevent such confusion

Copy link
Contributor Author

@kamil-stasiak kamil-stasiak Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Now it looks like this:
image


return (
<div className="flex flex-row flex-nowrap justify-center border-4">
<canvas ref={canvasRef} width={200} height={100}/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why so narrow? 480 would work too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Now the canvas width matches the width of its parent container

JellyfishContextProvider,
useCameraAndMicrophone,
useSelector
} = create<PeerMetadata, TrackMetadata>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my tutorial I'm using create in the App component. I'm not saying it's better but I think that using both is not ideal. What's preferred option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best option would be to extract this 'setup' to another file

@kamil-stasiak
Copy link
Contributor Author

Bug: When I connect and stop streaming either audio or video sometimes whole room crashes

I think it's a bug in Jellyfish, and I believe it's already fixed. Please check the latest commit on the 'main' branch

When I have auto streaming on and I connect to the server, on dashboard I see two video tracks. What's the cause? If one sends both audio and video, why there is another? If they're both video-only, what happens with audio and why there are two of them?

And I think it'a a bug in jellyfish-dashboard. We need to address it in a separate task

@kamil-stasiak kamil-stasiak marked this pull request as ready for review August 31, 2023 12:46
Copy link
Contributor

@Crackhoff Crackhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good!

readme.md Outdated

Prerequisites:

- Running [Jellyfish](https://github.com/jellyfish-dev/jellyfish) server.
- Created room and token of peer in that room.
You u can use [dashboard](https://github.com/jellyfish-dev/react-client-sdk/tree/main/examples/dashboard) example to create room and peer token.
You u can use [dashboard](https://github.com/jellyfish-dev/jellyfish-dashboard) to create room and peer token.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, "u"

@kamil-stasiak kamil-stasiak merged commit 29392f9 into main Aug 31, 2023
3 checks passed
@kamil-stasiak kamil-stasiak deleted the feature/EX-110-useCameraAndMic-hook branch August 31, 2023 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants