1453826 - Single dimension screen sharing resolution constraints could behave more reasonably

Status: RESOLVED DUPLICATE of bug 1634044

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1453826 - P1 - screen share scale aspect ratio

Review commit: https://reviewboard.mozilla.org/r/236358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236358/

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1453826 - P2 - screen share scale aspect ratio mochitest

Review commit: https://reviewboard.mozilla.org/r/236360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236360/

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

*That is bug 1449832, and bug 1453269 respectively.

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 6

[Andreas Pehrson [:pehrsons]]

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.

Comment 7

[Andreas Pehrson [:pehrsons]]

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.