Closed
Bug 1388129
Opened 7 years ago
Closed 7 years ago
Remote WebRTC video live-shrinks to 80x60 if scaleResolutionDownBy is used (regression)
Categories
(Core :: WebRTC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: jib, Assigned: dminor)
References
Details
(Keywords: regression)
Attachments
(1 file)
2.16 KB,
patch
|
jesup
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
STRs:
1. Open https://blog.mozilla.org/webrtc/fiddle-week-downscale-video-peerconnection/
2. Click the "Result" tab of the embedded fiddle, and share camera.
Expected result:
- Video elements appear at 640x480 and 320x240 respectively, and stay there.
Actual result:
- Video elements appear at 640x480 and 320x240 initially, but after a second or two,
the latter shrinks to 120x90, and then after a sec or two more, shrinks again to 80x60.
Tested on OSX and Win10.
Regression range:
4306:00.22 INFO: Narrowed inbound regression window from [23ad1168, 5bbdb7d3] (4 revisions) to [c7428449, 5bbdb7d3] (2 revisions) (~1 steps left)
4306:00.22 INFO: No more inbound revisions, bisection finished.
4306:00.22 INFO: Last good revision: c7428449127566105bdd94b2823b01e0e5e007d5
4306:00.22 INFO: First bad revision: 5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e
4306:00.22 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7428449127566105bdd94b2823b01e0e5e007d5&tochange=5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e
Comment 1•7 years ago
|
||
Likely a bug in how scaleDownBy is hooked into the config logic. Upstream doesn't have scaleDownBy support, and this code had to be restructured significantly because they switched to an encoder settings Factory API.
Reporter | ||
Updated•7 years ago
|
Rank: 15
Priority: -- → P1
Assignee | ||
Comment 3•7 years ago
|
||
Interestingly works as expected on Linux. I'll switch over to OSX.
Assignee | ||
Comment 4•7 years ago
|
||
The scaling occurs here [1]. When we start out, max_pixel_count is MAX_INT and we get the expected resolution. After a while, the quality scaler notices the low resolution and asks for it to be increased [2]. This causes max_pixel_count to be set here [3] which is then scaled down by the scaling factor, resulting in an even smaller resolution, which causes another request from the quality scaler.
We should either not scale when resolution_request_max_pixel_count_ is set or ignore it when scaleResolutionDownBy is specified. My inclination is to ignore the quality scaler if scaleResolutionDownBy is specified. :jib, do you know if the standards cover this situation? I couldn't find anything in the webrtc.org spec. Thanks!
[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/media/webrtc/trunk/webrtc/media/base/videoadapter.cc#136
[2] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/media/webrtc/trunk/webrtc/modules/video_coding/utility/quality_scaler.cc#170
[3] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/media/webrtc/trunk/webrtc/media/base/videoadapter.cc#125
Status: NEW → ASSIGNED
Flags: needinfo?(jib)
Comment 5•7 years ago
|
||
Conceptually, ScaleResolutionBy should be applied on the input stream (as if the camera was scaled by that amount). Note however that each layer of a simulcast stream can/will have different ScaleResolutionBy values; it simply has to be applied "first". This may be a pain to properly implement in the code. Note that if the actual input resolution changes, all the other dependent values will end up changing (i.e. you can't cache the result of ScaleResoltionBy, you have to apply it each frame). And you don't want to scale twice, you want to calculate the total scale (scaleresolutionby + quality scaling) and then apply it once.
The quality scaler must not be ignored; this handles reducing the resolution when CPU use or bandwidth being used is too high.
ScaleResolutionBy can be used in non-simulcast cases.
Reporter | ||
Comment 6•7 years ago
|
||
What Randell said. ScaleResolutionBy should be treated as a target size specifier. We should never scale higher than what it specifies.
Flags: needinfo?(jib)
Assignee | ||
Comment 7•7 years ago
|
||
This takes the minimum of the requested resolution from the quality scaler and scaleResolutionBy. Testing under system load, I do see it go smaller than 320x240 in response to the quality scaler, but it will return to the requested 320x240 when the load decreases.
Incidentally, this probably didn't reproduce for me on Linux due to me testing on my desktop and not generating enough system load for the quality scaler to kick in.
Attachment #8898943 -
Flags: review?(rjesup)
Comment 8•7 years ago
|
||
Comment on attachment 8898943 [details] [diff] [review]
Fix interaction between quality scaler and scaleResolutionBy
Review of attachment 8898943 [details] [diff] [review]:
-----------------------------------------------------------------
land, and we should uplift to beta
::: media/webrtc/trunk/webrtc/media/base/videoadapter.cc
@@ +136,3 @@
> // approximates (width/scale_resolution_by_) * (height/scale_resolution_by_)
> + int scaled_pixel_count = (in_width*in_height/scale_resolution_by_)/scale_resolution_by_;
> + max_pixel_count = std::min(max_pixel_count, scaled_pixel_count);
This needs some comment about what this is doing and why. This particular set of lines is largely our code, so we may end up trying to upstream it. Also, upstream code has changed a LOT in 61.
Attachment #8898943 -
Flags: review?(rjesup) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1427fb9bbe96
Fix interaction between quality scaler and scaleResolutionDownBy; r=jesup
Comment 10•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8898943 [details] [diff] [review]
Fix interaction between quality scaler and scaleResolutionBy
Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1341285
[User impact if declined]:
Incorrect scaling of video streams when under system load.
[Is this code covered by automated tests?]:
No. I think this would be prone to intermittent failures as the behaviour depends upon the how busy the computer running the tests is, which isn't predictable in automation.
[Has the fix been verified in Nightly?]:
Yes.
[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 change only affects video scaling.
[String changes made/needed]:
None.
Attachment #8898943 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
Comment 12•7 years ago
|
||
Comment on attachment 8898943 [details] [diff] [review]
Fix interaction between quality scaler and scaleResolutionBy
Fix for WebRTC regression in 56, let's uplift this for tomorrow's beta 6 build.
Attachment #8898943 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 14•7 years ago
|
||
(In reply to Dan Minor [:dminor] from comment #11)
> [Is this code covered by automated tests?]:
> No. I think this would be prone to intermittent failures as the behaviour
> depends upon the how busy the computer running the tests is, which isn't
> predictable in automation.
> [Has the fix been verified in Nightly?]:
> Yes.
> [Needs manual test from QE? If yes, steps to reproduce]:
> No.
Setting qe-verify- based on Dan's assessment on manual testing needs.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•