Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Description • 8 years ago

Parameters set with setParameters currently only take if set before negotiation happens. It should work to change settings mid-call as well, without renegotiation.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 1 • 8 years ago

Paul, is this something you guys need asap?

	Paul E. Jones
	Comment 2 • 8 years ago

Let me ask.  Either I'll come back with an answer or someone from the endpoint team will weigh in.

	Randell Jesup [:jesup] (needinfo me)
	Comment 3 • 8 years ago

tentatively assigning priority

	Paul E. Jones
	Comment 4 • 8 years ago

Mid-call, this is not so important to us.  It's only important that it works reliably when initially setting up the call.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 6 • 7 years ago

I imagine I fixed this in bug 1320101, but it might have become broken again by the webrtc.org 49 merge, I haven't had time to test. We could probably add test code in the simulcast test.

	Philipp Hancke [:fippo]
	Comment 7 • 7 years ago

i have quite a use-case for this. So for the mesh version of appear.in we scale down the maximum bandwidth depending on the number of participants in the room. This currently works with an SDP O/A cycle (local, no signaling) and mangling the TIAS in the remote description.

It looks like this would be a much better way to do it, avoiding one pointless renegotiation cycle.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 8 • 7 years ago

Here's a fiddle I've been using to test this manually: https://jsfiddle.net/pehrsons/7L9k7zLq/

	Byron Campen [:bwc]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8872667 [details]
Bug 1253499 - Forward maxBitrate and scaleResolutionDownBy straight to the VideoConduit.

https://reviewboard.mozilla.org/r/144216/#review147922

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2711
(Diff revision 1)
> +              constraint.constraints.maxBr;
> +            encoding.constraints.scaleDownBy =
> +              constraint.constraints.scaleDownBy;
> +          }
> +        }
> +        video->ConfigureSendMediaCodec(config);

Is this safe to do when the conduit is transmitting?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8872667 [details]
Bug 1253499 - Forward maxBitrate and scaleResolutionDownBy straight to the VideoConduit.

https://reviewboard.mozilla.org/r/144216/#review147922

> Is this safe to do when the conduit is transmitting?

It is! There's a mutex between doing this on main thread and new frames coming in on other threads.

	Randell Jesup [:jesup] (needinfo me)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8872664 [details]
Bug 1253499 - Add a getter for VideoConduit's current send config.

https://reviewboard.mozilla.org/r/144210/#review148286

	Randell Jesup [:jesup] (needinfo me)
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8872665 [details]
Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send codec.

https://reviewboard.mozilla.org/r/144212/#review148288

r+ if there's a good answer to my comment, or if Byron's comment in the code is no longer valid (in which case, remove it)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:579
(Diff revision 1)
>      // Stream dimensions must be divisable by 2^(n-1), where n is the number of layers.
>      // Each lower resolution layer is 1/2^(n-1) of the size of largest layer,
>      // where n is the number of the layer
>  
> -    // width/height will be overridden on the first frame; they must be 'sane' for
> -    // SetSendCodec()
> -    video_stream.width = width >> idx;
> -    video_stream.height = height >> idx;
> -    video_stream.max_framerate = mSendingFramerate;
>      auto& simulcastEncoding = codecConfig->mSimulcastEncodings[idx];
> +    video_stream.width = width / simulcastEncoding.constraints.scaleDownBy;
> +    video_stream.height = height / simulcastEncoding.constraints.scaleDownBy;
> +    video_stream.max_framerate = mSendingFramerate;

So how does this change to actually use the ScaleDownBy value for each layer deal with Byron's comment above?

	Randell Jesup [:jesup] (needinfo me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8872666 [details]
Bug 1253499 - Ensure the target bitrate never exceeds the max bitrate.

https://reviewboard.mozilla.org/r/144214/#review148290

	Andreas Pehrson [:pehrsons] Assignee
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8872665 [details]
Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send codec.

https://reviewboard.mozilla.org/r/144212/#review148288

> So how does this change to actually use the ScaleDownBy value for each layer deal with Byron's comment above?

I did a simple test and we crash after this check fails: http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc#62

It doesn't align with the comment however, since if the width is odd and the height even, dividing both by half will screw up the aspect ratio. This seems like a legacy bug we have with simulcast, though this patch provides some knobs to the application for triggering it.

Suggestions on how to fix?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 19 • 7 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #18)
> Suggestions on how to fix?

	Byron Campen [:bwc]
	Comment 20 • 7 years ago

(In reply to Andreas Pehrson [:pehrsons] from comment #19)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #18)
> > Suggestions on how to fix?

So there's this function:

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#1499

You would need to apply the scale, and then feed the resulting resolution into that function so it can be adjusted (maybe! there are no guarantees if the starting resolution width and height don't have a large enough common factor) to keep webrtc.org happy.

(Wow. Such broken. Much sad.)

	Randell Jesup [:jesup] (needinfo me)
	Comment 21 • 7 years ago

The right way to fix it is to modify webrtc.org to support non-^2 size steps.

In the meantime, we keep the ^2 bits, and yes that puts limits on the initial size; we could normalize to usable source resolutuions (which normally isn't a huge issue if the source is a camera), or kick out and stop adding layers when a division would create a bad simulcast layer.  

Note also this in upstream (simulcast.cc):  (3rd number is max_layers)

[08:41]	jesup	 // These tables describe from which resolution we can use how many
[08:41]	jesup	 // simulcast layers at what bitrates (maximum, target, and minimum).
[08:41]	jesup	 // Important!! Keep this table from high resolution to low resolution.
[08:41]	jesup	const SimulcastFormat kSimulcastFormats[] = {
[08:41]	jesup	  {1920, 1080, 3, 5000, 4000, 800},
[08:41]	jesup	  {1280, 720, 3,  2500, 2500, 600},
[08:41]	jesup	  {960, 540, 3, 900, 900, 450},
[08:41]	jesup	  {640, 360, 2, 700, 500, 150},
[08:41]	jesup	  {480, 270, 2, 450, 350, 150},
[08:41]	jesup	  {320, 180, 1, 200, 150, 30},
[08:41]	jesup	  {0, 0, 1, 200, 150, 30}};

	Andreas Pehrson [:pehrsons] Assignee
	Comment 26 • 7 years ago

Comment on attachment 8872665 [details]
Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send codec.

Jesup, can you take a look again?

This will now adjust the resolution if there is only one layer. For multiple simulcast layers, when the encoder's constraints apply, we stick to the old logic. Should there be an aspect ratio mismatch for a layer we'll drop it along with all lower layers and log a warning.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8875774 [details]
Bug 1253499 - Add mochitest for live-updating scaleResolutionDownBy.

https://reviewboard.mozilla.org/r/147192/#review151484

lgtm with nits.

::: dom/media/tests/mochitest/test_peerConnection_setParameters_scaleResolutionDownBy.html:18
(Diff revision 1)
> +
> +  let sender, localElem, remoteElem;
> +  let originalWidth, originalAspectRatio, originalScale;
> +
> +  async function checkScaleDownBy(scale) {
> +    sender.setParameters({ encodings: [{ scaleResolutionDownBy: scale }] });

await, or we'll miss any errors.

::: dom/media/tests/mochitest/test_peerConnection_setParameters_scaleResolutionDownBy.html:35
(Diff revision 1)
> +        is(test.pcLocal._pc.getSenders().length, 1,
> +            "Should have 1 local sender");
> +        is(test.pcLocal.localMediaElements.length, 1,
> +            "Should have 1 local sending media element");
> +        is(test.pcRemote.remoteMediaElements.length, 1,
> +            "Should have 1 remote media element");
> +
> +        sender = test.pcLocal._pc.getSenders()[0];
> +        if (!sender) {
> +          return Promise.reject(new Error("No sender"));
> +        }
> +
> +        localElem = test.pcLocal.localMediaElements[0];
> +        if (!localElem) {
> +          return Promise.reject(new Error("No local element"));
> +        }
> +
> +        remoteElem = test.pcRemote.remoteMediaElements[0];
> +        if (!remoteElem) {
> +          return Promise.reject(new Error("No remote element"));
> +        }
> +
> +        originalWidth = localElem.videoWidth;

We already get a ReferenceError here should localElem ever be undefined, so the three explicit tests above seem redundant. 

Establishing the preconditions with the is() statements seems sufficient.

If you decide to keep it, then style-wise I I'd prefer throw over Promise.reject, with the enclosing function (CHECK_PRECONDITIONS) an async function.

	Randell Jesup [:jesup] (needinfo me)
	Comment 28 • 7 years ago

Comment on attachment 8872665 [details]
Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send codec.

mozreviews shows this as r+... not sure why not here

	Andreas Pehrson [:pehrsons] Assignee
	Comment 29 • 7 years ago

(In reply to Randell Jesup [:jesup] from comment #28)
> Comment on attachment 8872665 [details]
> Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send
> codec.
> 
> mozreviews shows this as r+... not sure why not here

I changed to r? in bugzilla as a means of requesting a re-review. See comment 26. Would still be great if you had a look at the interdiff.

	Randell Jesup [:jesup] (needinfo me)
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8872665 [details]
Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send codec.

https://reviewboard.mozilla.org/r/144212/#review157914

	Bulk Bug Changes for mreavy's org
	Comment 31 • 7 years ago

Mass change P1->P2 to align with new Mozilla triage process

	Andreas Pehrson [:pehrsons] Assignee
	Comment 32 • 6 years ago

A failure here typically indicates a test error, so it's useful for debugging.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 41 • 6 years ago

webrtc.org is picky about resolutions for simulcasst layers.
As of current it will assert that all layers have identical aspect ratio.
We handle this by ignoring layers where the aspect ratio is not the same as
the highest layer's.

The new algorithm will, when simulcast is requested and at least one layer
is scaled to something other than 1.0, try to remedy this by:
- The highest resolution layer is cropped to 16-pixel alignment, to ensure
  that scaling options exist.
- A separate VideoAdapter is used for simulcast layers, with the highest
  layer's resolution as an aspect ratio requirement. This forces the
  simulcast adapter to retain that aspect ratio in any scaling decisions.

This doesn't make scaling decisions spec-compliant (floor the width and
height respectively) but it does allow for control of scaling via
setParameters and keeps scaling decisions in upstream code to ensure
good compat with upstream's part of the pipe; encoders, etc.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 43 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bef05ce81a318fed25e2e00e12c07446a3c6a0f

	Dan Minor [:dminor]
	Comment 44 • 6 years ago

Comment on attachment 9003549 [details]
Bug 1253499 - Add scaleDownBy encoding gtest. r?dminor

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 45 • 6 years ago

Comment on attachment 9003550 [details]
Bug 1253499 - Add simulcast scaleDownBy encoding gtest. r?dminor

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 46 • 6 years ago

Comment on attachment 9003552 [details]
Bug 1253499 - Update gtests to cover live-updates to codec config and new algorithm. r?dminor

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 47 • 6 years ago

Comment on attachment 9003553 [details]
Bug 1253499 - Add gtest for scaling all layers. r?dminor

Dan Minor [:dminor] has approved the revision.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 48 • 6 years ago

Comment on attachment 9003545 [details]
Bug 1253499 - Make VideoFrameEmitter use both width and height. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 49 • 6 years ago

Comment on attachment 9003548 [details]
Bug 1253499 - Add live setParameters checks to simulcast_OddResolution mochitest. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

	Byron Campen [:bwc]
	Comment 50 • 6 years ago

Comment on attachment 9003555 [details]
Bug 1253499 - Make RTPSender.setParameters reconfigure the encoder. r?bwc

Byron Campen [:bwc] has approved the revision.

	Byron Campen [:bwc]
	Comment 51 • 6 years ago

Comment on attachment 9003554 [details]
Bug 1253499 - Implement a new scaling algorithm for simulcast. r?dminor, r?bwc

Byron Campen [:bwc] has approved the revision.

	Byron Campen [:bwc]
	Comment 52 • 6 years ago

Comment on attachment 9003546 [details]
Bug 1253499 - Update simulcast mochitests to cover new scaling algorithm. r?bwc

Byron Campen [:bwc] has approved the revision.

	Byron Campen [:bwc]
	Comment 53 • 6 years ago

Comment on attachment 9003544 [details]
Bug 1253499 - Assert that chrome-only RID methods are succesful. r?bwc

Byron Campen [:bwc] has approved the revision.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 55 • 6 years ago

Comment on attachment 9003547 [details]
Bug 1253499 - Add mochitest for live-updating scaleResolutionDownBy. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 56 • 6 years ago

Comment on attachment 9009103 [details]
Bug 1253499 - Add method for changing size to VideoFrameEmitter. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 60 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5efd9818d2d744e58ffecb4d6766cf975b8916c5

	Dan Minor [:dminor]
	Comment 61 • 6 years ago

Comment on attachment 9009945 [details]
Bug 1253499 - Keep a single video codec config in MockCall. r?dminor

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 62 • 6 years ago

Comment on attachment 9009946 [details]
Bug 1253499 - Fix VideoConduit's maxFps gtest. r?dminor

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 63 • 6 years ago

Comment on attachment 9009947 [details]
Bug 1253499 - Don't lock the conduit's mutex in VideoStreamFactory to avoid deadlocks. r?dminor

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 64 • 6 years ago

Comment on attachment 9003554 [details]
Bug 1253499 - Implement a new scaling algorithm for simulcast. r?dminor, r?bwc

Dan Minor [:dminor] has approved the revision.

	Dan Minor [:dminor]
	Comment 66 • 6 years ago

Comment on attachment 9010277 [details]
Bug 1253499 - Break out VideoStreamFactory to separate file. r?dminor

Dan Minor [:dminor] has approved the revision.

	Pulsebot
	Comment 67 • 6 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5500e4f115fc
Assert that chrome-only RID methods are succesful. r=bwc
https://hg.mozilla.org/integration/autoland/rev/b27fa580c00c
Make VideoFrameEmitter use both width and height. r=jib
https://hg.mozilla.org/integration/autoland/rev/27cca6c30713
Update simulcast mochitests to cover new scaling algorithm. r=bwc
https://hg.mozilla.org/integration/autoland/rev/f6432123d6e0
Add mochitest for live-updating scaleResolutionDownBy. r=jib
https://hg.mozilla.org/integration/autoland/rev/3461e2709d1b
Add method for changing size to VideoFrameEmitter. r=jib
https://hg.mozilla.org/integration/autoland/rev/66fb680f4138
Add live setParameters checks to simulcast_OddResolution mochitest. r=jib
https://hg.mozilla.org/integration/autoland/rev/b68776d7c9ec
Add scaleDownBy encoding gtest. r=dminor
https://hg.mozilla.org/integration/autoland/rev/4cfdba4ea391
Add simulcast scaleDownBy encoding gtest. r=dminor
https://hg.mozilla.org/integration/autoland/rev/1b81855ea091
Update gtests to cover live-updates to codec config and new algorithm. r=dminor
https://hg.mozilla.org/integration/autoland/rev/6a902d5ec8ea
Add gtest for scaling all layers. r=dminor
https://hg.mozilla.org/integration/autoland/rev/ea21cea8c681
Implement a new scaling algorithm for simulcast. r=bwc,dminor
https://hg.mozilla.org/integration/autoland/rev/06098ed3fac7
Make RTPSender.setParameters reconfigure the encoder. r=bwc
https://hg.mozilla.org/integration/autoland/rev/5c29f92fef2a
Keep a single video codec config in MockCall. r=dminor
https://hg.mozilla.org/integration/autoland/rev/54b716bce11b
Fix VideoConduit's maxFps gtest. r=dminor
https://hg.mozilla.org/integration/autoland/rev/85d4d03a5103
Don't lock the conduit's mutex in VideoStreamFactory to avoid deadlocks. r=dminor
https://hg.mozilla.org/integration/autoland/rev/38ff8f0f79f3
Break out VideoStreamFactory to separate file. r=dminor

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 68 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5500e4f115fc
https://hg.mozilla.org/mozilla-central/rev/b27fa580c00c
https://hg.mozilla.org/mozilla-central/rev/27cca6c30713
https://hg.mozilla.org/mozilla-central/rev/f6432123d6e0
https://hg.mozilla.org/mozilla-central/rev/3461e2709d1b
https://hg.mozilla.org/mozilla-central/rev/66fb680f4138
https://hg.mozilla.org/mozilla-central/rev/b68776d7c9ec
https://hg.mozilla.org/mozilla-central/rev/4cfdba4ea391
https://hg.mozilla.org/mozilla-central/rev/1b81855ea091
https://hg.mozilla.org/mozilla-central/rev/6a902d5ec8ea
https://hg.mozilla.org/mozilla-central/rev/ea21cea8c681
https://hg.mozilla.org/mozilla-central/rev/06098ed3fac7
https://hg.mozilla.org/mozilla-central/rev/5c29f92fef2a
https://hg.mozilla.org/mozilla-central/rev/54b716bce11b
https://hg.mozilla.org/mozilla-central/rev/85d4d03a5103
https://hg.mozilla.org/mozilla-central/rev/38ff8f0f79f3

	Andreas Pehrson [:pehrsons] Assignee
	Comment 69 • 6 years ago

I think this warrants dev-docs. If nothing else then a note in the relnotes for devs.

	Chris Mills [:cmills]
	Comment 70 • 6 years ago

Note the docs team:

I've added a note to the Fx64 rel notes about this:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#APIs

I think you ought check out the docs too, and make sure they represent this change sufficiently.

	Eric Shepherd [:sheppy]
	Comment 71 • 6 years ago

:cmills --

I think all that's needed here to finish up is to perhaps add to setParameters() a "version_added" section to note the arrival of proper support for live changes to these.

	Chris Mills [:cmills]
	Comment 72 • 6 years ago

(In reply to Eric Shepherd [:sheppy] from comment #71)
> :cmills --
> 
> I think all that's needed here to finish up is to perhaps add to
> setParameters() a "version_added" section to note the arrival of proper
> support for live changes to these.

Yup, sounds good to me.

	Eric Shepherd [:sheppy]
	Comment 73 • 5 years ago

The following pages have been added:

https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSendParameters
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSendParameters/encodings
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpEncodingParameters
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpEncodingParameters/maxBitrate
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpEncodingParameters/scaleResolutionDownBy

This page has been updated:

https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSender/setParameters

The BCD files have been created for all of these APIs and should be accurate for Firefox and Chrome at least. PRs submitted:

https://github.com/mdn/browser-compat-data/pull/3217
https://github.com/mdn/browser-compat-data/pull/3214

Now I consider this truly dev-doc-complete. It would be helpful to have someone review the material; since the implementations are a little twisty in terms of which dictionary they put which properties into right now, a second pair of eyes to review would be useful.