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.

Sign up for GitHub

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

fix: low-resolution screen sharing for safari 17. #387

Merged
merged 2 commits into from
Oct 18, 2023
Merged

fix: low-resolution screen sharing for safari 17. #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
165c942
fix: low-resolution screen sharing for safari 17.
cloudwebrtc Oct 17, 2023
5345f3b
update.
cloudwebrtc Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/src/support/platform.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ bool lkPlatformIsTest() => Platform.environment.containsKey('FLUTTER_TEST');

BrowserType lkBrowser() => lkBrowserImplementation();

BrowserVersion lkBrowserVersion() => lkBrowserVersionImplementation();

enum PlatformType {
web,
windows,
Expand All @@ -54,3 +56,16 @@ enum BrowserType {
wkWebView,
unknown,
}

class BrowserVersion {
const BrowserVersion(this.major, this.minor, this.patch);

/// The major version number: "1" in "1.2.3".
final int major;

/// The minor version number: "2" in "1.2.3".
final int minor;

/// The patch version number: "3" in "1.2.3".
final int patch;
}
4 changes: 4 additions & 0 deletions lib/src/support/platform/io.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,7 @@ bool lkE2EESupportedImplementation() {
BrowserType lkBrowserImplementation() {
return BrowserType.unknown;
}

BrowserVersion lkBrowserVersionImplementation() {
return const BrowserVersion(0, 0, 0);
}
3 changes: 3 additions & 0 deletions lib/src/support/platform/web.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,6 @@ BrowserType lkBrowserImplementation() {
if (browser.isWKWebView) return BrowserType.wkWebView;
return BrowserType.unknown;
}

BrowserVersion lkBrowserVersionImplementation() => BrowserVersion(
browser.version.major, browser.version.minor, browser.version.patch);
7 changes: 7 additions & 0 deletions lib/src/track/local/local.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,13 @@ abstract class LocalTrack extends Track {
if (options.selfBrowserSurface != null) {
constraints['selfBrowserSurface'] = options.selfBrowserSurface!;
}

// Remove resolution settings to fix low-resolution screen share on Safari 17.
// related bug: https://bugs.webkit.org/show_bug.cgi?id=263015
if (lkBrowser() == BrowserType.safari &&
lkBrowserVersion().major == 17) {
Copy link
Contributor

Choose a reason for hiding this comment

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

good thinking to specifically fix it for this Safari version, too.
I think we should do something similar on the JS side.
The bugreport in webkit only talks about width: { max: constraints. maybe width: { ideal: would be possible as a workaround for Safari 17.
I remember it not being a valid workaround in general because ideal was causing issues in other Safari versions :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just tested using width: { ideal:, doesn't seem to fix it, I still get the low resolution screen capture :(

        if (lkBrowser() == BrowserType.safari &&
            lkBrowserVersion().major == 17) {
          constraints['video'] = {
            'width': {'ideal': options.params.dimensions.width},
            'height': {'ideal': options.params.dimensions.height},
          };
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for checking. then your fix seems like the best option :)

constraints['video'] = true;
}
}
stream = await rtc.navigator.mediaDevices.getDisplayMedia(constraints);
} else {
Expand Down