Conversation
|
This is great but doesn't handle what might happen if either of the streamer or the viewer happens to disconnect, close the tab, etc |
|
I will fix this 👍 |
|
@gbowne1 you can check now i have fixed the issues and made the following changes: I created a dedicated Watch page (watch.html+ watch.js) where viewers can join any live stream by entering the stream room ID. |
|
Do you have screenshots of the application you can contribute to Discussion tab? Make a new Discussion there. Especially to show your work I'll review your PR momentarily Could you also help us also review PRs in this Pull Request tab? I'll merge them as soon as there are a minimum of 1 or 2 accepted reviews ✅ https://github.com/gbowne1/codestream/pull Thanks |
|
i have added the screenshots in the discussion tab you can view it and am ready to review PRs!. Thanks |
There was a problem hiding this comment.
I checked out this change locally for testing and review for merge
This is a excellent initial implementation live broadcast for this stream application.
I also always appreciate screenshots, short video clips, etc of working changes. Logs, passing tests, other stuff useful so that developers have some idea what to expect when working on issues.
It appears to work properly allowing users to go live
Thanks for the opportunity to review your PR and for your contribution to this project
shishir-21
left a comment
There was a problem hiding this comment.
Reviewed the PR — the implementation looks good.
There are merge conflicts that need to be resolved before merging.
Happy to re-review once that’s done 👍
|
Well, we can do that via the GitHub merge conflict resolve thing or rebase and apply or? |
|
Either approach works 👍 Since the conflicts are limited to If you prefer a cleaner history, rebasing onto |
|
That sounds okay. I'm not terribly worried about history. I'll let any of the collaborators help resolve conflicts if they wish. Theres a few of PRs that have conflicts now. I tried to pick initial ones to merge that would have the least issues being merged. Github allows maintainers to commit fixes to open PRs so I'll allow that too Workflow runs are 👎 right now. I'll merge anything that's got the proper ✅ reviews and still a green merge button.. probably the best way for now during this round. |
|
This PR looks good. Resolving the merge conflicts right now, will approve once the commit has been made. |
|
I have resolved the merge conflicts, let me know if they're okay. |
|
Are the CI checks failing because of the changes or are there issues with the CI code itself? |
|
Workflow still fails... separate issue. It's an issue with the CI code |
|
This needs a rebase I think. I merged #87 and a long outstanding PR that had enough reviews to get merged Make sure everyone is up to date and sync |
shishir-21
left a comment
There was a problem hiding this comment.
I reviewed the implementation focusing on functionality. The WebRTC signaling flow between broadcaster and viewer is correctly implemented. The peer connection handling, ICE exchange, viewer tracking, and disconnect cleanup logic are all structured properly.
The feature works end-to-end and satisfies the “Go Live” requirement.
|
I reviewed this and it looks good functionally, but has merge conflicts with master. |
|
@shishir-21 @glenjaysondmello |
1 similar comment
|
@shishir-21 @glenjaysondmello |
Changes
**Backend **
GET /api/live-streamsto list active streamsFrontend - Broadcaster (
go-live.html,src/js/go-live.js)Frontend - Viewer (
src/js/main.js)Dependencies Added
socket.io- Server-side real-time communicationsocket.io-client- Client-side Socket.IO