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

Process Video Locally Infragram.org #421

Closed
wants to merge 11 commits into from
Closed

Conversation

Forchapeatl
Copy link
Collaborator

@Forchapeatl Forchapeatl commented Jun 19, 2022

Process Video Locally
Fixes #418
found at https://forchapeatl.github.io/infragram/index2.html

a2a4f5d9-4f0d-4496-af2b-11858bdb12a8.mp4
  • Allow conversion of entire video.
  • Implement video controls on canvas.

TEST RESULTS

Passed Test cases

  • video can be sleeked through muted / looped / paused and played.
  • video can be processed.
  • Video processing works before and after camera has been on.

Failed Test cases

  • Accommodate large video files > 10MB.

Hello @jywarren @TildaDares @stephaniequintana
The demo video is processed locally , but it might take a while for the video to load up the github page
It seems to work only after camera has been turned on.This bug is taking a while.

  • I plead you give me a week with the bug while I proceed to other mile stones.
  • Is the code base readable (in terms of variable declarations)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

@Forchapeatl Forchapeatl added enhancement in progress discussion ideas to discuss and refine before deciding whether to do them or not gsoc Issues associated with Google Summer of Code(GSOC) labels Jun 19, 2022
@welcome
Copy link

welcome bot commented Jun 19, 2022

Thanks for opening this pull request! This space is protected by our Code of Conduct - and we're here to help.
Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We'll work through them with you! 👍🎉😄


One thing that can help to get started is to make sure you've included a link back to the original issue you're solving, in the format fixes #0000 (for example). And to make sure the PR title describes what you're trying to do! (often it can be the same as the issue title) Thanks! 🙌


Then, you can say hello in our chatroom & share a link to this PR to get a review! 👋 ✅

@gitpod-io
Copy link

gitpod-io bot commented Jun 19, 2022

@jywarren
Copy link
Member

Hi! I hope to have a chance to give this a thorough read through in the next day, but I wonder if the camera start bug has to do with having to wait until it's initialized; there may be a need to listen for the right event?

processor: 'webgl'
});

openInPl = function openInPl() {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should move this into a shareable .js file or into the library itself... Just an idea?

Copy link
Collaborator Author

@Forchapeatl Forchapeatl Jun 20, 2022

Choose a reason for hiding this comment

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

Please help me with more details on this file. I can't seem to find a file named sharable.js in the infragram directory , I am somehow confused on the mentioned library. Please for some detail on this idea.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm typing from phone, I just meant a separate js file we can include into the different html files... A shareable file. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. Noted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you

index3.html Outdated
<br style="clear:both;" />

</div>
<div id="localVideoControls" style="display:none;">
Copy link
Member

Choose a reason for hiding this comment

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

So let's clearly label and explain these to @stephaniequintana who may be soon integrating this code into the new UI!

@@ -0,0 +1,2213 @@
isVideo = false,isCamera=false; // Turns off camera feed
Copy link
Member

Choose a reason for hiding this comment

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

Ah ok! Did you make changes to a copy of dist/infragram.js? So what may be better is to look where in /src/ you want to make the changes, and do them there... Then they'll generate a new dist/infragram.js when we run grunt build. Does that make sense? It compiles the code into /dist/. @cesswairimu or @TildaDares both have experience with this too! No worries we'll help if it doesn't make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes the grunt build command makes sense, thank you.

@jywarren
Copy link
Member

Hi @Forchapeatl - i wanted to try to help you pull apart the new code you've added to the /dist/infragram.js file, and so I made a new PR just to get a good 'diff' view -- at https://github.com/publiclab/infragram/pull/422/files?diff=unified&w=1

What I'm going to try to do is comment on each section and point to where those changes could be made in the /src/ directory. Let's try to make the changes there!

@jywarren
Copy link
Member

OK - it's not too complex - there are about 6 or so sections which need to be copied into corresponding code in /src/io/camera.js and /src/ui/interface.js -- press "load diff" and you'll see my comments at https://github.com/publiclab/infragram/pull/422/files?diff=unified&w=1 !

If you make those changes in the Forchapeatl-patch-1 branch, then run grunt build, you'll generate a new version of the compiled file called dist/infragram.js. Check that file in and then remove /dist/infragram3.js, and point index3.html at /dist/infragram.js, and we should be OK!

@Forchapeatl
Copy link
Collaborator Author

okay @jywarren ,thank you

Set "Id" on Webrtc video ELement
- Included logic to process video locally
- Included  logic for video controls (loop/mute/pause/play and seek)
@Forchapeatl
Copy link
Collaborator Author

Forchapeatl commented Jun 23, 2022

Hello @jywarren , @cesswairimu , @TildaDares the code base has been structured as requested on https://github.com/publiclab/infragram/pull/422/files?diff=unified&w=1

@jywarren
Copy link
Member

This looks excellent @Forchapeatl -- can you run grunt build and check in the updated dist/.... files? Then I'm going to give it a quick test in GitPod and we should be able to merge -- great work!!

@jywarren
Copy link
Member

No rush as it's very late here so I won't get to it until tomorrow. Congrats on the changes, they were complex!

@Forchapeatl
Copy link
Collaborator Author

Thank you, @jywarren

@jywarren
Copy link
Member

Hi @Forchapeatl I didn't see the dist files added again, just wanted to check if you needed any help?

@Forchapeatl
Copy link
Collaborator Author

Forchapeatl commented Jun 25, 2022

Hello @jywarren , all is well with grunt. This PR functionality is a bit limited / buggy . Please I will include the dist file once I am done with these limitations / bugs.

Video processing works only after camera has been on.
Accommodate large video files > 10MB.

Please permit me redirect you to another PR #427

@jywarren
Copy link
Member

Video processing works only after camera has been on.
Accommodate large video files > 10MB.

No problem, but I think the 10MB might be a tough one - i'm not sure how you're storing it but if it's in localStorage, there is a hard JS limit on how much can be stored. If you think it's a big issue, we could push people into the workflow of recording on their device, then uploading the recording and converting it, rather than recording live within the website. What do you think? So people would upload a video file in the same way as they would have uploaded a still image. @stephaniequintana also curious what you think about that workflow as an alternative. It could be simpler and could sidestep some of these issues?

@jywarren
Copy link
Member

Oh darn i'm being silly - this is a prerecorded video, and you're just saying that there was a 10mb limit to the processing of that "upload" - but was my idea correct that it was due to a localStorage file size limitation?

@jywarren
Copy link
Member

I want to say that we have processed larger images in https://github.com/publiclab/image-sequencer/ or https://github.com/jywarren/webgl-distort/ - I could be wrong, but I think we were just keeping such larger amounts of data in memory, or in the HTML DOM itself. Where is the 10mb limit happening?

localVideo.style.display = "none"
localVideo.style.width = "100px";
localVideo.style.height = "50px";
document.getElementById("video-container").appendChild(localVideo);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like everything is happening in the DOM here. Did you get a particular error you can share?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/publiclab/infragram/blob/Forchapeatl-patch-1/src/Infragram.js#L38-L39

Sorry about the mix up @jywarren . I meant that HD videos have a slower frame rate transition on the Infragram canvas.

For example, HD video makes better frame transitions with the below settings

    if (options.processor.type == "webgl") interval = 1;
    else interval = 15;

Copy link
Member

Choose a reason for hiding this comment

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

I see, ok! What did you think about making this driven by frame conversion completion instead of interval?

@stephaniequintana stephaniequintana mentioned this pull request Jul 7, 2022
5 tasks
@Forchapeatl
Copy link
Collaborator Author

Video Processing works now works without clicking the webcam button.

ad91639c-8660-4214-9962-ce3025b8bad1.mp4

@jywarren
Copy link
Member

Wow excellent! @Forchapeatl how are you thinking in terms of what should be ready to merge next? Thank you!

@Forchapeatl
Copy link
Collaborator Author

You 're welcome @jywarren

@jywarren
Copy link
Member

jywarren commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion ideas to discuss and refine before deciding whether to do them or not enhancement gsoc Issues associated with Google Summer of Code(GSOC) in progress
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
2 participants