1393687 - the parameter max-fs and max-fr do not work in firefox 56 beta

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[xpeng]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170814072924

Steps to reproduce:

we use the sdp parameter : max-fs & max-fr to control the video 's resolution & framerate , resently we find that the max-fr do not work in firefox 53/54/55  and the max-fs do not work in firefox 56 beta. 


Actual results:

max-fs & max-fr do not work 


Expected results:

the resolution & framerate should be controlled depend on the max-fs & max-fr

Comment 1

[xpeng]

Is there a new method to control the resolution and framerate?

Comment 2

[xpeng]

max-fs works with firefox 55_0b13 , do not work with firefox 56_0b1 and later version.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Drno or bwc can you confirm if this is regression is expected?

Comment 4

[Byron Campen [:bwc]]

I would not have expected this.

Comment 5

[xpeng]

If the max-fs & max-fr would not be supported . How to control the resolution and framerate now?  I asked the question on mozilla.dev.media group , the suggestion is to use RtpSender.setParameters({ encodings: [{ scaleResolutionDownBy: 
2.0 }] })  to scale down resolution. but it should not be the best solution for us.  I found that the max-fs & max-fr is still in sdp in firefox 56 beta, but do not work now. The firefox can not control the resolution and framerate by GetUserMedia 's constraints like chrome, so we need a way to control them to reach our goal.  It's very important for us , because the firefox 56 release version will be published soon later.

Comment 6

[xpeng]

We compared the code between ff 55_0b13 with 56_0b1  , the code has been ignored :

#if 0
  // XXX What we'd like to do for each simulcast stream...
  if (simulcastEncoding.constraints.scaleDownBy > 1.0) {
    uint32_t new_width = width / simulcastEncoding.constraints.scaleDownBy;
    uint32_t new_height = height / simulcastEncoding.constraints.scaleDownBy;

    if (new_width != width || new_height != height) {
      if (streamCount == 1) {
        CSFLogVerbose(logTag, "%s: ConstrainPreservingAspectRatio", __FUNCTION__);
        // Use less strict scaling in unicast. That way 320x240 / 3 = 106x79.
        ConstrainPreservingAspectRatio(new_width, new_height,
                                       &width, &height);
      } else {
        CSFLogVerbose(logTag, "%s: ConstrainPreservingAspectRatioExact", __FUNCTION__);
        // webrtc.org supposedly won't tolerate simulcast unless every stream
        // is exactly the same aspect ratio. 320x240 / 3 = 80x60.
        ConstrainPreservingAspectRatioExact(new_width * new_height,
                                            &width, &height);
      }
    }
  }
#endif

Comment 7

[Byron Campen [:bwc]]

^

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

(In reply to xpeng from comment #6)
> We compared the code between ff 55_0b13 with 56_0b1  , the code has been
> ignored :
> 
> #if 0
>   // XXX What we'd like to do for each simulcast stream...

that has nothing to do with max-fs and max-fr; that has to do with the limitations of the webrtc.org code around the relative sizes of layers (powers-of-2)

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

WebrtcVideoConduit::SelectSendResolution() implements max-fs:
    // Limit resolution to max-fs while keeping same aspect ratio as the
    // incoming image.
    if (mCurSendCodecConfig->mEncodingConstraints.maxFs) {

The issue is that in 56+, SelectSendResolution is only called on an input size change.  I think in 55 and before it was typically called on the first frame, always.  (i.e. mLastWidth/Height were 0, and the first frame would call this.)

It also calls SelectSendFrameRate, where max-mbps is implemented.  I don't see max-fr in the current code, but that's where one would limit it.

Note that max-fs could also be implemented in ConfigureSendMediaCodec, but probably the code SelectSendResolution should be called from VideoStreamFactory::CreateEncoderStreams(), which is invoked from ConfigureSendMediaCodec -- or some code should be factored out of SelectSendResolution and called from both.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

We'll want to fix this and uplift to beta.

Comment 11

[Gerry Chang [:gchang]]

Track 56+ as new regression in 56.

Comment 12

[xpeng]

max-fr will be supportted next version? we can not control the framerate now. so we have to limit the camera's framerate with getusermedia. It depends on the camera's capability.

Comment 13

[Dan Minor [:dminor]]

(In reply to xpeng from comment #12)
> max-fr will be supportted next version? we can not control the framerate
> now. so we have to limit the camera's framerate with getusermedia. It
> depends on the camera's capability.

As far as I can tell, max-fr has never been supported in Firefox (I can find no code to parse it in our sdp parser), so this isn't so much a regression as a new feature. I've filed a separate bug (Bug 1397428) to track supporting it as this is not the kind of thing that we would normally uplift to beta.

If anyone has any evidence that this has ever worked, please let me know and I'll fix it here. Otherwise, I suspect it will have to wait for Firefox 57 or 58.

Comment 14

[Dan Minor [:dminor]]

There is definitely code in our sdp parser for this, not sure how I missed that yesterday. My main concern was messing around with the sdp parser, I don't think restoring the connections from the parser to VideoConduit will require a large change, so I think it is fine to fix max-fr here as well.

Comment 15

[Dan Minor [:dminor]]

Attachment: Fix for max-fs

Comment 16

[Dan Minor [:dminor]]

Attachment: Fix for max-fr

I've been testing this using: https://jsfiddle.net/dminor/076wmzrb/. I'm planning to spend a bit more time on testing this before asking for review.

I'd also like to write a mochitest for this, but I might do it as a separate patch so as not to delay review / beta uplift.

Comment 17

[xpeng]

That's a good news if the modification also works in H264. you can post it to us to confirm when the new patch created .

the framerate and framesize shoule be controlled with max-fs & max-fr for vp8/vp9 . with max-fs & max-mbps for H264. 


For H264:
I can find the comment here:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#1837
// Limit frame rate based on max-mbps

I am not sure why the max-mbps do not work for h264 since firefox 53 . maybe here:
1.   https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#2442
or
2.   https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#668
    if (mConduit->mCurSendCodecConfig->mName == "H264") {
      if (mConduit->mCurSendCodecConfig->mEncodingConstraints.maxMbps > 0) {
        // Not supported yet!
        CSFLogError(logTag, "%s H.264 max_mbps not supported yet", __FUNCTION__);
      }
    }

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

>       if (mConduit->mCurSendCodecConfig->mEncodingConstraints.maxMbps > 0) {
>        // Not supported yet!

That's just the initial config before media flows.  The first link (#1837) means it should adapt the framerate when the input resolution changes - but as noted earlier, in 56 and later you may not see an initial resolution change that calls SelectSendFrameRate(); someone should set a breakpoint and verify.  You could also force an input resolution change and see if that causes max-mbps to be used to set the framerate.   (You can do this by using a canvas as a video source (canvas.captureStream()), and resize it. )

Comment 19

[Dan Minor [:dminor]]

I just noticed that this is cropping not scaling in response to max-fs. Needs more work.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

You probably need to modify SendVideoFrame() and the AdaptFrameResolution code - be careful, though, it needs to coordinate with requests for resolution changes from the sink.  Basically storing the max-fs and on each real resolution input change calculating the new w/h you want to scale down to, and use that as the maximum/original size for AdaptFrameResolution, or some such

Comment 21

[Dan Minor [:dminor]]

Attachment: Enforce max-fs using VideoAdapter

Thanks for the idea!

I've tested with VP8 and H.264 using this fiddle: https://jsfiddle.net/dminor/076wmzrb/. I'll prepare a mochitest.

I don't believe it's necessary to change when SelectSendResolution is called using this approach, but maybe my test doesn't trigger the right circumstances for it to cause problems.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8906728 [details] [diff] [review]
Enforce max-fs using VideoAdapter

Review of attachment 8906728 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming it passes tests (including some manual ones).  Please try forcing a bad (bandwidth-limited) connection in some manner to ensure it scales down correctly, and scales back up.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1700,3 @@
>        }
> +      mVideoAdapter.OnResolutionRequest(rtc::Optional<int>(max_fs),
> +                                        rtc::Optional<int>());

So long as this gets run for the first frame of a call, ok.

@@ +1904,5 @@
>    if (!mLockScaling) {
> +    mLastSinkWanted = wants;
> +
> +    // limit sink wants based upon max-fs constraint
> +    int max_fs = mCurSendCodecConfig->mEncodingConstraints.maxFs*16*16;

Just to avoid compiler failing to optimize, *(16*16)

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8906728 [details] [diff] [review]
Enforce max-fs using VideoAdapter

Actually mark

Comment 24

[Dan Minor [:dminor]]

Attachment: Enforce max-fs using VideoAdapter

Updated based on review comments, removed old C++ unit tests for max-fs and max-fr calculations which no longer work because the VideoAdapter is doing the calculations. Carrying forward r+.

I verified that we do see SelectSendResolution on the first video frame. I also manually tested that the load adaption still works with this patch in place.

Comment 25

[Dan Minor [:dminor]]

Attachment: Test for max-fs constraint

This adds a test for max-fs based upon the existing test_peerConnection_scaleResolution to ensure that setting max-fs actually changes the resolution.

I didn't write tests for different values of max-fs, that should be covered by [1] which we don't currently run because it brings in some other dependencies that we don't use. It probably makes more sense to get that working rather than to write a duplicate test.

[1] http://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/media/base/videoadapter_unittest.cc

Comment 26

[Dan Minor [:dminor]]

Comment on attachment 8907216 [details] [diff] [review]
Test for max-fs constraint

Looks like ASAN and Android are too slow for this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=954636673fa20c3e55ba78ea56f4a24208b5b2a6

I'll try extending the wait period and if that is unsuccessful, I'll skip the test.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8907216 [details] [diff] [review]
Test for max-fs constraint

Review of attachment 8907216 [details] [diff] [review]:
-----------------------------------------------------------------

why the concern over H264 here?  max-fs, as implemented, will apply to all video codecs equally, so why don't we just test vp8?  Perhaps this was inherited in the test you modified into this?

::: dom/media/tests/mochitest/test_peerConnection_maxFsConstraint.html
@@ +6,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "1244913",

wrong bug #

Comment 28

[Dan Minor [:dminor]]

Attachment: Test for max-fs constraint

Testing both VP8 and H.264 isn't necessary for the way I ended up implementing this fix, but I hit lots of situations where one or the other was broken with the other ways I tried to fix this. I think it would be good to test both in case we end up making further changes here.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17637a9cb3db
Use VideoAdapter to handle max-fs; r=jesup

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift to beta? Thanks. It would need to land today (assuming this lands successfully on m-c)

Comment 32

[Dan Minor [:dminor]]

Comment on attachment 8907205 [details] [diff] [review]
Enforce max-fs using VideoAdapter

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1341285
[User impact if declined]:
The max-fs parameter will not work, which makes it difficult for sites to control video size and thus resource utilization.
[Is this code covered by automated tests?]:
A new mochitest has been written and is waiting review.
[Has the fix been verified in Nightly?]:
Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: 
No, once landed we can get the reporter to verify.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
The only risk I see is a possible negative interaction between bandwidth adaptation and max-fs. I tested for this manually and I don't believe this will be a problem. Otherwise, this only impacts calls where max-fs is set, which is currently broken anyway.
[String changes made/needed]:
None.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8907205 [details] [diff] [review]
Enforce max-fs using VideoAdapter

Fix for video related regression in 56, let's uplift this for beta 12. 

xpeng, once we have beta 12 builds available (tomorrow), can you re-test? Thanks!

Comment 34

[Dan Minor [:dminor]]

Comment on attachment 8905670 [details] [diff] [review]
Fix for max-fr

I added some debug logging to WebrtcVideoConduit::StreamStatistics::Update to verify that this is working as expected. I see some variation, especially if the load adaptation triggers a resolution change, but on average, the frame rate is close to the value specified by max-fr.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9cc348828343

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/17637a9cb3db

Comment 37

[Bulk Bug Changes for mreavy's org]

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

Comment 38

[xpeng]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #33)
> Comment on attachment 8907205 [details] [diff] [review]
> Enforce max-fs using VideoAdapter
> 
> Fix for video related regression in 56, let's uplift this for beta 12. 
> 
> xpeng, once we have beta 12 builds available (tomorrow), can you re-test?
> Thanks!


OK , I will re-test it as soon as possible.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3022e88f7a8f
Fix handling of max-fr parameter; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/a36c26e1b55e
Add mochitest for max-fs; r=jesup

Comment 40

[Dan Minor [:dminor]]

It turns out there is an interaction between max-fs and max-fr. If max-fs is set small enough (e.g. 140) then the max-fr parameter does not seem to have an effect. This is why I had some problems testing the max-fr fix.

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3022e88f7a8f
https://hg.mozilla.org/mozilla-central/rev/a36c26e1b55e

Comment 42

[xpeng]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #33)
> Comment on attachment 8907205 [details] [diff] [review]
> Enforce max-fs using VideoAdapter
> 
> Fix for video related regression in 56, let's uplift this for beta 12. 
> 
> xpeng, once we have beta 12 builds available (tomorrow), can you re-test?
> Thanks!

I have tested the max-fs in firefox 56.0b12 , it works fine.  but the max-fr is still not working.

Comment 43

[Dan Minor [:dminor]]

Comment on attachment 8905670 [details] [diff] [review]
Fix for max-fr

Approval Request Comment
[Feature/Bug causing the regression]:
Not sure, this has been broken since Firefox 52 according to the reporter.
[User impact if declined]:
The max-fs parameter will not limit the frame rate.
[Is this code covered by automated tests?]:
No, frame rate depends upon machine capabilities and would likely lead to flaky tests in automation.
[Has the fix been verified in Nightly?]:
Yes, I checked it just now.
[Needs manual test from QE? If yes, steps to reproduce]:
No. 
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This is a one line change that only has an effect when max-fs is set and max-fs is currently broken anyway.
[String changes made/needed]:
None.

Comment 44

[Dan Minor [:dminor]]

(In reply to xpeng from comment #42)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #33)
> > Comment on attachment 8907205 [details] [diff] [review]
> > Enforce max-fs using VideoAdapter
> > 
> > Fix for video related regression in 56, let's uplift this for beta 12. 
> > 
> > xpeng, once we have beta 12 builds available (tomorrow), can you re-test?
> > Thanks!
> 
> I have tested the max-fs in firefox 56.0b12 , it works fine.  but the max-fr
> is still not working.

Thank you for testing. The max-fr fix has only landed in firefox 57 so far. I've just requested uplift for firefox 56, but it may be too late to uplift it before 56 is released.

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

It (max-fr) will not be in 56, we're considerably too late for that.

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Ryan, did this already land in 56?

Comment 47

[Ryan VanderMeulen [:RyanVM]]

"Bug 1393687 - Use VideoAdapter to handle max-fs." landed for 56 back in comment 35. Any subsequent patches have not. Sounds like that's the expected outcome based on comment 45.

Comment 48

[Liz Henry (:lizzard) (relman/hg->git project)]

OK then I'm going to wontfix this for 56. We have a lot we're juggling right now for RC1 and a certain RC2. This isn't a new regression so it's going to have to wait for 57.

Comment 49

[Dan Minor [:dminor]]

Comment on attachment 8905670 [details] [diff] [review]
Fix for max-fr

That makes sense to me, I didn't ask initially because it was for an older regression, but then I thought it made sense to ask and be told no, just in case :)

Comment 50

[xpeng]

(In reply to Dan Minor [:dminor] from comment #49)
> Comment on attachment 8905670 [details] [diff] [review]
> Fix for max-fr
> 
> That makes sense to me, I didn't ask initially because it was for an older
> regression, but then I thought it made sense to ask and be told no, just in
> case :)

Is there a plan to fix the issue for max-mbps(H264)? as comment in  #Comment 17.

Comment 51

[Dan Minor [:dminor]]

(In reply to xpeng from comment #50)
> (In reply to Dan Minor [:dminor] from comment #49)
> > Comment on attachment 8905670 [details] [diff] [review]
> > Fix for max-fr
> > 
> > That makes sense to me, I didn't ask initially because it was for an older
> > regression, but then I thought it made sense to ask and be told no, just in
> > case :)
> 
> Is there a plan to fix the issue for max-mbps(H264)? as comment in  #Comment
> 17.

That work is tracked in Bug 1376255.

Comment 52

[xpeng]

we find the max-fs does not work when reusing  RTCpeerconnection. maybe we can provide the demo several days later.

Comment 53

[Dan Minor [:dminor]]

(In reply to xpeng from comment #52)
> we find the max-fs does not work when reusing  RTCpeerconnection. maybe we
> can provide the demo several days later.

Please open a new bug for this.

Comment 54

[Dan Minor [:dminor]]

We should upstream the patch for max-fr.