New issue
Advanced search Search tips

Issue 901711 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

virtual/webrtc-wpt-unified-plan/external/wpt/webrtc/simplecall-no-ssrcs.https.html failing on chromium.mac/Mac10.13 Tests (dbg)

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Nov 5

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of ksakamoto@google.com

virtual/webrtc-wpt-unified-plan/external/wpt/webrtc/simplecall-no-ssrcs.https.html failing on chromium.mac/Mac10.13 Tests (dbg)

Builders failed on: 
- Mac10.13 Tests (dbg): 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29


 
Cc: hta@chromium.org
Components: Blink>WebRTC
Owner: hbos@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3395e692ae7fc77adf5675d94b3ab4420a9f8799

commit 3395e692ae7fc77adf5675d94b3ab4420a9f8799
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Mon Nov 05 04:31:38 2018

Mark simplecall-no-ssrcs.https.html as failure on Mac

TBR=hbos@chromium.org,hta@chromium.org

Bug:  901711 
Change-Id: I4ed1826e728cc777924ce4ab54050308db0bff31
Reviewed-on: https://chromium-review.googlesource.com/c/1316977
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605246}
[modify] https://crrev.com/3395e692ae7fc77adf5675d94b3ab4420a9f8799/third_party/WebKit/LayoutTests/TestExpectations

Cc: philipp....@googlemail.com
It does fail only with unified plan:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29/6181
* virtual/webrtc-wpt-unified-plan/external/wpt/webrtc/simplecall-no-ssrcs.https.html

ICE connects but ontrack is not called with any streams. This is the case for plan-b. Take this fiddle:
  https://jsfiddle.net/5jd8Lz97/
and change sdpSemantics as appropriate.

hta: good thing we had a test :-)
The behavior is correct, the test is doing wrong things. This is passing in Plan B essentially because of a bug, though you might call it a feature.

The MSIDs are carried in the SSRC lines, if you remove those that is equivalent to a track that doesn't belong to any stream.

The test is wired that in ontrack it sets the srcObject to the remote stream. Of course there is no remote stream, and there shouldn't be any remote stream, so the test fails.
This worked in Plan B because Plan B didn't support not having streamless tracks.

The fix is to replace this:

  var onRemoteTrack = test.step_func(function(event) {
    var videoTag = document.getElementById('remote-view');
    if (!videoTag.srcObject) {
      videoTag.srcObject = event.streams[0];  // <-- null
    }
  });

With this:

 var onRemoteTrack = test.step_func(function(event) {
    var videoTag = document.getElementById('remote-view');
    if (!videoTag.srcObject) {
      videoTag.srcObject = new MediaStream([event.track]);
    }
  });

Labels: -Pri-1 Pri-2
Labels: -Sheriff-Chromium
The SDP without the a=msid (or a=ssrc:msid) is legitimate and has been for longer than WebRTC exists.

The unified plan code seems to assume it can apply msid semantics in the absence of msid. This is going to cause issues...
SDP without MSIDs is legitimate and Unified Plan doesn't break that, it accurately reflects what was signaled.

One could change the spec to require streams but the only reason to do that as far as I can tell is for backwards compatibility, but I'm not convinced that is such a big issue. People who relied on streams likely already used streams and would not be unaffected. If there are edge cases where people are relying on streams without signaling them, they fixing their stuff is a one-liner.

I'll take this use case seriously when I'm convinced this use case is heavily relied upon. But remember that addStream and Firefox's addTrack never had streamless tracks.
We are discussing the behaviour of one of the most fundamental use-case:
one audio and one video track, signalled by SDP, no msid lines. In this case no a=ssrc lines.
This has resulted in a single stream ever since and that behaviour changes with unified plan. It continues to work in Firefox.

For purism, in unified plan mode it should not look at a=ssrc:... msid lines but it does which keeps a gateway i know from breaking.
Moving back the from github. Deep down in the msid draft we have this:
```
   o  If there is no msid attribute, the identifier of the
      MediaStreamTrack will be set to a randomly generated string, and
      it will be signalled as being part of a MediaStream with the
      WebIDL "label" attribute set to "Non-WebRTC stream".
```
which is more about packets for unsignaled tracks but is the closest I could find. Strictly speaking it seems wrong to fire until you have data?

JSEP has a provision for a=msid not being present:
If present, "a=msid" attributes MUST be parsed as specified in [I-D.ietf-mmusic-msid], Section 3.2, and their values stored, ignoring any "appdata" field.
What happens to unsignaled tracks is another issue which I know less about.

The webrtc-pc spec is clear about what to do with signaled tracks without msids; it says to only create MediaStreams for MSIDs that "the media description indicates", otherwise don't create any (as explained here https://github.com/versatica/mediasoup-client/issues/50#issuecomment-435930577).
https://github.com/rtcweb-wg/jsep/issues/856 -- filed a JSEP issue. I agree that what you quote from webrtc-pc seems correct but at the same time this has the potential for breaking quite some things.
Status: Started (was: Available)
Started on fixing this, note that the bug status reflects this test in the case of no MSIDs signaled; there is follow-up work to be done - on the spec, and then on testing and implementation if there is agreement.
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f68d96bef2b1216fdc2197f054f028c53b422117

commit f68d96bef2b1216fdc2197f054f028c53b422117
Author: Henrik Boström <hbos@chromium.org>
Date: Tue Nov 06 15:17:41 2018

Fix and re-enable simplecall-no-ssrcs.https.html.

With the changes, the test should pass regardless of if the Chrome or
Firefox behavior is used for what to do when no MSIDs are signaled.

Follow-up work is needed to test both the case of "a=msid:-" and no
"a=msid" line whatsoever, awaiting spec issue to be resolved:
https://github.com/w3c/webrtc-pc/issues/2027

// Relying on fippo's approval to land.
TBR=hta@chromium.org

Bug:  901711 
Change-Id: Icaf089d206413510aadad54a0c10b2674450385a
Reviewed-on: https://chromium-review.googlesource.com/c/1319612
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605678}
[modify] https://crrev.com/f68d96bef2b1216fdc2197f054f028c53b422117/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f68d96bef2b1216fdc2197f054f028c53b422117/third_party/WebKit/LayoutTests/external/wpt/webrtc/simplecall-no-ssrcs.https.html
[delete] https://crrev.com/c0fe73103abe68313dcd49155dbd0c6ca5c11b44/third_party/WebKit/LayoutTests/platform/mac-mac10.12/virtual/webrtc-wpt-unified-plan/external/wpt/webrtc/simplecall-no-ssrcs.https-expected.txt
[delete] https://crrev.com/c0fe73103abe68313dcd49155dbd0c6ca5c11b44/third_party/WebKit/LayoutTests/platform/win/virtual/webrtc-wpt-unified-plan/external/wpt/webrtc/simplecall-no-ssrcs.https-expected.txt

Status: Verified (was: Started)
Follow-up issues will surely follow based on https://github.com/w3c/webrtc-pc/issues/2027.
This issue, about testing if video is flowing despite the lack of ssrcs, is fixed and Verified.
Labels: M-72
Note: In the msid draft, "unsignaled track" and "track without msid" means the same thing.
This terminology was caused by the original version of msid being plan-b based.

Agree fully that this is an use case that has to Just Work.
Labels: Type-Bug

Sign in to add a comment