webrtc: make transceiver tests work in Firefox

[fippo]

makes most new transceiver tests pass by avoiding addTrack(track) which does not work in Firefox yet.

@henbos PTAL

@alvestrand @foolip it would be great if you folks ran the tests in Firefox at least once before pushing them and/or asked whether that was done during review. Most things that did not work in Firefox were easy to avoid. The setRemoteDescription(offer): ontrack's track.id is the same as track.id fails in Firefox but that is ok, the spec does NOT require this.

[foolip]

@alvestrand @foolip it would be great if you folks ran the tests in Firefox at least once before pushing them and/or asked whether that was done during review. Most things that did not work in Firefox were easy to avoid. The setRemoteDescription(offer): ontrack's track.id is the same as track.id fails in Firefox but that is ok, the spec does NOT require this.

Our plan for this is to first surface test regressions in PRs, and if that works propagate that up into the Chromium CL before it is merged. Manually testing changes in the Chromium tree in other browsers (on the same OS) is possible, but time-consuming, so I don't think that'll fix the problem at scale.

[henbos]

nit: Shouldn't this be indented?

[fippo]

yes, fixed + squashed

[henbos]

If this returns both a track and a stream now, a name like createTrackAndStreamWithCleanup() makes more sense.

However, in the spirit of moving away from streams-with-tracks, you don't really need a stream where the track belongs to it. The relationship, as far as webrtc is concerned, is entirely described in terms of the senders and receivers.

What about not returning the stream, and instead doing this...?

pc.addTrack(track, new MediaStream());

That way we don't have to return an array of two different return values, and the helper method can do just one thing.

Assuming Firefox does not barf at that too. But this is optional, the "add[Track/Transceiver]([0/1/2] stream[/s])" tests already test the "new MediaStream()" semantics.

[fippo]

last i checked the consensus was to avoid helper functions like this entirely. I won't clean up after you all the way :-p
Doing pc.addTrack(track, new MediaStream()) just to avoid limitations of a helper function seems even worse.

[fippo]

renamed to createTrackAndStreamWithCleanup and squashed

[henbos]

It's not to avoid a limitation of a helper function, it's to make the helper function do one thing: create a track that is cleaned up. The stream being created is an implementation detail we don't have to expose, and makes less sense when you want to have 0 or 2 streams for a track. It would be confusion to create multiple tracks just to get multiple streams, this is why we already do "new MediaStream()" in the "add[Track/Transceiver]([0/1/2] stream[/s])" tests, so I thought we could be consistent and do that everywhere.

[fippo]

using new MediaStream() means adding a track that is not part of the stream. While that is probably useful to test it is somewhat artificial. ISTR that Firefox did not like this at some point but it seems to work in FF52 (ESR) even.

WRT to helpers it seems that there is also getNoiseStream which returns a stream. I'd prefer nott o increase the number of different semantics we use even more

[henbos]

Whether the track is part of the stream or not is per-spec completely irrelevant. There is absolutely nothing that says we should test this, other than "this is how the legacy stuff worked". It makes sense to have tests to make sure that the APIs work regardless of if the stream is empty, has the track, has another track, etc, just to make sure that common use cases are supported. But they should not be the basis for designing tests testing APIs that have nothing to say about what the contents of the stream is.

Therefor we should design our tests not with that in mind.

[henbos]

So we should decide this based on: What makes more sense? A helper that gives us a track, or a helper that gives us both?

I argue the first makes slightly more sense, but I also realize I am bikeshedding. It's up to you.

[fippo]

E_TOO_MANY_HELPERS. I still prefer

const stream = await navigator.mediaDevices.getUserMedia({audio: true})
const [ŧrack] = stream.getTracks();

Lines of code are cheap.

Therefor we should design our tests not with that in mind.

We're much better of when taking into account the imperfection of implementations. In this case Firefox not liking addTrack without a stream. This shouldn't stop them from implementing that ofc but the compat hit should be in the addTrack() tests.

[fippo]

@foolip my point is that the current approach, saving dev-tme for chrome and pushing it to mozilla is rather impolite.

[agouaillard-cosmo]

Sorry I'm late to the review. I never get notifications in the first place because of the rather impolite behaviour of some submitters that intentionally block official reviewers.

[henbos]

LGTM with optional comment of making createTrackWithCleanup only return a track, and using new MediaStream() when you want a stream