-
-
Notifications
You must be signed in to change notification settings - Fork 459
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
Fix high resolution camera performance on mobile by allowing a default resolution #347
Conversation
-Allow default resolution of camera to be set to 1280x720 for devices that have high resolution cameras (prevents crash behaviour on mobile)
This reverts commit a13a722.
This reverts commit fea330c.
…est default resolution on mobile -Prevents huge performance issues on phones with high resolution cameras when using the sample app
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/ImageSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/WebCamSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/ImageSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/ImageSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/WebCamSource.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change suggestions - I learnt some good practices for PRs here also which I will use in future when making PRs. If you need any explicit changes to this, let me know!
Co-authored-by: Junrou Nishida <[email protected]>
…resolution within 300px, else return the first resolution found
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/WebCamSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/ImageSource.cs
Outdated
Show resolved
Hide resolved
…ce.cs Co-authored-by: Junrou Nishida <[email protected]>
Updated now only WebCamSource does the switching, the only outstanding issue I can see (other than me hopefully finally sorting the .cs scripts to be lf not crlf - sorry about that!) is that the resolution dropdown in the config menu doesn't actually show the current resolution being used when it is set automatically in WebCamSource.cs. Wondering what is the best way to approach that? |
Revert all the changes in ImageSourceConfig.cs |
All sorted there now, if there is anything else needing to be changed on this, let me know. I'm going to get back to looking into the graph latency issue this week but I'm pretty stumped with that as I'm still learning how the new sample app works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think we're almost there.
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/WebCamSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/WebCamSource.cs
Outdated
Show resolved
Hide resolved
Assets/Mediapipe/Samples/Common/Scripts/ImageSource/WebCamSource.cs
Outdated
Show resolved
Hide resolved
…ImageSource/WebCamSource.cs Co-authored-by: Junrou Nishida <[email protected]>
…s/Common/Scripts/ImageSource/WebCamSource.cs Co-authored-by: Junrou Nishida <[email protected]>
All done, thanks for your help on this also 🦾 Seems the Github pull request commits I can make off your changes also makes the line endings in files changed on Github crlf - I'll have to see if that's a setting on my end on the Github website! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM
Use a manually set default resolution within 300px to choose the nearest default resolution on mobile. Is auto-selected if the value matches or if it is within the 300px range
-Prevents huge performance issues on phones with high resolution cameras when using the sample app