scaleResolutionDownBy and maxBitrate don't update live.

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
Rank:
18
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: jib, Assigned: pehrsons)

Tracking

(Depends on 1 bug, {dev-doc-complete})

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(URL)

Attachments

(16 attachments, 5 obsolete attachments)

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
(Reporter)

Description

3 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.
(Reporter)

Comment 1

3 years ago
Paul, is this something you guys need asap?
Flags: needinfo?(paulej)

Comment 2

3 years ago
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

Comment 4

3 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.
(Reporter)

Updated

2 years ago
Summary: scaleResolutionDownBy doesn't update live. → scaleResolutionDownBy and maxBitrate don't update live.
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1338573
(Reporter)

Updated

2 years ago
Rank: 23 → 21
(Assignee)

Comment 6

2 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.
(Reporter)

Updated

2 years ago
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.
(Assignee)

Comment 8

2 years ago
Here's a fiddle I've been using to test this manually: https://jsfiddle.net/pehrsons/7L9k7zLq/
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 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?
Attachment #8872667 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 14

2 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.
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+
(Assignee)

Comment 18

2 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?
(Assignee)

Comment 19

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 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.
Attachment #8872665 - Flags: review+ → review?(rjesup)
(Reporter)

Comment 27

2 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.
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+
(Assignee)

Comment 29

2 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.
(Assignee)

Updated

2 years ago
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
(Assignee)

Updated

8 months ago
Attachment #8872664 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8872665 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8872666 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8872667 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8875774 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Depends on: 1404992
(Assignee)

Comment 32

8 months ago
A failure here typically indicates a test error, so it's useful for debugging.
(Assignee)

Comment 41

8 months 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.
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+
(Reporter)

Comment 48

7 months 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.
Attachment #9003545 - Flags: review+
(Reporter)

Comment 49

7 months 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.
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+
(Reporter)

Comment 55

7 months 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.
Attachment #9003547 - Flags: review+
(Reporter)

Comment 56

7 months 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.
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+

Comment 67

7 months 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
(Assignee)

Comment 69

7 months ago
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.
(Reporter)

Updated

3 months ago
Depends on: 1519556
You need to log in before you can comment on or make changes to this bug.