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

ResizeMode (crop-and-scale) is underspecified #584

Open
henbos opened this issue May 2, 2019 · 11 comments · Fixed by #623

Comments

@henbos
Copy link
Contributor

commented May 2, 2019

When downscaling a resolution, there are at least three ways you could do it if the new aspect ratio is not the same as the original one:

  • Stretch it.
  • Don't stretch it, achieve new resolution by "zooming in" to the middle of it.
  • Don't stretch it, achieve new resolution by making the picture smaller and adding black bars to fill out the part of the rectangle that doesn't have the picture.

The spec downscaling and/or cropping but does not say how this is achieved.

@guidou what does Chrome do?

@guidou

This comment has been minimized.

Copy link

commented May 3, 2019

Chrome uses the largest centered rectangle with the same aspect ratio of the original frame that fits entirely inside of the target resolution.
So, basically, first downscale the original (as little as possible) while maintaining the aspect ratio until the target width or height is matched, and then crop to match the other dimension if necessary.

@henbos

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Great, I think that was what @jan-ivar was hoping for, yes?

@eric-carlson

This comment has been minimized.

Copy link

commented May 3, 2019

WebKit doesn't trim, it uses letterbox scaling: the video's clean aperture rectangle is scaled to fit completely in the destination size. If the destination pixel aspect ratio is different, the remainder of the destination is filled with black.

@jan-ivar

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@henbos Yes, that's what I expected, and complies with my "more modes" reading of the spec:

"The user agent MAY use cropping and downscaling to offer more resolution choices than this camera naturally produces. The reported sequence MUST list all the means the UA may employ to derive resolution choices for this camera."

In other words, the purpose is to simulate camera modes. Since I've never seen a camera naturally emit a track containing black bars, I would not expect that output.

I was even going to say that I thought black bars go against the "don't invent pixels" rule we have, but then I couldn't find such a rule. Turns out we only have it in WebRTC: "The media MUST NOT be upscaled to create fake data that did not occur in the input source".

An oversight? I kinda think that's implied. Do we want to add language to make this clearer?

@jan-ivar

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@eric-carlson Thanks for the info. The WebKit vs Chrome behavior here seems a bit of a web compat issue, which probably speaks to that the spec could be clearer.

@alvestrand

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

In discussion in the WG, my memory says that we did not want to permit distortion, so stretching was right out. Crop-and-scale was intended to be "crop to desired aspect ratio, then scale to desired # of pixels".
We also decided not to do black bars.
For reference, the CSS object-fit property is described here: https://www.w3schools.com/css/css3_object-fit.asp - we decided to not to "fill" or "contain".

@aboba aboba added the TPAC 2019 label Aug 21, 2019
@alvestrand

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

crop-and-scale was supposed to do the same as CSS "object-fit: cover", except that we don't want to upscale ever.

@henbos

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

I'l just make it say "The media MUST NOT be upscaled to create fake data that did not occur in the input source".

@guest271314

This comment has been minimized.

Copy link

commented Sep 13, 2019

Does this only affect MediaStreamTrack from getUserMedia()?

Chromium does provide resizeMode constraint when the track is derived from HTMLVideoElement.captureStream() though AFAICT no value that is set has any impact on the resulting video track.

Further, Chromium provides getSettings() for tracks derived from HTMLVideoElement.captureStream() which ultimately outputs incorrect results for width and height when the underlying file is a WebM file having variable resolution frames output by MediaRecorder then replayed at HTML <video> element at Chromium, only the initial width and height are output. For example, given a 2 second WebM video where the first 1 second is 400x300 and the second second is 300x150 getSettings() width and height output 400, 300 for the duration of the video.

@henbos

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Sometimes it makes sense to have the same constraints for tracks sourced by other means than GUM, and sometimes it does not. It has not been clear in the past. Going forward: Each spec needs to explicitly say which constraints are applicable to its tracks. I.e. captureStream() has to explicitly say that it supports crop-and-scale if that is something it does.

Chromium assumed that constraints applied no matter the source as long as they "made sense", but different browsers may have made different assumptions.

I believe chromium implements reading width and height by looking at frames, and reporting what it has seen recently, but I don't know the details. The spec should be clear what to do here, not surprised if chromium has a bug here.

@henbos henbos reopened this Sep 26, 2019
@henbos

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

Reopen to update the sentence according to TPAC conclusion:

Issue 584
Safari has potential issues with downscale restriction in multiple apps accessing to camera.
RESOLUTION: Remove upscale from the proposed sentence and merge

@henbos henbos added Ready for PR and removed PR exists labels Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.