Closed Bug 1406937 Opened 3 years ago Closed 2 years ago

Write unittest for the video-encode path through VideoConduit

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: pehrsons, Assigned: dminor)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
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?
Attachment #8932065 - Flags: review?(apehrson) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29cd64e64bb1
Add unittests for the video-encode path through VideoConduit; r=pehrsons
https://hg.mozilla.org/mozilla-central/rev/29cd64e64bb1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.