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

Section 5.2: centering, scaling, cropping #1283

Closed
aboba opened this issue May 31, 2017 · 19 comments
Closed

Section 5.2: centering, scaling, cropping #1283

aboba opened this issue May 31, 2017 · 19 comments

Comments

@aboba
Copy link
Contributor

@aboba aboba commented May 31, 2017

From EKR: https://lists.w3.org/Archives/Public/public-webrtc/2017May/0166.html

The advice here (center, scale, then crop) differs from a MUST in JSEP S 3.6.

If the original resolution exceeds the size limits in the attribute,
the sender SHOULD apply downscaling to the output of the
MediaStreamTrack in order to satisfy the limits. Downscaling MUST
NOT change the track aspect ratio.

@fluffy
Copy link
Contributor

@fluffy fluffy commented Aug 23, 2017

Related to rtcweb-wg/jsep#828

@stefhak
Copy link
Contributor

@stefhak stefhak commented Aug 29, 2017

FWIW, in the minutes from TPAC/Lisbon (search for 'crop' on that page) it says we decided on 'center, scale, crop'. That is also con you get to the discussion (and there is also a discussion in the end of #305 that seems to confirm that decision).

Slide 73 in https://www.w3.org/2011/04/webrtc/wiki/images/1/1e/WebRTCWG-2016-09-22.pdf
is the one shown when we discussed at TPAC (if memory serves).

@ekr
Copy link
Contributor

@ekr ekr commented Aug 29, 2017

This algorithm seems bad for a pretty obvious reason: it destroys information and there's no particular reason to think that the center of the image is where the action is.

@juberti
Copy link
Contributor

@juberti juberti commented Aug 30, 2017

Right, e.g. screenshares.

As mentioned in rtcweb-wg/jsep#828, I think this text made sense at an earlier point in time, but PeerConnection no longer has any API surface through which cropping can be indicated. So I think this text should probably disappear.

stefhak added a commit to stefhak/webrtc-pc that referenced this issue Sep 4, 2017
@stefhak stefhak added the PR exists label Sep 4, 2017
@aboba
Copy link
Contributor Author

@aboba aboba commented Sep 6, 2017

@juberti PR #1570 replaces the text with a reference to JSEP.

@fluffy
Copy link
Contributor

@fluffy fluffy commented Sep 7, 2017

At the next meeting, if we decide to go with what JSEP says, then this PR looks good to reflect that into the spect.

stefhak added a commit to stefhak/webrtc-pc that referenced this issue Oct 2, 2017
@juberti
Copy link
Contributor

@juberti juberti commented Oct 13, 2017

I think we reached consensus for this issue, and looks like #1570 addresses it.

Are we ready to close this issue?

@stefhak
Copy link
Contributor

@stefhak stefhak commented Oct 13, 2017

When we discussed this at yesterday's interim, it was clear that everyone was not on board, there was not consensus to merge #1570 and we can't close this issue just yet.
To record the direction agreed on yesterday:

  • we should still keep some text in webrtc-pc on what to do when the video has to be resized for transmission
  • the text must clarify what is meant by words used (now e.g. "desired" is used but never defined)
  • we will also add a know on RTCRtpSender where the developer can choose between alternatives "fill", "contain", "auto" where fill equals "center,scale,crop", "contain" means pad, "auto" means auto (whatever that is)
    @fluffy to help drafting the PR
@alvestrand
Copy link
Contributor

@alvestrand alvestrand commented Oct 16, 2017

I have changed my opinion on this.
We should make the spec simpler, not more complex.
The simplest answer, when faced with a situation where you can't rescale the video to fit the constraints, is what JSEP currently says: "NO".
No letterboxing, cropping or pillaring. Either you can scale the video to fit the constraints, or sending the video fails.

@stefhak
Copy link
Contributor

@stefhak stefhak commented Oct 16, 2017

@alvestrand the 'constraints' you talk about are the ones imposed by the encoder and decoder (via imageattr, right? (We also have track constraints so I'm a bit confused).
Personally I also like JSEP's way of speccing this, but with my chair hat on all I want now is something that gets consensus.

alvestrand added a commit that referenced this issue Oct 16, 2017
Fixes #1283.
@alvestrand
Copy link
Contributor

@alvestrand alvestrand commented Oct 16, 2017

@stefhak Yes. I looked at #1570 and think the requirement can be made even simpler - I cooked up #1636 to show what I mean.
To reiterate: On the call, I thought the complex proposal being floated there (parts of it by me) made sense. I've changed my mind, so I'm no longer part of the apparent consensus on Thursday.

@stefhak
Copy link
Contributor

@stefhak stefhak commented Oct 16, 2017

@alvestrand I like #1636 - clearer than #1570.
BTW, do you read the spec in the same way as I do, that scaling down for transmission is allowed regardless of (mandatory) track constraints?
Say you have a track that is set (mandatory constraints) to exactly 1280x720, and it is possible to transmit anything between 1x1 and 640x480, then it would be OK to scale to 640x360 and transmit?
And if the remote application did getSettings() on the received track it would say 640x360?

@fluffy
Copy link
Contributor

@fluffy fluffy commented Oct 16, 2017

@alvestrand
Copy link
Contributor

@alvestrand alvestrand commented Oct 17, 2017

@fluffy no, I'm not OK with aspect ratio changes. You agreed in JSEP that aspect ratios should not change; we can't be OK one place and not OK in the other.
Besides, aspect ratio change loses information. If distortion's required, let the recipient do it.
@stefhak I see the constraints as controlling what the track delivers (defined as "track resolution" in JSEP). That's input to the transmission function, and the transmission function is permitted to scale down what it gets, as per JSEP.
Calling getSettings() on the recipient's track will return what came across. That's going to be 640x360 in the case you posit. I don't see any sense in returning any other number.

@alvestrand
Copy link
Contributor

@alvestrand alvestrand commented Oct 17, 2017

Note: I see no reason not to propose the control and the relaxed MUST NOT requirement as an extension to the spec after CR. If users think it useful and implementors want to implement it, it can be added.
But I think the spec at CR is better off with writing cropping, letterboxing and aspect ratio change out of it.

@stefhak
Copy link
Contributor

@stefhak stefhak commented Oct 17, 2017

@alvestrand I'm in agreement with the second part of #1283 (comment) (the one addressed to me).

@alvestrand
Copy link
Contributor

@alvestrand alvestrand commented Oct 17, 2017

@stefhak wrt #1283 (comment) - my interpretation of the constraints text in MediaCapture is that a platform is allowed, but not required, to let getUserMedia() return a scaled and/or cropped version of what the physical camera produces. The spec talks about a range of possible configurations - platforms that do scaling & cropping offer a very large number of configurations; platforms that don't, don't. That's certainly how Chrome's code works (it does scale to fit with specified constraints, and opens the device in a mode that will allow it to satisfy the constraints by croping and/or scaling).
Hope that makes sense....

dontcallmedom added a commit that referenced this issue Oct 17, 2017
@stefhak
Copy link
Contributor

@stefhak stefhak commented Oct 18, 2017

In response to #1283 (comment): that is sort of what I expected, but that, to me, also indicates that this issue requires more implementation and use experience before resolving. As an example, say dimensions up to X by Y can be transmitted, and webrtc-pc forbids cropping to match imageattr. We could then have the situation that applying height and width constraints to the track to limit it to X by Y gives another result (cropped) than just feeding the unconstrained track to the sender.

@stefhak
Copy link
Contributor

@stefhak stefhak commented Oct 23, 2017

I've opened w3c/mediacapture-main#495 on mediacapture-main, it is related to this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants