Closed
Bug 1393687
Opened 7 years ago
Closed 7 years ago
the parameter max-fs and max-fr do not work in firefox 56 beta
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
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)
(deleted),
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dminor
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
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
Updated•7 years ago
|
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.
Comment 3•7 years ago
|
||
Drno or bwc can you confirm if this is regression is expected?
Flags: needinfo?(drno)
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
Updated•7 years ago
|
Blocks: 1341285
Keywords: regression
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
Comment 8•7 years ago
|
||
(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)
Comment 9•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Flags: needinfo?(dminor)
Reporter | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
(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.
Assignee | ||
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
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.
Reporter | ||
Comment 17•7 years ago
|
||
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•7 years ago
|
||
> 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. )
Assignee | ||
Comment 19•7 years ago
|
||
I just noticed that this is cropping not scaling in response to max-fs. Needs more work.
Comment 20•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(drno)
Assignee | ||
Comment 21•7 years ago
|
||
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 22•7 years ago
|
||
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•7 years ago
|
||
Comment on attachment 8906728 [details] [diff] [review] Enforce max-fs using VideoAdapter Actually mark
Attachment #8906728 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 24•7 years ago
|
||
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+
Assignee | ||
Comment 25•7 years ago
|
||
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)
Assignee | ||
Comment 26•7 years ago
|
||
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 27•7 years ago
|
||
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 #
Assignee | ||
Comment 28•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 29•7 years ago
|
||
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)
Assignee | ||
Comment 32•7 years ago
|
||
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+
Assignee | ||
Comment 34•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9cc348828343
Comment 37•7 years ago
|
||
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Reporter | ||
Comment 38•7 years ago
|
||
(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.
Updated•7 years ago
|
Attachment #8905670 -
Flags: review?(rjesup) → review+
Updated•7 years ago
|
Attachment #8907554 -
Flags: review?(rjesup) → review+
Comment 39•7 years ago
|
||
Pushed by dminor@mozilla.com: 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
Assignee | ||
Comment 40•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
![]() |
||
Comment 41•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3022e88f7a8f https://hg.mozilla.org/mozilla-central/rev/a36c26e1b55e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter | ||
Comment 42•7 years ago
|
||
(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)
Assignee | ||
Comment 43•7 years ago
|
||
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?
Assignee | ||
Comment 44•7 years ago
|
||
(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.
Ryan, did this already land in 56?
Flags: needinfo?(ryanvm)
Comment 47•7 years ago
|
||
"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.
Assignee | ||
Comment 49•7 years ago
|
||
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?
Reporter | ||
Comment 50•7 years ago
|
||
(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.
Assignee | ||
Comment 51•7 years ago
|
||
(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.
Reporter | ||
Comment 52•7 years ago
|
||
we find the max-fs does not work when reusing RTCpeerconnection. maybe we can provide the demo several days later.
Assignee | ||
Comment 53•7 years ago
|
||
(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.
Assignee | ||
Comment 54•6 years ago
|
||
We should upstream the patch for max-fr.
Blocks: webrtc_upstream_bugs
You need to log in
before you can comment on or make changes to this bug.
Description
•