Closed
Bug 1406937
Opened 7 years ago
Closed 7 years ago
Write unittest for the video-encode path through VideoConduit
Categories
(Core :: WebRTC: Audio/Video, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla59
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.
Reporter | ||
Updated•7 years ago
|
Rank: 28
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=257da20c2f562b4f14531d4b7d2e51a3d957fc57
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/29cd64e64bb1 Add unittests for the video-encode path through VideoConduit; r=pehrsons
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29cd64e64bb1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•