1406935 - Write unittest for configuring VideoConduit

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P3)

Description

[Andreas Pehrson [:pehrsons]]

Configure VideoConduit similar to how our signaling layer does it, and see that we call into a mocked encoder stack with the right values.

Repeat for different signaling properties, like b=TIAS, max-fs, max-fr. See VideoCodecConfig.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1406935 - Add mocks for AudioStreams, VideoStreams and Call;

Review commit: https://reviewboard.mozilla.org/r/201736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201736/

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1406935 - Add unittests for ConfigureSendMediaCodec;

Review commit: https://reviewboard.mozilla.org/r/201738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201738/

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1406935 - Add unittests for ConfigureRecvMediaCodecs;

Review commit: https://reviewboard.mozilla.org/r/201740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201740/

Comment 4

[Dan Minor [:dminor]]

Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5101f982864c6fca506467c35ab5ccea147f1c1a

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8930618 [details]
Bug 1406935 - Add unittests for ConfigureSendMediaCodec;

https://reviewboard.mozilla.org/r/201738/#review207032


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`


::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:69
(Diff revision 1)
> +                                           UniquePtr<cricket::VideoAdapter>(mAdapter));
> +    std::vector<unsigned int> ssrcs = {42};
> +    mVideoConduit->SetLocalSSRCs(ssrcs);
> +  }
> +
> +  ~VideoConduitTest()

Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override]

  ~VideoConduitTest()
  ^
                      override

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:69
(Diff revision 1)
> +                                           UniquePtr<cricket::VideoAdapter>(mAdapter));
> +    std::vector<unsigned int> ssrcs = {42};
> +    mVideoConduit->SetLocalSSRCs(ssrcs);
> +  }
> +
> +  ~VideoConduitTest()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8930617 [details]
Bug 1406935 - Add mocks for AudioStreams, VideoStreams and Call;

https://reviewboard.mozilla.org/r/201736/#review207312

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h:81
(Diff revision 1)
>      mCall.reset(webrtc::Call::Create(config));
>    }
> +
> +  explicit WebRtcCallWrapper(webrtc::Call* aCall)
> +  {
> +    mCall.reset(aCall);

Assert that aCall is non-null here. And let this and `Create(Call)` take a `UniquePtr<Call>&&` instead, since it transfers ownership?

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8930618 [details]
Bug 1406935 - Add unittests for ConfigureSendMediaCodec;

https://reviewboard.mozilla.org/r/201738/#review207314

Great!

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:83
(Diff revision 1)
> +    unsigned int cbcrplane_length = (width*height + 1)/2;
> +    unsigned int video_length = yplane_length + cbcrplane_length;
> +    uint8_t* buffer = new uint8_t[video_length];
> +    memset(buffer, 0x10, yplane_length);
> +    memset(buffer + yplane_length, 0x80, cbcrplane_length);
> +    return conduit->SendVideoFrame(buffer, video_length, width, height,

Should this call into mVideoConduit instead? Or make it a static function.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:833
(Diff revision 1)
> +  } else {
> +    // Reset to defaults
> +    mSendStreamConfig.rtp.ulpfec.ulpfec_payload_type = -1;
> +    mSendStreamConfig.rtp.ulpfec.red_payload_type = -1;
> +    mSendStreamConfig.rtp.ulpfec.red_rtx_payload_type = -1;

Are these fixes you found thanks to the tests? If so (great!) could you put them in their own patch? :-)

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8930619 [details]
Bug 1406935 - Add unittests for ConfigureRecvMediaCodecs;

https://reviewboard.mozilla.org/r/201740/#review207324

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:199
(Diff revision 1)
> +  codecs.push_back(&codecConfig);
> +  codecs.push_back(&codecConfig);

Why two?

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8930619 [details]
Bug 1406935 - Add unittests for ConfigureRecvMediaCodecs;

https://reviewboard.mozilla.org/r/201740/#review207324

> Why two?

It tests this [1]. I've added a comment to explain why.

[1] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#1321

Comment 10

[Dan Minor [:dminor]]

Attachment: Bug 1406935 - Ensure maximum bitrate is at least minimum bitrate in VideoConduit;

Review commit: https://reviewboard.mozilla.org/r/202110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202110/

Comment 11

[Dan Minor [:dminor]]

Attachment: Bug 1406935 - Reset FEC payload types in ConfigureSendMediaCodec;

Review commit: https://reviewboard.mozilla.org/r/202112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202112/

Comment 12

[Dan Minor [:dminor]]

Attachment: Bug 1406935 - Initialize mRembFbSet in VideoCodecConfig ctor;

Review commit: https://reviewboard.mozilla.org/r/202114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202114/

Comment 13

[Dan Minor [:dminor]]

Comment on attachment 8930617 [details]
Bug 1406935 - Add mocks for AudioStreams, VideoStreams and Call;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201736/diff/1-2/

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8930618 [details]
Bug 1406935 - Add unittests for ConfigureSendMediaCodec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201738/diff/1-2/

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8930619 [details]
Bug 1406935 - Add unittests for ConfigureRecvMediaCodecs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201740/diff/1-2/

Comment 16

[Michael Froman [:mjf]]

Comment on attachment 8931018 [details]
Bug 1406935 - Reset FEC payload types in ConfigureSendMediaCodec;

https://reviewboard.mozilla.org/r/202112/#review207570

Looks good to me.

Comment 17

[Michael Froman [:mjf]]

Comment on attachment 8931019 [details]
Bug 1406935 - Initialize mRembFbSet in VideoCodecConfig ctor;

https://reviewboard.mozilla.org/r/202114/#review207572

Looks good to me.

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8931017 [details]
Bug 1406935 - Ensure maximum bitrate is at least minimum bitrate in VideoConduit;

https://reviewboard.mozilla.org/r/202110/#review207784

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74ecfa0dab0b
Add mocks for AudioStreams, VideoStreams and Call; r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/b1e08eeccbc8
Ensure maximum bitrate is at least minimum bitrate in VideoConduit; r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/34cf807c35e4
Reset FEC payload types in ConfigureSendMediaCodec; r=mjf
https://hg.mozilla.org/integration/autoland/rev/854d24a58633
Add unittests for ConfigureSendMediaCodec; r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/6838edf96b1c
Initialize mRembFbSet in VideoCodecConfig ctor; r=mjf
https://hg.mozilla.org/integration/autoland/rev/a6631a517973
Add unittests for ConfigureRecvMediaCodecs; r=pehrsons

Comment 20

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/74ecfa0dab0b
https://hg.mozilla.org/mozilla-central/rev/b1e08eeccbc8
https://hg.mozilla.org/mozilla-central/rev/34cf807c35e4
https://hg.mozilla.org/mozilla-central/rev/854d24a58633
https://hg.mozilla.org/mozilla-central/rev/6838edf96b1c
https://hg.mozilla.org/mozilla-central/rev/a6631a517973