Do we know whether this is a WebRTC issue, a Hangouts backend issue, or ... ?

Re #1: It reminded me of the old issues we had with bandwidth estimate dropping to some very low value - I was able to flip through slides pretty quickly if I did so in quick succession, but things got unresponsive after being on the same slide for some time.

The other candidate area that may cause this would be the tab-capture API, since we stop generating frames after a little while (I think?) if the tab isn't actually changing.

+miu, amp: Guys, any thoughts on how to diagnose further?

I haven't touched Cast to Hangouts code for months now, but one of the last things we were looking at was improving the handling of static content (exactly like this bug).

I thought we had modified it to always send a constant fps (1 or 5, I can't remember).  In which case ~10s delay should not be possible, but I have definitely anecdotally seen this same behavior in recent presentations.

My knowledge is out of date, though, so I don't know the current settings, or if the underlying code has been changed.  (I think there is another bug with more discussion, it might have been something related to simulcast).

Hmm...this shouldn't be a problem anymore. There was internal discussion about Hangouts using 5 FPS as their minimum setting. I think this is a Hangouts bug, and not a tab capture bug, unless the 5 FPS min constraint is not being respected. I'll double-check and loop in the relevant folks...

Okay. This is either a WebRTC or Hangouts bug. I've confirmed that capture is occurring at 5 FPS when content is not changing. Furthermore, I can increase the capture frame rate when I move the mouse (since the cursor will be redrawn). In the Hangout, I see very severe frame dropping (updates only once every few seconds, typically). I did not turn on "smooth motion" for this.

A hint at the the possible problem: When I turn on logging in Chrome, I get this error message repeated at a rate of 5 FPS:

[1:15:0524/182732.331791:WARNING:statscollector.cc(973)] The SSRC 1576092047 is not associated with a receiving track

From here: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?rcl=01915025bc1873e556d09b5083b5d7ccc31da068&l=973

sprang: Does this hint at the problem? Can you take a look?

I just checked chrome://webrtc-internals. When I continuously move the mouse around, the graphs show googFrameRateInput at a solid 30 FPS. However, the googFrameRateSent bounces around between 5 and 12 FPS.

Nevertheless, on my other machine, the video is definitely updating at less than 5 FPS: It's like a few FPS, then freeze for a few seconds, then a few FPS. So, frames seem to be getting dropped after WebRTC sends them, but before they are played back on the other client.

Also, I should correct comment #5: Capture is not at 5 FPS on an unchanging screen. According to the graphs, it's actually 1 FPS. So, the MR Extension isn't setting the min frame rate to 5 (the default is 1). I could try setting it to 5, but I don't see that as being the problem at-hand (given what we see happens at 30 FPS).

Quick update...
I could repro this on a chromebook (veyron-minnie) on dev channel. When user input screen changes stops stops, the input frame rate drops to 0 - then after a while bunch a FIR requests arrive and then nothing until something on screen changes or the mouse is moved.
There seem to be something else going on as well, as we're only sending a single stream at 1mbps, which will be too high for some to receive.

Yep, this looks specific to capturer on chromeos.

I tried correlating this with the simulcast screenshare experiment, but that actually improved things. The 1mbps without temporal layers is an artifact of the hw encoder and has nothing to do with this.

amp@, can you verify if min fps is actually configured correctly? If at all possible, update it to 5fps as well since we'll have significant quality reductions even with 1fps.

I'm no longer on the media router team and I'm not sure if the config has changed since I left (nor can I remember where the config is located anymore).

Assigning to miu@ has he may know or at least know who would know.

Ping miu@, have you been able to test this on cros?

No, but in #c6, the sender was Chrome on Linux and the receiver was Chrome on Mac.

miu, I think the problem observed in #6 is different from what's reported here. I can easily reproduce this on Linux M60 and googFrameRateInput is stuck at 0 for a long time, 10 seconds or so. See attached screenshot, between the two last spikes the shared presentation was stuck on the wrong slide. The last spike occurred when I moved my mouse. 

Deleted: Screenshot from 2017-05-31 15:10:33.png 7.6 KB

	Screenshot from 2017-05-31 15:10:33.png 7.6 KB View Download

Hmm...So, a guy on the Chrome for Education team just brought a related issue to my attention (first frame not making it to the receiver after launching mirroring). That was on M59, though, so not sure if it's the same issue.

RCR'ed. This was caused by the following change:

commit 7f1e2a9882f033206ffac41e8f1b5b0ee7aee634
Author: guidou <guidou@chromium.org>
Date:   Thu Apr 6 03:41:23 2017

    Spec compliant video constraints for getUserMedia behind flag.

The change in #c17 changed the behavior of the refresh_interval computation. It does not use the default refresh interval when the min/max frame rate are not provided if the legacy-form constraints are given to gUM() call.

With some "printf debugging" I found that it actually sets the refresh interval to 60 seconds. Yes, that's one frame per 60 seconds, and I confirmed it was doing that. It should be 1 per second. The reason is that min_frame_rate variable seems to be 0.0166667 (which is passes the > 0.0 check) even if no minimum framerate was ever specified in the constraints.

guido: PTAL. Feel free to use me to review any patches on this issue. :-)

Also, in the meantime, I'll look into seeing whether I can set the min frame rate from the Media Router extension to fix Hangouts (and Cast for Education).

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

commit 9d5685faf6178778aaec42384e4c3f81890799b5
Author: guidou <guidou@chromium.org>
Date: Fri Jun 02 18:19:45 2017

Change default minimum screencast frame rate to zero.

This is causing issues with refresh-interval computations that
treat zero as a special value and expect zero as the default
minimum, as it was with the old constraints algorithm.

BUG= 725696 

Review-Url: https://codereview.chromium.org/2918113002
Cr-Commit-Position: refs/heads/master@{#476728}

[modify] https://crrev.com/9d5685faf6178778aaec42384e4c3f81890799b5/content/renderer/media/media_stream_constraints_util_video_content.cc
[modify] https://crrev.com/9d5685faf6178778aaec42384e4c3f81890799b5/content/renderer/media/media_stream_constraints_util_video_content.h
[modify] https://crrev.com/9d5685faf6178778aaec42384e4c3f81890799b5/content/renderer/media/media_stream_constraints_util_video_content_unittest.cc

The patch I landed should fix it. 
wez@, miu@: can you verify it? 
I think we should merge this to both 60 and 59.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-60; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-60 label, otherwise remove Merge-TBD label. Thanks.

Wait...This fixed one issue but introduced a new bug: The problem is that we want the default to be used when the minimum frame rate is not specified. However, if it *is* specified as zero, we will now erroneously use the default instead. This means there's no way to say "zero" minimum framerate anymore.

IMHO, the above change should be reverted. Instead, the return values of the constraints getters should be checked (https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc?rcl=cbf4515e37ae65e2b73088888e826311a4d20787&l=295). If false, it means the constraint wasn't specified and the default should be used. If true, then we actually have numbers we should use and so the default should NOT be used.

For a refresher, I'd recommend looking at how the original code change broke things to understand what the desired behavior is: https://chromium.googlesource.com/chromium/src/+/7f1e2a9882f033206ffac41e8f1b5b0ee7aee634.

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

commit 56ed98a48e61f1ba075a5352939155379f21b2c2
Author: guidou <guidou@chromium.org>
Date: Sat Jun 03 00:00:15 2017

Revert of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2918113002/ )

Reason for revert:
Does not properly fix the bug.

Original issue's description:
> Change default minimum screencast frame rate to zero.
>
> This is causing issues with refresh-interval computations that
> treat zero as a special value and expect zero as the default
> minimum, as it was with the old constraints algorithm.
>
> BUG= 725696 
>
> Review-Url: https://codereview.chromium.org/2918113002
> Cr-Commit-Position: refs/heads/master@{#476728}
> Committed: https://chromium.googlesource.com/chromium/src/+/9d5685faf6178778aaec42384e4c3f81890799b5

TBR=hbos@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 725696 

Review-Url: https://codereview.chromium.org/2919973003
Cr-Commit-Position: refs/heads/master@{#476836}

[modify] https://crrev.com/56ed98a48e61f1ba075a5352939155379f21b2c2/content/renderer/media/media_stream_constraints_util_video_content.cc
[modify] https://crrev.com/56ed98a48e61f1ba075a5352939155379f21b2c2/content/renderer/media/media_stream_constraints_util_video_content.h
[modify] https://crrev.com/56ed98a48e61f1ba075a5352939155379f21b2c2/content/renderer/media/media_stream_constraints_util_video_content_unittest.cc

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

commit 56ed98a48e61f1ba075a5352939155379f21b2c2
Author: guidou <guidou@chromium.org>
Date: Sat Jun 03 00:00:15 2017

Revert of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2918113002/ )

Reason for revert:
Does not properly fix the bug.

Original issue's description:
> Change default minimum screencast frame rate to zero.
>
> This is causing issues with refresh-interval computations that
> treat zero as a special value and expect zero as the default
> minimum, as it was with the old constraints algorithm.
>
> BUG= 725696 
>
> Review-Url: https://codereview.chromium.org/2918113002
> Cr-Commit-Position: refs/heads/master@{#476728}
> Committed: https://chromium.googlesource.com/chromium/src/+/9d5685faf6178778aaec42384e4c3f81890799b5

TBR=hbos@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 725696 

Review-Url: https://codereview.chromium.org/2919973003
Cr-Commit-Position: refs/heads/master@{#476836}

[modify] https://crrev.com/56ed98a48e61f1ba075a5352939155379f21b2c2/content/renderer/media/media_stream_constraints_util_video_content.cc
[modify] https://crrev.com/56ed98a48e61f1ba075a5352939155379f21b2c2/content/renderer/media/media_stream_constraints_util_video_content.h
[modify] https://crrev.com/56ed98a48e61f1ba075a5352939155379f21b2c2/content/renderer/media/media_stream_constraints_util_video_content_unittest.cc

Thanks miu@ and gidou@ for pitching in.

However, it's really not good that a regression that impacted multiple products made it this far.   I think a post mortem will be a good exercise once the dust settles.

The original  Bug 657733  seems to be a rewrite of the constraint processing algorithm for getUserMedia - but I don't see a web platform launch or Blink intent to implement in my inbox.  Normally that is where compatibility issues are brought to light when web platform behavior is changed.

Also we should figure out if there are gaps in our end to end testing (this was internally reported only a week ago, by developers not on our team).

I'll work with miu@ to set something up. 

miu@:  The new bug you describe was also in the old code. If you explicitly set a zero frame rate the default was used anyway since the code for setting the refresh interval ignored values <= 0.

This was the code:

      if (GetConstraintMinAsDouble(	
              constraints, &blink::WebMediaTrackConstraintSet::frameRate,	
              &value) &&	
          value > 0.0) {	
        refresh_interval =	 
            base::TimeDelta::FromMicroseconds(base::saturated_cast<int64_t>(	  
                base::Time::kMicrosecondsPerSecond / value));	 
      }

The behavior change occurred because the default minimum was changed from 0.0 to 1/60.0 (the old algorithm also ignored values less than 1/60.0 elsewhere).
So, the reverted patch did restore old behavior.

Nevertheless, I just sent for review https://codereview.chromium.org/2922013002/ which is a more complete patch with the behavior you propose.

I think we should reland the old patch, test it for a few days and merge it to 60 and 59 since it is a much simpler patch. Then land https://codereview.chromium.org/2922013002/ and merge it to 60 only.  WDYT?

mfoltz@: The postmortem sounds like a good idea. 

The Blink intent you are looking for is https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/MediaStreamTrack$20Constraints/blink-dev/ANta5sQXoGA/c38zjE0GBwAJ

 Bug 657733  covers part of that intent.

The changes introduced by  bug 657733  are nontrivial, so we added a flag to allow reverting to the old algorithm in case the new one broke too many things. So far, most bug reports have been due to the new algorithm selecting different default values for resolution and frame rate in video content capture.

This has affected a few applications relying on the default values for correctness (sometimes these are bugs in the applications because constraints don't have  default values and applications should expect any valid value if a property is not constrained). Nevertheless, until now all these bugs surfaced during the 59 canary/dev period right after the new algorithm was enabled, and were fixed by restoring default values to what they were in the old algorithm.

This is another bug caused by a change to a default value (in this case minimum frame rate). The difference is that it was another part of chrome instead of an application relying on it. I'm also surprised that it surfaced so late. I would have expected it to be caught earlier eiher by automatic or manual tests, which did happen in other similar cases (e.g.,  bug  709915  and bug 713231).

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

commit 14b29e4c08342d12e9eb607158f6d18cabf650c5
Author: guidou <guidou@chromium.org>
Date: Sat Jun 03 17:23:47 2017

Reland of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2919973003/ )

Reason for revert:
Will fix the patch before relanding.

Original issue's description:
> Revert of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2918113002/ )
>
> Reason for revert:
> Does not properly fix the bug.
>
> Original issue's description:
> > Change default minimum screencast frame rate to zero.
> >
> > This is causing issues with refresh-interval computations that
> > treat zero as a special value and expect zero as the default
> > minimum, as it was with the old constraints algorithm.
> >
> > BUG= 725696 
> >
> > Review-Url: https://codereview.chromium.org/2918113002
> > Cr-Commit-Position: refs/heads/master@{#476728}
> > Committed: https://chromium.googlesource.com/chromium/src/+/9d5685faf6178778aaec42384e4c3f81890799b5
>
> TBR=hbos@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 725696 
>
> Review-Url: https://codereview.chromium.org/2919973003
> Cr-Commit-Position: refs/heads/master@{#476836}
> Committed: https://chromium.googlesource.com/chromium/src/+/56ed98a48e61f1ba075a5352939155379f21b2c2

TBR=hbos@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 725696 

Review-Url: https://codereview.chromium.org/2921123002
Cr-Commit-Position: refs/heads/master@{#476899}

[modify] https://crrev.com/14b29e4c08342d12e9eb607158f6d18cabf650c5/content/renderer/media/media_stream_constraints_util_video_content.cc
[modify] https://crrev.com/14b29e4c08342d12e9eb607158f6d18cabf650c5/content/renderer/media/media_stream_constraints_util_video_content.h
[modify] https://crrev.com/14b29e4c08342d12e9eb607158f6d18cabf650c5/content/renderer/media/media_stream_constraints_util_video_content_unittest.cc

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

guido: Plan SGTM. :) Didn't realize the old behavior was broken too! We should probably add some layout tests (or browser tests?) to ensure the various behaviors around having (or not having) framerate constraints work as expected.

I'm perf sheriffing today, among other fire fights, so I'll have to take a look at the code review tomorrow.

miu@: No rush for the code review. Since we agree on the plan, the important thing is to merge the original fix to M60 and M59.

I filed  bug 729746  to handle the more complete fix in order to avoid confusion with the merge requests.

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

commit b20dec95b9faf5c28d4cf7c38311d71bf1ce111a
Author: Guido Urdaneta <guidou@chromium.org>
Date: Wed Jun 07 10:18:46 2017

Reland of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2919973003/ )

Reason for revert:
Will fix the patch before relanding.

Original issue's description:
> Revert of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2918113002/ )
>
> Reason for revert:
> Does not properly fix the bug.
>
> Original issue's description:
> > Change default minimum screencast frame rate to zero.
> >
> > This is causing issues with refresh-interval computations that
> > treat zero as a special value and expect zero as the default
> > minimum, as it was with the old constraints algorithm.
> >
> > BUG= 725696 
> >
> > Review-Url: https://codereview.chromium.org/2918113002
> > Cr-Commit-Position: refs/heads/master@{#476728}
> > Committed: https://chromium.googlesource.com/chromium/src/+/9d5685faf6178778aaec42384e4c3f81890799b5
>
> TBR=hbos@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 725696 
>
> Review-Url: https://codereview.chromium.org/2919973003
> Cr-Commit-Position: refs/heads/master@{#476836}
> Committed: https://chromium.googlesource.com/chromium/src/+/56ed98a48e61f1ba075a5352939155379f21b2c2

TBR=hbos@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 725696 

Review-Url: https://codereview.chromium.org/2921123002
Cr-Original-Commit-Position: refs/heads/master@{#476899}
Review-Url: https://codereview.chromium.org/2929573002 .
Cr-Commit-Position: refs/branch-heads/3112@{#218}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/b20dec95b9faf5c28d4cf7c38311d71bf1ce111a/content/renderer/media/media_stream_constraints_util_video_content.cc
[modify] https://crrev.com/b20dec95b9faf5c28d4cf7c38311d71bf1ce111a/content/renderer/media/media_stream_constraints_util_video_content.h
[modify] https://crrev.com/b20dec95b9faf5c28d4cf7c38311d71bf1ce111a/content/renderer/media/media_stream_constraints_util_video_content_unittest.cc

Closing as fixed. Will wait for a few days after the patch is in the next beta to request merge to 59.

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

commit 4b4922a6746be9095e7f22da4357a017f9b640bb
Author: Guido Urdaneta <guidou@chromium.org>
Date: Tue Jun 13 09:47:06 2017

Reland of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2919973003/ )

Reason for revert:
The original patch does fix the bug.

Original issue's description:
> Revert of Change default minimum screencast frame rate to zero. (patchset #1 id:1 of https://codereview.chromium.org/2918113002/ )
>
> Reason for revert:
> Does not properly fix the bug.
>
> Original issue's description:
> > Change default minimum screencast frame rate to zero.
> >
> > This is causing issues with refresh-interval computations that
> > treat zero as a special value and expect zero as the default
> > minimum, as it was with the old constraints algorithm.
> >
> > BUG= 725696 
> >
> > Review-Url: https://codereview.chromium.org/2918113002
> > Cr-Commit-Position: refs/heads/master@{#476728}
> > Committed: https://chromium.googlesource.com/chromium/src/+/9d5685faf6178778aaec42384e4c3f81890799b5
>
> TBR=hbos@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 725696 
>
> Review-Url: https://codereview.chromium.org/2919973003
> Cr-Commit-Position: refs/heads/master@{#476836}
> Committed: https://chromium.googlesource.com/chromium/src/+/56ed98a48e61f1ba075a5352939155379f21b2c2

TBR=hbos@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 725696 

Review-Url: https://codereview.chromium.org/2921123002
Cr-Original-Original-Commit-Position: refs/heads/master@{#476899}
Review-Url: https://codereview.chromium.org/2929573002 .
Cr-Original-Commit-Position: refs/branch-heads/3112@{#218}
Cr-Original-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
Review-Url: https://codereview.chromium.org/2935013002 .
Cr-Commit-Position: refs/branch-heads/3071@{#781}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/4b4922a6746be9095e7f22da4357a017f9b640bb/content/renderer/media/media_stream_constraints_util_video_content.cc
[modify] https://crrev.com/4b4922a6746be9095e7f22da4357a017f9b640bb/content/renderer/media/media_stream_constraints_util_video_content.h
[modify] https://crrev.com/4b4922a6746be9095e7f22da4357a017f9b640bb/content/renderer/media/media_stream_constraints_util_video_content_unittest.cc

Issue 733183 has been merged into this issue.

Verified this issue on Chrome OS Minnie with M59 59.0.3071.135 / 9460.76.0 stable and M60 60.0.3112.80 / 9592.71.0 stable