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

Add Ability to Update Video Resolution through getUserMedia #173

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

chukohsin
Copy link
Contributor

@chukohsin chukohsin commented Aug 29, 2020

Decription

This update is based on a previous PR

This adds the ability for user to update video resolution through getUserMedia constraints. Currently we hardcoded five resolution options (1080p, 720p, 480p, 360p, 240p). An error message would be shown if any error occurs. (e.g. overconstrained, not allowed etc.). A success message would be shown when resolution gets updated successfully.

SW-select-resolution


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

  • tests pass -- see README.md for how to run them
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request are descriptively named
  • if possible, multiple commits squashed if they're smaller changes
  • reviewed/confirmed/tested by another contributor or maintainer

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

@gitpod-io
Copy link

gitpod-io bot commented Aug 29, 2020

@jywarren
Copy link
Member

Hello, would you be able to rebase this over the latest main branch following the merge of #172? Thank you!

@jywarren
Copy link
Member

This looks really good on close review. Would it be possible to add another option to change the camera to the rear-facing camera for mobile devices? I see some usage in temasys/adapterjs here:

https://github.com/Temasys/AdapterJS/blob/e671a3c1ce9a4865c1f77a7cd869931cb8bf23c5/publish/adapter.debug.js#L4303-L4319

I think that means the face option can be configured to rear and perhaps the first step is to do:

face: {
  ideal: 'rear'
}

so it defaults to rear camera, which is how the spectrometer would attach:

image

This could easily be its own separate PR, but as this PR touches the same code and needs a rebase, perhaps it's an easy addition? Thank you!!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

This would be the only change on top of the rebase! Thanks!

examples/capture/getUserMedia.js Show resolved Hide resolved
chukohsin and others added 3 commits September 22, 2020 20:37
This adds the ability for user to update video resolution through getUserMedia constraints. Currently we hardcoded five resolution options (1080p, 720p, 480p, 360p, 240p). An error message would be shown if any error occurs. (e.g. overconstrained, not allowed etc.). A success message would be shown when resolution gets updated successfully.
@jywarren
Copy link
Member

Rebased it, let's see!

@jywarren
Copy link
Member

This looks good, it didn't use the environment-facing mode in Android Chrome but maybe it'll do so on iOS...

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 this pull request may close these issues.

2 participants