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 configurations to let the user choose the webcam device, the aspect ratio and the resolution #579

Merged
merged 24 commits into from
May 6, 2020

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Apr 22, 2020

Closes #546
Closes #16
Fixes #442
Fixes #217

Deployed here

This PR changes the video source step. The previews don't have a small text description below them anymore. Instead, there is an overlayed cog icon in the bottom right corner. Clicking that brings up a configuration dialog and a small information bubble, both also overlayed over the video. In the case of the camera stream, the user can select a specific webcam device, the aspect ratio and the "quality" (just a more user-friendly word for resolution). In the case of the desktop stream, the user can only select a quality.

Open questions:

  • Should we make the overlay fully opaque instead of the current white with opacity 95%? My initial idea for transparency was that the user would not accidentally wonder about "stuff missing from the bottom of the video". However, I had to increase the opacity to 95% as otherwise the configuration dialog was really hard to read. The background video can still sometimes make reading a bit harder. -> I made it fully opaque now.
  • Will we run in problems when browser do not return the exact resolution/aspect ratio? The resolution and aspect ratios are passed as ideal and not exact. That's because (a) we want to allow a bit of flexibility to the browser and (b) using exact has even stranger browser behavior. My assumption is that most users don't even open these settings, and if they do that they only change the video device. The settings are hidden by default for a reason. -> I added a short explanation text.

We also should add a device chooser for the sound devices, but this PR is already large enough as is. Sound has to be in another PR.

I just noticed that we should do that to avoid clash with other things
deployed on the same domain.
Having tested this with a number of browser and devices, it looks like
this doesn't work most of the time. Right now we request it with
`ideal`, but even with `exact` it doesn't seem to make a difference. So
I assume we will disable that again as otherwise users are too easily
confused. However, most code in this commit is also used for the
quality selector.
This feature is still not finished. Open tasks/questions:
- Should the setting passed as `ideal`? Firefox does some strange stuff
  with this. Passing as `exact` doesn't work with desktop capture in
  Firefox or Chrome. We could pass it as `max` and/or `min` but... that
  seems wrong.
- We need to check the `maxHeight` in settings. Still unclear how we
  communicate this to the user.
- It would be beneficial if you could somehow check which quality
  settings won't work. And/or better signal to the user if a quality
  setting worked.
When requesting specific aspect ratios or resolutions, sometimes the
browser can't stick to that request and returns a video stream ignoring
those contraints. This commit colors the preference buttons green or
yellow depending on whether the browser could satisfy the request.
This restructuring removes some duplicate code and is generally easier
to understand.
It is not needed anymore and just makes the algorithm more complex.
Previously, if there was no stream, the resize algorithm would assume
an aspect ratio of 16:9. Which is annoying when the old and the new
stream are 4:3 for example.
Now the user can scroll and the X is always visible
I asked a few other people about those texts, including a certainly
technically non-versed user :P We tweaked the texts and with this,
everything should be pretty clear to the user. Particularly the "if in
doubt" part of the text really helps as inexperienced users actually
stick to those advices quite well.
Previously there existed a number of box where the box would be too
high or too small.
@LukasKalbertodt
Copy link
Member Author

I declare this PR as ready now. It has eaten way too much time already. I updated several things, mostly polish, design, usability on small screens and most importantly: a short text explaining the the preferences to the user. I let a few people test this and I think it is fine the way it is now.

However, there are still tons of possible improvements for the future:

  • Browser bugs. Lots and lots. I will try to report all bugs I noticed, but holy moly.
  • Use MediaStreamTrack.applyConstraints instead of rerequesting the stream. This is the preferred way to change these settings. However, it comes with strange behaviors and browser bugs on its own.
  • With applyConstraints it should be possible to easily identify supported resolutions and aspect ratios. Then we can indeed check beforehand and only show supported options.
  • The sliding in of the settings menu should preferably be done with margin-bottom or something instead of height. Currently there is a progress bar visible during the transition which is meh.

@lkiesow
Copy link
Contributor

lkiesow commented Apr 30, 2020

Generally, looks good. I do have two complaints though:

  • Changing settings can lead to a jumping UI (see below) which is pretty annoying.
  • The settings menu uses about half of the camera preview making it hard to judge how the image really looks like. It basically requires me to change the settings and then close the menu to evaluate the settings. At the same time, half of the screen is empty.

jumping-ui

@LukasKalbertodt
Copy link
Member Author

Good points.

First regarding space usage: I noticed this as well but haven't come up with a good solution yet. However, I wanted to remove this max width container for a long time already. I wanted to create another PR for it or include it in some other style changes, but I can try to see if it at least improves this issue.

The jumping UI is not a bug but a feature :D Well, not that specifically, but in general we want Studio to react to changes in video dimensions to layout the page in a space optimal manner. Another example is a phone that you turn by 90° and thus swapping height and width of the video dimension. Anyway, I see how it is a problem here. We can also see if removing the max-width container solves the issue in most situations.

@LukasKalbertodt
Copy link
Member Author

LukasKalbertodt commented May 4, 2020

I removed the max-width and I think it improves this a lot. The most common use cases are probably:

  • User on 16:9 screen with one 16:9 video source
  • User on 16:9 screen with two 16:9 video sources
  • User on 16:9 screen with one 16:9 and one 4:3 video source

I tested those cases on a screen with virtual resolution (think OS UI scaling) of between roughly 1600x900 and 1920x1080. Those virtual resolutions are the most common ones, I'd assume. And in all those cases, no UI jumping was happening, the video settings menu covered less than half of the video preview and most of the screen space was utilized.

The settings menu uses about half of the camera preview making it hard to judge how the image really looks like. It basically requires me to change the settings and then close the menu to evaluate the settings.

I also don't think this is too bad. There is not really anything "artistic" about those settings that would require close inspection of the video stream (not like, e.g. a video filter).

So I think this PR is ready to merge. It does not make anything worse (AFAICT), and while the new feature is not perfect yet it already helps most users who need it.

@LukasKalbertodt
Copy link
Member Author

I talked to Lars and he agrees that this can be merged as is. Still several possible improvements, but this PR is already large enough.

@LukasKalbertodt LukasKalbertodt merged commit e3507de into elan-ev:master May 6, 2020
@LukasKalbertodt LukasKalbertodt deleted the webcam-selection branch May 6, 2020 12:59
lkiesow pushed a commit that referenced this pull request May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants