Thank you for the thorough bug report!

> but let me know if it would help to write up a minimal test page and I'll do so.
If it's not too much trouble, would you create a minimal test page?

As for triaging - I'm not sure if this falls in the realm of tommi@'s team or pthatcher@'s, so I'm randomly setting pthatcher as owner. I'm setting this as a P1 in the hopes that someone can identify what fixed this in 51 (or broke this in 50) so that we can try to request an appropriate merge before 50 goes to the stable channel.

repro'd in 50.0.2661.49 by setting
  pc2.getRemoteStreams()[0].getVideoTracks()[0].enabled = false
and then to true again on  https://webrtc.github.io/samples/src/content/peerconnection/pc1/
event log attached. When converting it to a pcap with https://github.com/fippo/dump-webrtc-event-log the extra bye is in packet #837

Deleted: bye.log 225 KB

bye.log 225 KB View Download

Probably we are (were?) calling StopSend as a result of the disable, which seems wrong. Disable should just mute the outbound media.

fippo, can you provide some guidance on using dump-webrtc-event-log? I tried using "node rtp.js bye.log | text2pcap -u 10000,20000 - bye.pcap" but the 837th packet didn't look like a "bye".

Also, I couldn't reproduce the issue with 50.0.2661.49, using the "pc1" sample and disabling the remote track. Though if I disable the LOCAL track, a BYE is sent. Is there something I'm missing?

taylor: did you right-click and "decode as" -> "rtcp" in wireshark when viewing the bye.pcap? second report in packet.

Coudld not repro with 50.0.2661.57 that way anymore. Might be a good sign if that is fixed.

rolly?

If I decode as RTCP all I see is "RTCP frame length check: OK - 0 bytes". (Sorry, I'm a pretty novice Wireshark user).

Also, there hasn't been any WebRTC change on the m50 branch for two weeks, so I doubt the root problem is fixed. I still can't reproduce it though.

I confirm that the issue is still happening in version 50.0.2661.57 beta (64-bit)

I still can't reproduce it, on any version. Would it be possible to provide a minimal test page, as you offered earlier? And/or provide a chrome debug log?

Here is a test page:
https://dl.dropboxusercontent.com/u/2001525/bye/sender.html

Start the sender, and then click in the "mute and unmute sender".

If you want to use directly wireshark, you need to disable encription for webrtc:
/opt/google/chrome-beta/google-chrome-beta --disable-webrtc-encryption

Then open wireshark and capture the loopback interface. When clicking on the mute-unmute button, you will see the bye packet passing through

The test page disables and enables the track of the sender (which I did notice sends a BYE back in comment #8).

But I thought this bug was about disabling and enabling the track of the receiver? 

oh, you are totally right. Even though the description refers to the receiver, the bug applies to both of them. In the page that fippo commented, you can also use this code to repro the bug in the sender:

pc1.getLocalStreams()[0].getVideoTracks()[0].enabled = false; pc1.getLocalStreams()[0].getVideoTracks()[0].enabled = true;

Just to be clear: do you ever see this bug if you *only* disable the track in the remote stream? Because that's what I've been unable to reproduce.

If this only occurs when disabling the local track, I can start figuring out what CLs can be cherry-picked to fix it in M50. I just want to be sure I fully understand the cause of the issue.

no, it when I *enable* the track again after it has been disabled

I modified your test page to disable/enable the remote tracks, and ran with google-chrome-beta, but I still do not see any goodbye messages.

If possible, could you attach a chrome debug log to this bug (ideally also logging when you flip the "enabled" flag)?

In the meantime, I'll at least work on fixing the "bye" message when the local track is disabled.

[Automated comment] Less than 2 weeks to go before stable on M50, manual review required.

Has the issue been fixed? I don't see what classes we're asking to be merged. 

Cl, not classes (auto correct) 

The issue was fixed recently by this CL: https://codereview.webrtc.org/1840043005

Here's a CL to cherry pick this fix into M50: https://codereview.webrtc.org/1870283002/

Great! Thanks for the update.  Do you think this is serious enough to ask for a merge to 50 (it'll be going to stable in just over a week I would guess)?  We'll need to have some bake time on dev and beta first (looks like the fix just made it in time before beta), so we'd probably be looking at merging in about a week's time anyway.

The merge request process requires confirmation that the fix works in Canary (proxy for ToT). https://codereview.webrtc.org/1840043005 looks like it landed in trunk about a week ago, and from looking at DEPS files, it looks like the fix first appears in Chrome 51.0.2701.0. 

deadbeef@, can you confirm that you've verified that your fix works with Chrome 51.0.2701.0 (or a newer version)?

Yes, I've verified it works in Chrome 51.0.2705.0. Also, I believe the fix is low-risk, since it just adds back a block of code that was inadvertently deleted during refactoring.

As for how serious the issue is: would you care to weigh in, jpujol@? How many systems are affected by the spurious BYE?

RFC 3550 states the following: "When a participant wishes to leave a session, a BYE packet is transmitted to inform the other participants of the event."

So my understanding is that systems in the middle of the conference may disconnect the bye sender, according to that.

Because of it, I think it is a very important issue to be fixed

Merge approved for M50 (branch 2661). Pls go ahead merge.

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/302fb224a1b462b42d806a8b509122da89ad8142

commit 302fb224a1b462b42d806a8b509122da89ad8142
Author: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Mon Apr 11 23:31:41 2016

Don't reconfigure the encoder if the video options aren't changing.

Review URL: https://codereview.webrtc.org/1840043005

Cr-Commit-Position: refs/heads/master@{#12222}
(cherry picked from commit 8ef0c11d69e94c9ebc39f30cf6702c38285ca1ca)

BUG= chromium:598928 
R=pbos@webrtc.org

Review URL: https://codereview.webrtc.org/1870283002 .

Cr-Commit-Position: refs/branch-heads/50@{#8}
Cr-Branched-From: a5d8e4eef58574ddb4c4c818de71491a52887f45-refs/heads/master@{#11765}

[modify] https://crrev.com/302fb224a1b462b42d806a8b509122da89ad8142/webrtc/media/engine/fakewebrtccall.cc
[modify] https://crrev.com/302fb224a1b462b42d806a8b509122da89ad8142/webrtc/media/engine/fakewebrtccall.h
[modify] https://crrev.com/302fb224a1b462b42d806a8b509122da89ad8142/webrtc/media/engine/webrtcvideoengine2.cc
[modify] https://crrev.com/302fb224a1b462b42d806a8b509122da89ad8142/webrtc/media/engine/webrtcvideoengine2_unittest.cc

Taylor@ Is this fixed? If not -- is this Pri-1 (given the time of the last update)?

Yes, it was fixed and merged to M50, just forgot to update status.