Implement mirrored video feature for createCapture (#6441) #6703
Conversation
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for taking this on! I've added a few comments, let me know what you think!
| * </code> | ||
| * </div> | ||
| */ | ||
| p5.prototype.createCapture = function(...args) { |
There was a problem hiding this comment.
Can we update the doc comments above this to mention the flipped parameter, and maybe add an example that uses it?
There was a problem hiding this comment.
I will soon update the inline documentation and give a simple good-to-go example.
| videoEl.width = domElement.width; | ||
| videoEl.height = domElement.height; | ||
| if (flipped) { | ||
| videoEl.elt.style.transform = 'scaleX(-1)'; |
There was a problem hiding this comment.
If you call yourCapture.hide() and image(yourCapture, 0, 0) to draw it to the canvas, is it flipped there too? If not, we may need to update the part of the code where media elements draw to their internal canvas and draw it scaled there too (see this comment for details: #6441 (comment))
There was a problem hiding this comment.
There was a problem hiding this comment.
This time i have changed the _ensureCanvas() so that it works fine.
src/dom/dom.js
Outdated
| constraints = arg; | ||
| flipped = constraints.flipped || false; | ||
| } | ||
| else if (typeof arg === 'boolean') { |
There was a problem hiding this comment.
I think it's more futureproof to force people to use the settings object (createCapture(VIDEO, { flipped: true })) than to add additional overloads with a boolean argument (createCapture(VIDEO, true)) in case we need to add any other additional boolean settings. How do you feel about taking out this branch?
src/dom/dom.js
Outdated
| else if (typeof arg === 'object') constraints = arg; | ||
| else if (typeof arg === 'function') callback = arg; | ||
| else if (typeof arg === 'object') { | ||
| constraints = arg; |
There was a problem hiding this comment.
This gets passed into getUserMedia below. We might want to extract just the bits we need for constraints so we don't accidentally send extra info in in the future if we add anything to the settings object. Maybe we could do something like this?
constraints = { video: arg.video, audio: arg.audio };
src/dom/dom.js
Outdated
| if (arg.flipped !== undefined) { | ||
| flipped = arg.flipped; | ||
| } else { | ||
| constraints = arg; |
There was a problem hiding this comment.
Right now I think this means we either can provide flipped, or we can provide constraints on the capture size. I think we want to support both at once, but just with the flipped arg removed. Maybe something like:
if (arg.flipped !== undefined) {
flipped = arg.flipped;
}
constraints = { ...arg };
delete constraints.flipped;
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
I think this is good to go now! Since we've started our release process with a beta build, I'm going to hold off on merging this until after we finish the release so that we don't accidentally introduce new bugs during the testing period. I'll merge this right after that though!
2 participants
Resolves #6441
Changes:
createCapture.createCapturefunction to accept a boolean parameter for flipping the video.Screenshots of the change:
PR Checklist
npm run lintpasses