Canvas Recording and Video resolution change #446

Forchapeatl · 2022-08-13T08:42:53Z

Canvas Recording and Video resolution change
Fixes #418
fount at https://forchapeatl.github.io/infragram/indexs.html
Passed Test Case

Resolution Change.
Canvas Recording.

Failed Test Case

Canvas assumes wrong size upon full screen exit.

a0e9f751-ee48-40f2-9ec3-02b215c9.mp4

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

Canvas Recording and Video resolution change

gitpod-io · 2022-08-13T08:42:56Z

Forchapeatl · 2022-08-13T22:02:55Z

@jywarren , @TildaDares and @stephaniequintana may I please have your reviews on this PR ?

jywarren

This looks great, @Forchapeatl! I left some questions and a couple ideas. Thank you!

jywarren · 2022-08-14T16:59:45Z

src/ui/interface.js

@@ -228,4 +230,67 @@ module.exports = function Interface(options) {
    $("[rel=popover]").popover()
    return true;
  });
+
+  //Start Handle multiple webcam resolutions


Here I think it's important to say what we're changing the resolution of: the incoming video stream? The canvas? The outgoing saved video?

Hi @Forchapeatl what do you think about this function name? Thanks!

Oh I don't see the change, did you commit it?

jywarren · 2022-08-14T17:02:11Z

src/ui/interface.js

+  const stream = canvas.captureStream(); // grab our canvas MediaStream
+  const rec = new MediaRecorder(stream); 
+
+  function exportVid(blob) {


This looks great. It's convenient for this all to be near the interface code, but do you imagine a possible future reorganization where it could live in... i'm not sure, the /src/io/ folder? Somewhere else? Since it's not technically part of the interface. We don't have to think about this yet, but we could put a comment in here to eventually move it, or open an issue to think about this in the future.

jywarren · 2022-08-14T17:03:30Z

src/ui/interface.js

+  const ctx = canvas.getContext('2d');
+  var x = 0;
+  const stream = canvas.captureStream(); // grab our canvas MediaStream
+  const rec = new MediaRecorder(stream); 


Does this cost any performance to start here, i.e. should we think about only setting this up if people start using video? Or, is it OK and not too much computational work?

jywarren · 2022-08-14T17:05:34Z

src/ui/interface.js

+    changeResolution('800px','600px')
+  });
+  $('#hd').click(function(e){
+    changeResolution('2400px','1800px')


Ah, here, should we stick to a standard definition of HD? This table was kind of helpful... we might say "720 HD" or "1080 HD"?

https://en.wikipedia.org/wiki/List_of_common_resolutions#Digital_Standards

I see you addressed this! 👍🏽

Done as requested

stephaniequintana · 2022-08-15T18:24:15Z

src/ui/interface.js

+  $('#startRecord').click(function(e){
+    const chunks = [];
+
+    rec.ondataavailable = e => chunks.push(e.data);
+    rec.onstop = e => exportVid(new Blob(chunks, {type: 'video/h264'}));
+    rec.start();
+    document.getElementById('startRecord').style.display='none';
+    document.getElementById('stopRecord').style.display='block';
+  })
+  $('#stopRecord').click(function(e){
+    rec.stop();
+    document.getElementById('stopRecord').style.display='none';
+    document.getElementById('startRecord').style.display='block'; 
+    document.getElementById('downloadButton').style.display='block'; 
+  })


As a whole, it's functioning well when I test it. My only suggestion would be to consider changing the background color of the record button to red while recording. Perhaps in addition to toggling the display from none/block we can add that here. Great job, @Forchapeatl 🚀

Before Recording

During Recording

After Recording

jywarren · 2022-08-15T19:28:39Z

Love the color change idea. For accessibility, it's often a good idea to change the icon of the button too, or to invert the colors, so the icon is dark and the background light (or reverse depending). That way it still works for folks who don't see colors the same way. You could make a CSS class which overrides the styles and toggle that class in jQuery, or you could swap a FontAwesome icon by changing the classname.

…

On Mon, Aug 15, 2022 at 2:25 PM Stephanie Quintana ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ui/interface.js <#446 (comment)>: > + $('#startRecord').click(function(e){ + const chunks = []; + + rec.ondataavailable = e => chunks.push(e.data); + rec.onstop = e => exportVid(new Blob(chunks, {type: 'video/h264'})); + rec.start(); + document.getElementById('startRecord').style.display='none'; + document.getElementById('stopRecord').style.display='block'; + }) + $('#stopRecord').click(function(e){ + rec.stop(); + document.getElementById('stopRecord').style.display='none'; + document.getElementById('startRecord').style.display='block'; + document.getElementById('downloadButton').style.display='block'; + }) As a whole, it's functioning well when I test it. My only suggestion would be to consider changing the background color of the record button to red while recording. Perhaps in addition to toggling the display from none/block we can add that here. Great job, @Forchapeatl <https://github.com/Forchapeatl> 🚀 — Reply to this email directly, view it on GitHub <#446 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J4A6QKV3XBEUOTA5QDVZKDQNANCNFSM56NZK7CQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

used standard definition of HD

Included boolean variables

Changed Icon color while recording

Changed icons for recording control

used requested indentation on boolean variables

Forchapeatl · 2022-08-17T13:53:36Z

@jywarren this PR is ready to merge. May I ?

re-commit

jywarren · 2022-08-19T16:30:21Z

Hi @Forchapeatl I had not yet approved this one because i wanted to know your views on the questions above - can you reply up there? I'd like to hear from you on each of them, and had hoped for the function changeResolution to be changed in this PR. No worries, these weren't critical changes, but please do your best to respond to questions and clarifications before merging, thank you!

Forchapeatl · 2022-08-20T00:47:11Z

Hi @Forchapeatl I had not yet approved this one because i wanted to know your views on the questions above - can you reply up there? I'd like to hear from you on each of them, and had hoped for the function changeResolution to be changed in this PR. No worries, these weren't critical changes, but please do your best to respond to questions and clarifications before merging, thank you!

Sorry about that @jywarren , I didn't realize my poor actions. For this behavior I am very sorry. It wont happen again.

jywarren · 2022-08-22T12:07:39Z

No worries! Let me know what your responses are to the questions!

…

On Fri, Aug 19, 2022, 8:47 PM FORCHA ***@***.***> wrote: Hi @Forchapeatl <https://github.com/Forchapeatl> I had not yet approved this one because i wanted to know your views on the questions above - can you reply up there? I'd like to hear from you on each of them, and had hoped for the function changeResolution to be changed in this PR. No worries, these weren't critical changes, but please do your best to respond to questions and clarifications before merging, thank you! Sorry about that @jywarren <https://github.com/jywarren> — Reply to this email directly, view it on GitHub <#446 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J33FNYYK2UYMKDJF4DV2ATJVANCNFSM56NZK7CQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Canvas Recording and Video resolution change

31d8125

Canvas Recording and Video resolution change

Forchapeatl added 2 commits August 13, 2022 09:44

Delete infragram2.js

c6689af

Canvas recording and Resolution Change

c7a9b77

Forchapeatl requested a review from jywarren August 13, 2022 22:00

jywarren reviewed Aug 14, 2022

View reviewed changes

stephaniequintana reviewed Aug 15, 2022

View reviewed changes

Forchapeatl added 7 commits August 17, 2022 01:43

used standard definition of HD

48f3780

used standard definition of HD

used standard definition of HD

8bb68dd

used standard definition of HD

Included boolean variables

e750bc0

Included boolean variables

Added comment on Resolution handle

4628b59

Changed Icon color while recording

f656b1f

Changed Icon color while recording

Changed icons for recording control

6112ffd

Changed icons for recording control

used requested indentation on boolean variables

5d8b7d8

used requested indentation on boolean variables

re-commit

a922019

re-commit

Forchapeatl merged commit 853857a into main Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canvas Recording and Video resolution change #446

Canvas Recording and Video resolution change #446

Forchapeatl commented Aug 13, 2022 •

edited

Loading

gitpod-io bot commented Aug 13, 2022

Forchapeatl commented Aug 13, 2022

jywarren left a comment

jywarren Aug 14, 2022

jywarren Aug 17, 2022

jywarren Aug 17, 2022

jywarren Aug 14, 2022

jywarren Aug 14, 2022

jywarren Aug 14, 2022

jywarren Aug 17, 2022

Forchapeatl Aug 17, 2022

stephaniequintana Aug 15, 2022

Forchapeatl Aug 17, 2022

jywarren commented Aug 15, 2022 via email

Forchapeatl commented Aug 17, 2022

jywarren commented Aug 19, 2022

Forchapeatl commented Aug 20, 2022 •

edited

Loading

jywarren commented Aug 22, 2022 via email

Canvas Recording and Video resolution change #446

Canvas Recording and Video resolution change #446

Conversation

Forchapeatl commented Aug 13, 2022 • edited Loading

gitpod-io bot commented Aug 13, 2022

Forchapeatl commented Aug 13, 2022

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Aug 15, 2022 via email

Forchapeatl commented Aug 17, 2022

jywarren commented Aug 19, 2022

Forchapeatl commented Aug 20, 2022 • edited Loading

jywarren commented Aug 22, 2022 via email

Forchapeatl commented Aug 13, 2022 •

edited

Loading

Forchapeatl commented Aug 20, 2022 •

edited

Loading