Closed Bug 1393687 Opened 4 years ago Closed 4 years ago

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

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + wontfix
firefox57 --- fixed

People

(Reporter: xpeng, Assigned: dminor)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

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
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Is there a new method to control the resolution and framerate?
max-fs works with firefox 55_0b13 , do not work with firefox 56_0b1 and later version.
Drno or bwc can you confirm if this is regression is expected?
Flags: needinfo?(drno)
I would not have expected this.
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.
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
^
Flags: needinfo?(rjesup)
Blocks: 1341285
Keywords: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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)
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(dminor)
We'll want to fix this and uplift to beta.
Rank: 15
Priority: -- → P1
Track 56+ as new regression in 56.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Flags: needinfo?(dminor)
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.
(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.
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.
Attached patch Fix for max-fs (obsolete) — Splinter Review
Attached patch Fix for max-frSplinter Review
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.
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__);
      }
    }
>       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. )
I just noticed that this is cropping not scaling in response to max-fs. Needs more work.
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
Flags: needinfo?(dminor)
Flags: needinfo?(drno)
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.
Attachment #8905669 - Attachment is obsolete: true
Flags: needinfo?(dminor)
Attachment #8906728 - Flags: review?(rjesup)
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 on attachment 8906728 [details] [diff] [review]
Enforce max-fs using VideoAdapter

Actually mark
Attachment #8906728 - Flags: review?(rjesup) → review+
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.
Attachment #8906728 - Attachment is obsolete: true
Attachment #8907205 - Flags: review+
Attached patch Test for max-fs constraint (obsolete) — Splinter Review
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
Attachment #8907216 - Flags: review?(rjesup)
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.
Attachment #8907216 - Flags: review?(rjesup)
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 #
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.
Attachment #8907216 - Attachment is obsolete: true
Attachment #8907554 - Flags: review?(rjesup)
Keywords: leave-open
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17637a9cb3db
Use VideoAdapter to handle max-fs; r=jesup
Can you request uplift to beta? Thanks. It would need to land today (assuming this lands successfully on m-c)
Flags: needinfo?(dminor)
Duplicate of this bug: 1376255
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.
Flags: needinfo?(dminor)
Attachment #8907205 - Flags: approval-mozilla-beta?
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!
Flags: needinfo?(xpeng)
Attachment #8907205 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8905670 - Flags: review?(rjesup)
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
(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.
Attachment #8905670 - Flags: review?(rjesup) → review+
Attachment #8907554 - Flags: review?(rjesup) → review+
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/3022e88f7a8f
https://hg.mozilla.org/mozilla-central/rev/a36c26e1b55e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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.
Flags: needinfo?(xpeng)
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.
Attachment #8905670 - Flags: approval-mozilla-beta?
(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.
It (max-fr) will not be in 56, we're considerably too late for that.
Ryan, did this already land in 56?
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
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 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 :)
Attachment #8905670 - Flags: approval-mozilla-beta?
(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.
(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.
we find the max-fs does not work when reusing  RTCpeerconnection. maybe we can provide the demo several days later.
(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.
We should upstream the patch for max-fr.
You need to log in before you can comment on or make changes to this bug.