Closed
Bug 1453826
Opened 7 years ago
Closed 4 years ago
Single dimension screen sharing resolution constraints could behave more reasonably
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
DUPLICATE
of bug 1634044
People
(Reporter: ng, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Applying a constraint to a single dimension yields a result which can be at least twice as large as requested, and does not necessarily honor the original aspect ratio.
Applying screen sharing constraints should produce results that strictly obey the source aspect ratio, and if possible be no larger than the provided constraint.
Using demo https://codepen.io/brianpeiris/pen/NYydvb?editors=0010 , with a native resolution of 3840 x 2160:
Requesting a width of 720 yields a resulting image of size 2560 x 1440. Requesting a width of 719 yields 2560x1339.
Reporter | ||
Updated•7 years ago
|
Summary: Screen sharing resolution constraints could behave more reasonably → Single dimension screen sharing resolution constraints could behave more reasonably
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8967637 -
Flags: review?(apehrson)
Attachment #8967638 -
Flags: review?(apehrson)
Reporter | ||
Comment 3•7 years ago
|
||
I have added the patches I prepared for 1449832, though I am not sure if it is desirable to use them or just roll any of the work into 1453269. I think at least the mochitest should be useful.
Reporter | ||
Comment 4•7 years ago
|
||
*That is bug 1449832, and bug 1453269 respectively.
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8967637 [details]
Bug 1453826 - P1 - screen share scale aspect ratio
https://reviewboard.mozilla.org/r/236358/#review242130
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:496
(Diff revision 1)
> + if ((aConstraintHeight && aConstraintWidth) ||
> + (!aConstraintWidth && !aConstraintHeight) ||
> + !aProps.height() || !aProps.width()) {
> + return;
> + }
> + int32_t gcd = EuclidGCD(aProps.width(), aProps.height());
The common screen dimensions are likely to share a power of 2 factor. That factor can be trivially removed before computing the gcd, which can then be multiplied by that factor. Window and application shares are not as likely to have a sizable common power of 2 factor.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:496
(Diff revision 1)
> + if ((aConstraintHeight && aConstraintWidth) ||
> + (!aConstraintWidth && !aConstraintHeight) ||
> + !aProps.height() || !aProps.width()) {
> + return;
> + }
> + int32_t gcd = EuclidGCD(aProps.width(), aProps.height());
Once the constraints hack is removed the scaled dimension can be stored and only needs to be recalculated if the contraint dimensions change or the dimensions of the received frame changes.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:537
(Diff revision 1)
> req_ideal_width = (mCapability.width >> 16) & 0xffff;
> req_ideal_height = (mCapability.height >> 16) & 0xffff;
> }
> -
> + // See bug Bug 1453269
> + switch (mMediaSource) {
> + case MediaSourceEnum::Screen:
MediaSourceEnum::Browser still exists but isn't in use. I have opted not to include it because it is not used elsewhere in this file.
Updated•7 years ago
|
Blocks: Screensharing
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8967637 [details]
Bug 1453826 - P1 - screen share scale aspect ratio
https://reviewboard.mozilla.org/r/236358/#review242220
I had a bunch of nits written down before I ended up testing the algorithm and decided to r- for breaking max constraints and for weird scaling results while resizing a captured window. Pleas focus on the algorithm comment before addressing the nits.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:489
(Diff revision 1)
> +// Translate screen share aspect ratio to constraint aspect ratio if only a
> +// single dimension constraint is supplied.
> +inline void
> +preserveScreenShareAspectRatio(int32_t& aConstraintWidth,
> + int32_t& aConstraintHeight,
> + const camera::VideoFrameProperties& aProps)
Instead of passing in all props, could you pass in a `aWidth` and a `aHeight` variable?
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:491
(Diff revision 1)
> + if ((aConstraintHeight && aConstraintWidth) ||
> + (!aConstraintWidth && !aConstraintHeight) ||
> + !aProps.height() || !aProps.width()) {
> + return;
> + }
Could you split these into separate guards, each with a comment inside the block on which case they cover and why it's fine to return early then?
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:491
(Diff revision 1)
> +inline void
> +preserveScreenShareAspectRatio(int32_t& aConstraintWidth,
> + int32_t& aConstraintHeight,
> + const camera::VideoFrameProperties& aProps)
> +{
> + if ((aConstraintHeight && aConstraintWidth) ||
Since aConstraintHeight and aConstraintWidth are ints I prefer to be explicit about the zero checks.
Alas `if (aConstraintHeight > 0 && aConstraintWidth > 0) {`
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:492
(Diff revision 1)
> +preserveScreenShareAspectRatio(int32_t& aConstraintWidth,
> + int32_t& aConstraintHeight,
> + const camera::VideoFrameProperties& aProps)
> +{
> + if ((aConstraintHeight && aConstraintWidth) ||
> + (!aConstraintWidth && !aConstraintHeight) ||
Same here, `if (aConstraintWidth == 0 && aConstraintHeight == 0) {`.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:493
(Diff revision 1)
> + int32_t& aConstraintHeight,
> + const camera::VideoFrameProperties& aProps)
> +{
> + if ((aConstraintHeight && aConstraintWidth) ||
> + (!aConstraintWidth && !aConstraintHeight) ||
> + !aProps.height() || !aProps.width()) {
Same here, `aProps.height() == 0 || aProps.width() == 0) {`
On splitting the conditions out you can keep this one together.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:497
(Diff revision 1)
> + int32_t minWidth = aProps.width() / gcd;
> + int32_t minHeight = aProps.height() / gcd;
Would widthUnit/heightUnit be better names? I was a bit confused until I stepped through, and you use "unit" in the comments below.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:500
(Diff revision 1)
> + if (aConstraintHeight) {
> + // Clamp height to multiple of the unit height
> + aConstraintHeight -= (aConstraintHeight % minHeight);
> + // Apply aspect ratio in unit widths
> + aConstraintWidth = minWidth * aConstraintHeight / minHeight;
> + } else {
> + // Clamp width to multiple of the unit width
> + aConstraintWidth -= (aConstraintWidth % minWidth);
> + // Apply aspect ratio in unit heights
> + aConstraintHeight = minHeight * aConstraintWidth / minWidth;
> + }
You could write this code only once by doing something like this just after the top guards and then operating on that.
```
int32_t& requestedDimension;
int32_t& blankDimension;
if (aConstraintHeight > 0) {
requestedDimension = aConstraintHeight;
blankDimension = aConstraingWidth;
} else {
requestedDimension = aConstraintWidth;
blankDimension = aConstraintHeight;
}
int32_t gcd ...
```
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:501
(Diff revision 1)
> + // Clamp height to multiple of the unit height
> + aConstraintHeight -= (aConstraintHeight % minHeight);
> + // Apply aspect ratio in unit widths
> + aConstraintWidth = minWidth * aConstraintHeight / minHeight;
Sooo, consider a window of resolution 853x709 and a max height constraint of 708.
gcd = 1
minWidth = 853
minHeight = 709
aConstraintHeight = 708 - (708 % 709) = 0
aConstraintWidth = 853 * 0 / 709 = 0
This is caught later by:
aConstraintHeight = 709
aConstraintWidth = 853
We'd break the spec here.
If you imagine someone dynamically scaling a window with their mouse, the resolution we end up scaling to can get really weird depending on the GCD.
There's not really any need to maintain an exact aspect ratio here either. I think a soft scaling and a slight crop might be the way to go.
This is the reason for the r-.
::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:540
(Diff revision 1)
> + // Preserve aspect ratio for screen sharing if only a single dimension
> + // is supplied.
> + preserveScreenShareAspectRatio(req_max_width, req_max_height, aProps);
> + preserveScreenShareAspectRatio(req_ideal_width, req_ideal_height, aProps);
Make sure to back out the code from bug 1449832 too.
Attachment #8967637 -
Flags: review?(apehrson) → review-
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8967638 [details]
Bug 1453826 - P2 - screen share scale aspect ratio mochitest
https://reviewboard.mozilla.org/r/236360/#review242536
Looks good. Please add a test to each test case for scaling through applyConstraints too. Clearing r? until we have this.
Attachment #8967638 -
Flags: review?(apehrson)
Updated•7 years ago
|
Rank: 15
Priority: -- → P2
Updated•4 years ago
|
Updated•4 years ago
|
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•