Closed Bug 1404039 Opened 4 years ago Closed 4 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
https://hg.mozilla.org/mozilla-central/rev/fba516dc5620
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.