Closed
Bug 1404039
Opened 7 years ago
Closed 7 years ago
Add a unittest for VideoConduit getting a signal to reduce quality due to load or bandwidth
Categories
(Core :: WebRTC: Audio/Video, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: pehrsons, Assigned: dminor)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Basically we need a testcase for bug 1403714.
Reporter | ||
Comment 1•7 years ago
|
||
Perhaps we can do it with a mochitest if we explicitly remove max-fs from the remote sdp?
Might need a means to overrule the bandwidth estimator through a chrome-only api.
Comment 2•7 years ago
|
||
https://jsfiddle.net/yxbLvjm6/14/ reproduces the issue nicely. If you have left-right comparison for video elements that should be easy to detect
Reporter | ||
Comment 3•7 years ago
|
||
Thanks fippo!
It should be easy to write a mochitest based on our tias test, [1]. We remove max-fs and add resize and frame flow checks to the remote side.
Dan, do you feel comfortable taking this?
[1] http://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_restrictBandwidthWithTias.html
Flags: needinfo?(dminor)
Assignee | ||
Comment 4•7 years ago
|
||
Sure thing.
Assignee: nobody → dminor
Rank: 25
Flags: needinfo?(dminor)
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
I started in on a mochitest but it was awkward to write because video flows for a second or two before the quality adapter calls OnSinkWantsChanged and the video freezes. I thought instead this might be a good place to start with the VideoConduit unit tests we want to write since we don't need much of a framework in place to be able to test this.
This is more of a feedback? than a review?, please let me know what you think.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8919883 [details]
Bug 1404039 - Add a unittest for VideoConduit getting a signal to reduce quality due to load or bandwidth;
https://reviewboard.mozilla.org/r/190820/#review196016
C/C++ static analysis found 2 defects in this patch.
You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:56
(Diff revision 1)
> +};
> +
> +class VideoConduitTest : public ::testing::Test {
> +};
> +
> +TEST_F(VideoConduitTest, TestOnSinkWantsChanged)
Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]
::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:56
(Diff revision 1)
> +};
> +
> +class VideoConduitTest : public ::testing::Test {
> +};
> +
> +TEST_F(VideoConduitTest, TestOnSinkWantsChanged)
Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8919883 [details]
Bug 1404039 - Add a unittest for VideoConduit getting a signal to reduce quality due to load or bandwidth;
https://reviewboard.mozilla.org/r/190820/#review196256
Looks great! Perhaps my suggestion to separate ConfigureSendMediaCodec tests is overboard since that test is clearly for OnSinkWantsChanged. However passing through some video frames would be a good addition since that can affect this logic.
::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:68
(Diff revision 1)
> + videoConduit->ConfigureSendMediaCodec(&codecConfig);
> + videoConduit->OnSinkWantsChanged(wants);
Shouldn't configuring the codec (by our signaling) and OnSinkWantsChanged (by bw or load adaptation) be separate tests?
I think the regular case at startup (for instance) is that we ConfigureSendMediaCodec() and pass in a frame. The frame will then trigger configuration of VideoAdapter.
You can see the VideoTrackEncoder (from MediaRecorder) tests on how to generate frames: http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/media/gtest/TestVideoTrackEncoder.cpp#25
::: media/webrtc/signaling/src/media-conduit/VideoConduit.h:289
(Diff revision 1)
> - explicit WebrtcVideoConduit(RefPtr<WebRtcCallWrapper> aCall);
> + explicit WebrtcVideoConduit(RefPtr<WebRtcCallWrapper> aCall,
> + cricket::VideoAdapter* aVideoAdapter = nullptr);
More of a nit, but I'd prefer if passing the video adapter was required. It could be a UniquePtr rref.
You can also remove `explicit` if you do this.
Attachment #8919883 -
Flags: review?(apehrson) → review+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8919883 [details]
Bug 1404039 - Add a unittest for VideoConduit getting a signal to reduce quality due to load or bandwidth;
https://reviewboard.mozilla.org/r/190820/#review196256
> Shouldn't configuring the codec (by our signaling) and OnSinkWantsChanged (by bw or load adaptation) be separate tests?
>
> I think the regular case at startup (for instance) is that we ConfigureSendMediaCodec() and pass in a frame. The frame will then trigger configuration of VideoAdapter.
>
> You can see the VideoTrackEncoder (from MediaRecorder) tests on how to generate frames: http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/media/gtest/TestVideoTrackEncoder.cpp#25
I have to call ConfigureSendMediaCodec so that mCurSendCodecConfig is not a null pointer at [1]. I agree that we need separate tests of the VideoAdapter, but I'd prefer to handle that as a separate bug (I filed Bug 1410500) although some of the other tests we plan to write might also provide some coverage.
[1] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#1928
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fba516dc5620
Add a unittest for VideoConduit getting a signal to reduce quality due to load or bandwidth; r=pehrsons
![]() |
||
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•