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

Reconsider adding aspectRatio: 16 / 9 to default camera constraints #1571

Closed
VincentVanlaer opened this issue Apr 22, 2021 · 12 comments · Fixed by #1576
Closed

Reconsider adding aspectRatio: 16 / 9 to default camera constraints #1571

VincentVanlaer opened this issue Apr 22, 2021 · 12 comments · Fixed by #1576

Comments

@VincentVanlaer
Copy link

Previously, this was added in the default camera constraints, but was removed in #1066. For my webcam (Logitech C920), this causes an aspect ratio of 4:3 to be chosen, even if 16:9 is supported. This is because if "aspectRatio" is not specified in the ConstraintSet, the browser is free to chose any aspect ratio (as long as other constraints are satisfied) and my camera supports 4:3 and 16:9 for a height of 720 pixels.

First of all, the Media Capture and Streams W3C standard indicates that unless specified in the advanced section of the constraints, bare values (i.e. no "min", "max", ...) should be consider as "ideal". This means that the constraint is not required. A camera that does not support 16:9 should not cause getUserMedia to fail if "aspectRatio: 16 / 9" is specified. If it does, then this is a browser bug and should be reported to the developers of the browser. Therefore, it seems to me that #1066 was a workaround for a buggy browser.

Adding "aspectRatio: 16 / 9" will also improve the user experience, as the jitsi-meet UI is built for a 16:9 aspect ratio. If the camera is not operating in a 16:9 aspect ratio then black borders will be added to the video feed, deteriorating the user experience, especially if the user knows that their camera supports 16:9.

It is of course currently already possible for administrators of a Jitsi instance to add this constraint themselves in the jitsi-meet config. This feature however is quite difficult to discover for users. Personally I first though that this might be a browser bug or that there was something wrong with my camera and only after a while found jitsi/jitsi-meet#6244. This with the background of being a maintainer of a Jitsi instance. I have heard from another user who had the same issue, but had no clue that this was caused by missing configuration, and therefore did not consider reporting it.

I would therefore like the change in #1066 to be reconsidered and potentially be reverted.

@saghul
Copy link
Member

saghul commented Apr 22, 2021

That's odd because I get 16/9 with the same camera. Maybe different revisions? We tried a number of things like specifying height and width with mixed results...

Note that you can add that constraint yourself, this is just the default.

@saghul
Copy link
Member

saghul commented Apr 22, 2021

Ping @jallamsetty1

@saghul
Copy link
Member

saghul commented Apr 22, 2021

I now remember. In Jitsi Meet we also pass the ideal height, and that triggers 16/9 aspect ratio. This also fixed old webcams without 16/9 aspect ratio support.

@VincentVanlaer
Copy link
Author

That's odd because I get 16/9 with the same camera. Maybe different revisions? We tried a number of things like specifying height and width with mixed results...

I tested it with both firefox and chromium and got the same result, it might be a revision thing then.

Note that you can add that constraint yourself, this is just the default.

I am aware of that, that's what I did to fix it for the specific instance that I manage. However, as I already mentioned in the issue description, not every administrator will probably do this. I think it is a sensible default.

I now remember. In Jitsi Meet we also pass the ideal height, and that triggers 16/9 aspect ratio. This also fixed old webcams without 16/9 aspect ratio support.

I see indeed that there's an ideal height, but the camera I have supports both 16/9 and 4/3 at a height of 720 pixels. So that won't help in this case.

@saghul
Copy link
Member

saghul commented Apr 22, 2021

Sounds like a sensible change (adding the ideal width, that is) to me. WDYT @jallamsetty1? I'd add the width to the constraints by default, which is how we've been running for a while already: https://meet.jit.si/config.js

Would you like to make a PR?

@saghul
Copy link
Member

saghul commented Apr 22, 2021

I see indeed that there's an ideal height, but the camera I have supports both 16/9 and 4/3 at a height of 720 pixels. So that won't help in this case.

Sorry, I meant width.

@jallamsetty1
Copy link
Member

Sounds like a sensible change (adding the ideal width, that is) to me. WDYT @jallamsetty1? I'd add the width to the constraints by default, which is how we've been running for a while already: https://meet.jit.si/config.js

Would you like to make a PR?

We could uncomment the constraints in config.js so that the defaults are set to 1280, 720 always on other deployments ?

@saghul
Copy link
Member

saghul commented Apr 22, 2021

Oh please no. Here is my favorite config file: {}. A whole bunch of nothing.

Ideally things work as expected without any changes in the user part.

If we un comment things in config.js users won't get updates.

Having sensible defaults is a goal we have, that's why the config file is almost entirely comments today.

@jallamsetty1
Copy link
Member

Oh please no. Here is my favorite config file: {}. A whole bunch of nothing.

Ideally things work as expected without any changes in the user part.

If we un comment things in config.js users won't get updates.

Having sensible defaults is a goal we have, that's why the config file is almost entirely comments today.

Well, we do have defaults which sets the ideal height to 720. While changing it to an ideal width may fix this issue where a camera offers 720 with 4:3 and 16:9 aspect ratio, there is no guarantee that the browser will pick 16:9 resolution if the camera supports more than 1 aspect ratio with width as 1280, like a 5:4.

@jallamsetty1
Copy link
Member

@saghul Are you saying we add both by default ? We did have browser bugs where gUM would fail when the camera doesn't support 16:9 even though we are providing ideal constraints and gUM shouldn't fail. I am not against adding it, users will run into these bugs on deployments that don't set the constraints explicitly.

@saghul
Copy link
Member

saghul commented Apr 22, 2021

I mean we'd add height and width (sorry I mixed the words on my earlier post).

IIRC that the most compatible config we know of.

@VincentVanlaer
Copy link
Author

Thanks a lot for fixing it so quickly!

Apak00 pushed a commit to muratovic/lib-config that referenced this issue Aug 26, 2021
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.

3 participants