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

feat: Screen Share improvement for Group Calls [WPB-10134] #18445

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

EnricoSchw
Copy link
Contributor

@EnricoSchw EnricoSchw commented Dec 6, 2024

StoryWPB-10134 [AVS] Change Screen Share constrains for better Screen Share quality in Group Call

Screen Share improvement for Group Calls

This is an improvement to the readability of screen sharing:

  • I have removed the downscaling on the sender side.
  • I mark screen share videos as "All Content Detailed."
  • If someone receives a 4K screen share on their 1080p monitor, it will naturally be downscaled on the receiver's side. For this case, I have added a zoom feature.
  • We now send screen shares at 5 fps by default.

Screenshots/Screencast (for UI changes)

Bildschirmfoto 2024-12-06 um 17 48 37

@WolfiWire

I think you'll have an opinion on this, but I'd like to adjust the zoom feature in a fallow up PR. I hope this is ok
(These PRs always take so long :) )

Checklist

  • [x ] mentions the JIRA issue in the PR name (Ex. [WPB-XXXX])
  • PR has been self reviewed by the author;
  • Hard-to-understand areas of the code have been commented;
  • If it is a core feature, unit tests have been added;

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 46.31%. Comparing base (4dac7ea) to head (15d4a49).
Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #18445      +/-   ##
==========================================
- Coverage   46.33%   46.31%   -0.03%     
==========================================
  Files         846      846              
  Lines       26399    26408       +9     
  Branches     5986     5989       +3     
==========================================
- Hits        12233    12231       -2     
- Misses      12634    12643       +9     
- Partials     1532     1534       +2     

Comment on lines 111 to 118
const handleZoomClick = () => {
if (isZoomedIn) {
setIsZoomedIn(false);
} else {
setIsZoomedIn(true);
}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const handleZoomClick = () => {
if (isZoomedIn) {
setIsZoomedIn(false);
} else {
setIsZoomedIn(true);
}
};
const handleZoomClick = () => {
setIsZoomedIn(prev => !prev);

Comment on lines 126 to 127
onClick={handleZoomClick}
onKeyDown={handleZoomClick}
Copy link
Contributor

Choose a reason for hiding this comment

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

two are unnecessarily, only onClick should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You use both events in many places... I thought it was a workaround. I will remove it

this.isConferenceCallingEnabled = ko.pureComputed(
() => this.teamFeatures()?.conferenceCalling?.status === FeatureStatus.ENABLED,
);
this.isConferenceCallingEnabled = ko.pureComputed(() => true);
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's gonna be always true I suggest a bigger refactor to just get rid of this property every where, but for now we could at least set it to an always true observable maybe

Suggested change
this.isConferenceCallingEnabled = ko.pureComputed(() => true);
this.isConferenceCallingEnabled = ko.observable(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

O good finding.. ❤️ Thx. this is a hack to activate group call.. This should never go to staging. I will remove this!

@EnricoSchw EnricoSchw merged commit d469b8f into dev Dec 10, 2024
13 checks passed
@EnricoSchw EnricoSchw deleted the feat/WPB-10134-screenshare branch December 10, 2024 13:12
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