917916 - MediaConduit unit tests do not match how we use conduits in the code and need better RTCP checks

Status: RESOLVED INCOMPLETE

(Core :: WebRTC: Networking, defect, P4)

Description

[Randell Jesup [:jesup] (needinfo me)]

The mediaconduit_unittests use incomplete/wrong abstractions of how conduits are used and interact with pipelines and transports.  For example, both ConfigureSendMediaCodec() and ConfigureRecvMediaCodecs() are called on a single conduit, but in practice we can't had a conduit with both currently.  The fake transports are a single unidirectional RTP/RTCP transport, while really there are two bidirectional transports (each taking RTP in one direction, and RTCP in the other).

Also, it doesn't have any tests that check packet reception (rtp or rtcp), though it reports values in text.  And it reports 0's for frames received always, though there's code to accumulate the numbers.

They also only check a send-only scenario.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Make mediaconduit unittests match real-world usage and check packet reception

first part (make it closer to real-world use).  Will fail on the SR checks currently.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: extend mediaconduit testing to be both bidirectional and unidirectional

Add bidirectional checks (as well as unidirectional), and enable the renderer sink for video so we can count received frames.  NOTE: assumes it's applied on top of the other patch here *and* the updated bug 864654 patch.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Note: these patches are slightly bitrotted due to bug 864654 changing design before landing

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 806794 [details] [diff] [review]
Make mediaconduit unittests match real-world usage and check packet reception

revectoring to byron - you may want to send it to Nils instead.  

This may well still be relevant; I haven't checked how much this has changed.  Likely moderately to highly bitrotted.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 806799 [details] [diff] [review]
extend mediaconduit testing to be both bidirectional and unidirectional

Ditto - this patch is more likely to be overtaken by events than the other, but worth checking

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 806794 [details] [diff] [review]
Make mediaconduit unittests match real-world usage and check packet reception

Review of attachment 806794 [details] [diff] [review]:
-----------------------------------------------------------------

I think that all of the stuff in here that breaks conduits up into one send and one recv conduit is no longer needed, since we don't split these anymore. Tracking RTCP better is nice though, and I don't think we have a unit-test that keeps track of SR and RR separately right now.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 806799 [details] [diff] [review]
extend mediaconduit testing to be both bidirectional and unidirectional

Review of attachment 806799 [details] [diff] [review]:
-----------------------------------------------------------------

This will probably need to be completely reworked, since we don't do the split conduit stuff at all now.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Byron Campen [:bwc] from comment #6)
> Comment on attachment 806794 [details] [diff] [review]
> Make mediaconduit unittests match real-world usage and check packet reception
> 
> Review of attachment 806794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that all of the stuff in here that breaks conduits up into one send
> and one recv conduit is no longer needed, since we don't split these
> anymore. Tracking RTCP better is nice though, and I don't think we have a
> unit-test that keeps track of SR and RR separately right now.

We definitely want a unit test that verifies SR's get through (as that was broken for a long time due to bug 864654) and better RTCP checking.

Comment 9

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 10

[Dan Minor [:dminor]]

We should be adding new media conduit unit tests to audioconduit_unittest and videoconduit_unittest. I'm planning to remove mediaconduit_unittest in Bug 1420893. The audio portion of this test has been disabled for years and the video portion is now redundant with what is in videoconduit_unittest.