Closed Bug 1253499 Opened 5 years ago Closed 2 years ago

scaleResolutionDownBy and maxBitrate don't update live.

Categories

(Core :: WebRTC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jib, Assigned: pehrsons)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(16 files, 5 obsolete files)

46 bytes, text/x-phabricator-request
bwc
: review+
Details | Review
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review
46 bytes, text/x-phabricator-request
bwc
: review+
Details | Review
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
bwc
: review+
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
bwc
: review+
Details | Review
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
46 bytes, text/x-phabricator-request
dminor
: review+
Details | Review
Parameters set with setParameters currently only take if set before negotiation happens. It should work to change settings mid-call as well, without renegotiation.
Paul, is this something you guys need asap?
Flags: needinfo?(paulej)
Let me ask.  Either I'll come back with an answer or someone from the endpoint team will weigh in.
Flags: needinfo?(paulej)
tentatively assigning priority
Rank: 23
Priority: -- → P2
Mid-call, this is not so important to us.  It's only important that it works reliably when initially setting up the call.
Summary: scaleResolutionDownBy doesn't update live. → scaleResolutionDownBy and maxBitrate don't update live.
Duplicate of this bug: 1338573
Rank: 23 → 21
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.
Rank: 21 → 18
Priority: P2 → P1
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.
Here's a fiddle I've been using to test this manually: https://jsfiddle.net/pehrsons/7L9k7zLq/
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
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?
Attachment #8872667 - Flags: review?(docfaraday) → review+
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.
Comment on attachment 8872664 [details]
Bug 1253499 - Add a getter for VideoConduit's current send config.

https://reviewboard.mozilla.org/r/144210/#review148286
Attachment #8872664 - Flags: review?(rjesup) → 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?
Attachment #8872665 - Flags: review?(rjesup) → review+
Comment on attachment 8872666 [details]
Bug 1253499 - Ensure the target bitrate never exceeds the max bitrate.

https://reviewboard.mozilla.org/r/144214/#review148290
Attachment #8872666 - Flags: review?(rjesup) → review+
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?
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #18)
> Suggestions on how to fix?
Flags: needinfo?(rjesup)
Flags: needinfo?(docfaraday)
(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.)
Flags: needinfo?(docfaraday)
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}};
Flags: needinfo?(rjesup)
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.
Attachment #8872665 - Flags: review+ → review?(rjesup)
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.
Attachment #8875774 - Flags: review?(jib) → review+
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
Attachment #8872665 - Flags: review?(rjesup) → review+
(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.
Attachment #8872665 - Flags: review+ → review?(rjesup)
Comment on attachment 8872665 [details]
Bug 1253499 - Accomodate for changes to scaleDownBy when configuring send codec.

https://reviewboard.mozilla.org/r/144212/#review157914
Attachment #8872665 - Flags: review?(rjesup) → review+
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Attachment #8872664 - Attachment is obsolete: true
Attachment #8872665 - Attachment is obsolete: true
Attachment #8872666 - Attachment is obsolete: true
Attachment #8872667 - Attachment is obsolete: true
Attachment #8875774 - Attachment is obsolete: true
Depends on: 1404992
A failure here typically indicates a test error, so it's useful for debugging.
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.
Comment on attachment 9003549 [details]
Bug 1253499 - Add scaleDownBy encoding gtest. r?dminor

Dan Minor [:dminor] has approved the revision.
Attachment #9003549 - Flags: review+
Comment on attachment 9003550 [details]
Bug 1253499 - Add simulcast scaleDownBy encoding gtest. r?dminor

Dan Minor [:dminor] has approved the revision.
Attachment #9003550 - Flags: review+
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.
Attachment #9003552 - Flags: review+
Comment on attachment 9003553 [details]
Bug 1253499 - Add gtest for scaling all layers. r?dminor

Dan Minor [:dminor] has approved the revision.
Attachment #9003553 - Flags: review+
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.
Attachment #9003545 - Flags: review+
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.
Attachment #9003548 - Flags: review+
Comment on attachment 9003555 [details]
Bug 1253499 - Make RTPSender.setParameters reconfigure the encoder. r?bwc

Byron Campen [:bwc] has approved the revision.
Attachment #9003555 - Flags: review+
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.
Attachment #9003554 - Flags: review+
Comment on attachment 9003546 [details]
Bug 1253499 - Update simulcast mochitests to cover new scaling algorithm. r?bwc

Byron Campen [:bwc] has approved the revision.
Attachment #9003546 - Flags: review+
Comment on attachment 9003544 [details]
Bug 1253499 - Assert that chrome-only RID methods are succesful. r?bwc

Byron Campen [:bwc] has approved the revision.
Attachment #9003544 - Flags: review+
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.
Attachment #9003547 - Flags: review+
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.
Attachment #9009103 - Flags: review+
Comment on attachment 9009945 [details]
Bug 1253499 - Keep a single video codec config in MockCall. r?dminor

Dan Minor [:dminor] has approved the revision.
Attachment #9009945 - Flags: review+
Comment on attachment 9009946 [details]
Bug 1253499 - Fix VideoConduit's maxFps gtest. r?dminor

Dan Minor [:dminor] has approved the revision.
Attachment #9009946 - Flags: review+
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.
Attachment #9009947 - Flags: review+
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.
Attachment #9003554 - Flags: review+
Comment on attachment 9010277 [details]
Bug 1253499 - Break out VideoStreamFactory to separate file. r?dminor

Dan Minor [:dminor] has approved the revision.
Attachment #9010277 - Flags: review+
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
I think this warrants dev-docs. If nothing else then a note in the relnotes for devs.
Keywords: dev-doc-needed
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.
: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.
(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.
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.
Depends on: 1519556
You need to log in before you can comment on or make changes to this bug.