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

Testing new p5 mirror function #106

Closed
ziyuan-linn opened this issue Mar 22, 2024 · 5 comments · Fixed by #127
Closed

Testing new p5 mirror function #106

ziyuan-linn opened this issue Mar 22, 2024 · 5 comments · Fixed by #127
Assignees

Comments

@ziyuan-linn
Copy link
Member

I was testing the new p5 mirror function released in 1.9.1 along with handPose.

When mirroring is enabled with the following code:

video = createCapture(VIDEO, { flipped: true });

this happens:
not_mirrored
The webcam video is mirrored correctly but the handPose keypoints are still not mirrored.

ml5 runs inferences on the underlying HTML video element when given a p5 video element, which is probably never mirrored. Mirroring an HTML video element is not very straightforward, and I don't think it would make sense for p5 to do it.

Luckily, the tfjs models all have a flipHorizontal property. We could simply set it to true:

handPose = ml5.handPose({ flipHorizontal: true });

and everything will work correctly:
mirrored

We now have an intuitive two-line solution to the mirroring problem🎉! Perhaps we can add some mirrored examples? We can probably also mention this somewhere in the docs or the website.

@shiffman
Copy link
Member

This is great, thank you for digging into this @ziyuan-linn! Do I understand correctly that this already works without any changes to the ml5.js codebase and it's just a matter of properly documenting this? I wonder if we should create a page on the website all about "flipping video" since it is something that goes across all models. Can we apply the same flipHorizontal property to body pose, face mesh, and even body segmentation?

Maybe we should follow p5.js's lead and introduce a flipped: true property for all models? Is this something that should be passed in when the model is loaded or when startDetect()/startClassify() is called?

@ziyuan-linn
Copy link
Member Author

ziyuan-linn commented Mar 23, 2024

@shiffman Yes, this should work without any changes to the codebase. I think a page on the website about this is a great idea!

All of the image-based models already have a flipHorizontal property, it is the name used by the underlying tfjs models. We could also rename them to flipped for consistency. I don't really feel strongly either way.

For the current ml5's API, options can only be passed in when loading the model. ml5 saves the flipHorizontal value internally until detectStart() is called and passes the value to tfjs at runtime. We could make detectStart() accept an options parameter, but I personally think it introduces unnecessary complexity.

(oops, I accidentally clicked close issue)

@shiffman
Copy link
Member

Ah, that makes sense. I'm of two minds

(1) It's good to use the actual language and properties of the underlying model as much as possible.
(2) Things are cleared when they are aligned with p5.js wherever possible. I also prefer the simplicity / conciseness of flipped.

I think I lean towards going with (2). How about we put flipped in the ml5.js documentation. Then when the user sets the flipped property, we transform it to flipHorizontal underneath the hood. If a user sets flipHorizontal that should also work, but we'll document the models with flipped. What do you think?

I agree we shouldn't add it to detectStart() and do an end run around the model's actual behavior and settings.

This looks so nice and lovely to me:

let video = createCapture(VIDEO, { flipped: true });
let handPose = ml5.handPose({ flipped: true });

@shiffman
Copy link
Member

shiffman commented Apr 2, 2024

@ziyuan-linn it occurred to me today that if you are making some adjustments / updates to the examples, we should include info on how to flip the video in them!

@ziyuan-linn
Copy link
Member Author

@shiffman Sounds good! I think implementing flipped as an alias for flipHorizontal is a good idea. However, only flipHorizontal will be valid until the next release. I can update the examples with flipHorizontal for now.

Do you think the example sketches should be mirrored by default with comments/info on how to set them to not flipped, or vice versa?

@ziyuan-linn ziyuan-linn self-assigned this Apr 2, 2024
@ziyuan-linn ziyuan-linn linked a pull request Apr 22, 2024 that will close this issue
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 a pull request may close this issue.

2 participants