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)

57 Branch
enhancement

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.
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.
https://jsfiddle.net/yxbLvjm6/14/ reproduces the issue nicely. If you have left-right comparison for video elements that should be easy to detect
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)
Sure thing.
Assignee: nobody → dminor
Rank: 25
Flags: needinfo?(dminor)
Priority: -- → P3
See Also: → 1404250
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 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]
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+
Blocks: 1404994
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
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: