Closed
Bug 1406937
Opened 8 years ago
Closed 8 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•8 years ago
|
Rank: 28
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
Reporter | ||
Comment 3•8 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•