New issue
Advanced search Search tips

Issue 787461 link

Starred by 1 user
Status: Fixed
Owner:
hbos@chromium.org
Closed: Nov 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 777777



Sign in to add a comment

Flaky addTrack() test.

Project Member Reported by hbos@chromium.org, Nov 21 2017 
This fix, https://chromium-review.googlesource.com/c/chromium/src/+/753598/, unifies the callbacks and makes the order of ontrack and promise well-defined ( https://crbug.com/777777 ).

However, it exposes another flake, sometimes this fails:

FAIL addTrack() with two tracks and one stream makes ontrack fire twice with the tracks and shared stream. assert_array_equals: Expected the remote stream's tracks to be the [first, second] remote tracks. property 0, expected object "[object MediaStreamTrack]" but got object "[object MediaStreamTrack]"

The CL should land, but before fixing this issue we need to handle tracks being added to streams in the unified callback, not in the stream's OnChanged, assuming that is the cause of this, investigating.

Comment 1 by hbos@chromium.org, Nov 21 2017 

The flaky failure is because the test asserted getTracks() to be (track1, track2) but it was sometimes (track2, track1). The CL linked above where I noticed this is not actually related, the problem is here:
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_media_stream_adapter.cc?type=cs&sq=package:chromium&l=228

The RemoteWebRtcMediaStreamAdapter has a map (ptr order) which can change the order between webrtc stream and WebMediaStream when a stream has multiple tracks.

The reason this was not noticed earlier is because the test that flakes has been marked as flaky due to  https://crbug.com/777777  ever since it was added. We didn't realize there were multiple flakes.

Will fix.

Comment 2 by hbos@chromium.org, Nov 21 2017 

Hmm, for some reason I am unable to to repro on master branch, I'll implement the fix as part of the CL that exposed it.

Comment 3 by hbos@chromium.org, Nov 21 2017 

Or a separate one, making the CL that expose it depend on it.

Comment 4 by hbos@chromium.org, Nov 21 2017 

Here's the fix: https://chromium-review.googlesource.com/c/chromium/src/+/782321 but even after that lands the test will be marked flaky due to  https://crbug.com/777777 .
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 22 2017 

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

commit fbcc09282dc8cd984d220f1bf5135d6f3c44701f
Author: Henrik Boström <hbos@chromium.org>
Date: Wed Nov 22 12:35:22 2017

Make WebMediaStream's track order match webrtc::MediaStreamInterface's.

The previous use of std::map made the track order "random" based on track
pointer values.

This fixes subtle bugs, one of which was detected in
https://chromium-review.googlesource.com/c/chromium/src/+/753598/,
where the stream.getTracks() track order became very flaky. It is
likely this bug is already causing flakiness, but I was unable to
repro outside of that CL.

When this CL and the referenced CL lands we'll be able to remove the
test from TestExpectations as a flaky test. But due to another flake,
this CL is not enough to remove it from TestExpectations.

The test in question:
external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-tracks.https.html

Bug:  787461 
Change-Id: I0d5450bf0534a2148818aa843404d6dd43ac5e0b
Reviewed-on: https://chromium-review.googlesource.com/782321
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518616}
[modify] https://crrev.com/fbcc09282dc8cd984d220f1bf5135d6f3c44701f/content/renderer/media/webrtc/webrtc_media_stream_adapter.cc
[modify] https://crrev.com/fbcc09282dc8cd984d220f1bf5135d6f3c44701f/content/renderer/media/webrtc/webrtc_media_stream_adapter.h

Comment 6 by hbos@chromium.org, Nov 22 2017

Status: Fixed (was: Started)
►

Sign in to add a comment