Skip to content
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

Added Streaming functionality for iOS Browsers #442

Merged
merged 16 commits into from
Aug 8, 2022
Merged

Conversation

Forchapeatl
Copy link
Collaborator

@Forchapeatl Forchapeatl commented Aug 5, 2022

Hosted on https://forchapeatl.github.io/infragram/indexs.html

@gitpod-io
Copy link

gitpod-io bot commented Aug 5, 2022

package.json Outdated
@@ -25,11 +25,9 @@
"bootstrap": "~3.4.0",
"bootstrap5": "npm:bootstrap@^5.1.3",
"font-awesome": "~4.7.0",
"getusermedia-js": "~1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Ah, if we remove these here, won't it break v1? Shall we leave them in even though v2 won't be using them?

Copy link
Member

Choose a reason for hiding this comment

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

Or alternatively, should we implement the same changes in index2.html into index.html and fix both v1 and v2 in this respect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OH sorry

Copy link
Collaborator Author

@Forchapeatl Forchapeatl Aug 5, 2022

Choose a reason for hiding this comment

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

Let's not break v1 for now. We can break it after v2 is complete , when @stephaniequintana and I synchronize our codebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jywarren , package.json has been fixed. Please may I merge?

@jywarren
Copy link
Member

jywarren commented Aug 5, 2022 via email

@stephaniequintana
Copy link
Collaborator

FANTASTIC work, @Forchapeatl! Kudos!!! 🚀 🎆

@Forchapeatl
Copy link
Collaborator Author

sorry about that @jywarren it affects the functionality of camera.js a whole lot. As the bootstrap modal is replaced by offcanvas and WebcamVideo element is not created by WebRTC. Should I create a seperate cameraV2.js file ?

@jywarren
Copy link
Member

jywarren commented Aug 6, 2022

sorry about that @jywarren it affects the functionality of camera.js a whole lot. As the bootstrap modal is replaced by offcanvas and WebcamVideo element is not created by WebRTC. Should I create a seperate cameraV2.js file ?

It's ok! @stephaniequintana would you like to explain how we could use the options flag to switch behaviors between the two versions?

@Forchapeatl
Copy link
Collaborator Author

Forchapeatl commented Aug 6, 2022

Hello @jywarren the options.version condition has been included to the camera.js and dist/infragram.js files. Please have a look at the video below . hosted on (version 2 )https://forchapeatl.github.io/infragram/indexs.html and (version 1) https://forchapeatl.github.io/infragram/index.html . May I merge this PR ?

9b0cfba2-1835-4ddf-a9a5-a7fa222e5624.online-video-cutter.com.mp4

@Forchapeatl Forchapeatl merged commit 9547f0b into main Aug 8, 2022
@welcome
Copy link

welcome bot commented Aug 8, 2022

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to http://infragram.org in the next few days.
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

@Forchapeatl Forchapeatl changed the title Added Streaming functionality for Safari Browsers Added Streaming functionality for iOS Browsers Aug 8, 2022
// detection), we handle getting video/images for our canvas
// from our HTML5 <video> element.
if (webRtcOptions.context === "webrtc") {
if(options.version == 1){
Copy link
Member

Choose a reason for hiding this comment

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

This all looks great! Please do try to standardize spacing -- there are some auto-formatters you can use too to achieve this! So, if (options.version == 1) { <= note the spaces. But not a big deal -- it just is nice over the long term (over years) to have the code be very regularly formatted.

Thanks @Forchapeatl!!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ohh, please do you mean indenting ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, actually just notice there's a space between if and ( in line 109, but on 133 that's missing. it's a very small thing and not super important but is kind of nice to preserve completely consistent formatting as much as possible. Thank you!

var canvas, ctx; // Initialize getUserMedia with options
var canvas, ctx;
options.webcamVideoSelector = options.webcamVideoSelector || 'webcamVideoEl';
options.webcamVideoEl = document.getElementById(options.webcamVideoSelector); // Initialize getUserMedia with options
Copy link
Member

Choose a reason for hiding this comment

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

very nice

@jywarren
Copy link
Member

jywarren commented Aug 9, 2022

So, one more thing @Forchapeatl -- here, are you generating the dist/infragram.js and then renaming it dist/infragram2.js? If all the version differences are managed by the options.version conditionals now, can't we begin leaving it as dist/infragram.js instead of infragram2.js, since both versions will be able to run off the same file?

And thank you for merging this and moving it along -- i think sorry it might have needed just one more review before doing so but it's not a big deal since you did confirm that the previous version was working fine!

However if you open a new PR where we just use dist/infragram.js we'll have to be extra sure all features work in both versions since they'll all be running off the same file from there onward. Make sense?

Thanks!!!

@jywarren
Copy link
Member

jywarren commented Aug 9, 2022

Also just cc'ing @stephaniequintana can you and @Forchapeatl just put a 👍 on the above so I know we're all coordinated? Thank you! Exciting!!!!

@jywarren
Copy link
Member

jywarren commented Aug 9, 2022

And thanks for your patience this week as I'm teaching long hours so less able to reply quickly! Definitely continue relying on reviews from each other and other mentors as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GSoC Infragram.org full-screen UI and video upload Discussion and Planning
3 participants