1406937 - Write unittest for the video-encode path through VideoConduit

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Configure VideoConduit like our signaling layer would do it, then pass a frame through the VideoConduit and see that it has the correct properties.

Perhaps most important (as a hard-to-test corner case in mochitests) is to pass in a subsequent frame with a different resolution (both larger and smaller) and see that we call into the mocked encoder stack properly.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1406937 - Add unittests for the video-encode path through VideoConduit;

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

Comment 2

[Dan Minor [:dminor]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=257da20c2f562b4f14531d4b7d2e51a3d957fc57

Comment 3

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Comment on attachment 8932065 [details]
Bug 1406937 - Add unittests for the video-encode path through VideoConduit;

https://reviewboard.mozilla.org/r/203120/#review208514

Excellent!! Now I can easily add tests when I finally get back to bug 1253499 \o/

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:815
(Diff revision 1)
> +  ASSERT_EQ(sink->mVideoFrame.width(), 960);
> +  ASSERT_EQ(sink->mVideoFrame.height(), 640);
> +  ASSERT_GE(sink->mVideoFrame.timestamp(), timestamp);
> +
> +  // maxFs should not force pixel count above what a sink has requested
> +  wants.max_pixel_count = rtc::Optional<int>(3500*16*16);

Can you comment on what the magic numbers here mean?

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:863
(Diff revision 1)
> +  mVideoConduit->StartTransmitting();
> +  SendVideoFrame(1280, 720);
> +  ASSERT_EQ(sink->mVideoFrame.width(), 1280);
> +  ASSERT_EQ(sink->mVideoFrame.height(), 720);
> +  uint32_t timestamp = sink->mVideoFrame.timestamp();
> +  ASSERT_GE(timestamp, 0U);

This is basically testing your `VideoConduitTest` helper method `SendVideoFrame`. Could we make the captureTime an explicit arg in `SendVideoFrame` instead, and assert here that it hasn't been tampered with?

Comment 4

[Dan Minor [:dminor]]

Comment on attachment 8932065 [details]
Bug 1406937 - Add unittests for the video-encode path through VideoConduit;

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29cd64e64bb1
Add unittests for the video-encode path through VideoConduit; r=pehrsons

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/29cd64e64bb1