Skip to content

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Feb 17, 2023

๐Ÿ–ผ๏ธ Screenshots

๐Ÿš๏ธ Before ๐Ÿก After
image image

As mentioned in corellated issue #8815 :

... Even if the camera supports a higher resolution in calls the resolution is requested to be 720x540. This is a default value chosen for performance reasons, both for CPU load when encoding the video and for network bandwidth when sending it.

Maximum capable video resolution is fixed at 4:3. This PR adjusts width and height of local video frame in stripe to fit in desired resolution and avoid its cropping

๐Ÿšง TODO

  • Remove video width lock at 300px
  • Set aspect-ratio to mentioned resolution at 4:3
  • Code review

๐Ÿ Checklist

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I have this view on my local test. Is it was was expected (it is not cropped)? Shouldn't it be stretch so the video would have rounded corners?

image

@ShGKme
Copy link
Contributor

ShGKme commented Feb 20, 2023

Hmm... Now it looks ok.

image

@ShGKme
Copy link
Contributor

ShGKme commented Feb 20, 2023

Steps to reproduce:

  1. Make a call from account 1 in a group chat
  2. Join a call with account 2 with an enabled camera
    2
  3. By default, account 1 will be shown. The Video is cropped.
    image
  4. Leave and re-join. The video is not cropped, but also not rounded:
    image
  5. Switch to the grid view and back to the speaker view. Everything is ok.
    image

@ShGKme
Copy link
Contributor

ShGKme commented Feb 20, 2023

I may admit that this may be a peculiarity of my environment... Does it work fine on yours?

@Antreesy Antreesy force-pushed the fix/noid/consider-aspect-ratio-for-local-video branch from fb537c0 to a62c711 Compare February 21, 2023 18:54
@nickvergessen nickvergessen removed their request for review February 22, 2023 06:05
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

The code looks fine, no cropping now. The aspect ratio is 1.33333 fallback until reconnecting and reopening the local video on my test.

I'm not sure if we want to merge it without solving #8815.

change

@Antreesy
Copy link
Contributor Author

Antreesy commented Feb 22, 2023

Is it what is expected in the PR?

Yes, because videostream comes with aspect-ratio: 1.33, and previous styles cutted it even more, up to 1.24.
I want to fix at least this for now, and use as a basement in following up fixes on mentioned issue.
Later we could pass a defined aspect-ratio from source with constraints, and use it as SSOT

@Antreesy Antreesy force-pushed the fix/noid/consider-aspect-ratio-for-local-video branch from a62c711 to 5710ece Compare February 23, 2023 15:16
@Antreesy Antreesy changed the title Remove local video cropping in Stripe Set local video in Stripe to 4:3 aspect-ratio Feb 23, 2023
@Antreesy Antreesy requested a review from ShGKme February 23, 2023 15:36
@Antreesy Antreesy merged commit 3c5449f into master Feb 24, 2023
@Antreesy Antreesy deleted the fix/noid/consider-aspect-ratio-for-local-video branch February 24, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants