All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=825790

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=d9ccf1fa0b2f9e04ad6b7a2059a930818e1b9156bc40de16726d3dc718d0eaf0


Bot(s) for this bug's original alert(s):

webrtc-win-large-tests

The graph labels doesn't say that larger values are better, but I guess they are? Then this is an improvement, but I can't find any likely cl for the change.

Graph looks stuck at 14.5, so maybe the test isn't working as intended?

Looks like an improvement, and it doesn't seem stuck, just more stable. Could be:
https://webrtc.googlesource.com/src/+/bb60a3a5fa999d0dcf12319068bb6ea01f4a64c3

Erik, could it be that this change made freezes less likely?

I suggest we close either way, but would be good to know

I really don't know why this would have any effect here. But there is also a corresponding reduction in time between rendered frames:
https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQqrXqhAgM

Linking that graph to this bug. And there's also

https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICQypb9oQsM

showing "sender_time" getting larger and more noisy.

Are the tests "screenshare_slides_lossy_limited_ALR" using VP8 temporal layers? Then the cl refactoring this code seems like a probable cause.

This doesn't make any sense. None of the other platforms are affected, and no other screenshare test with loss is affected.

+ilnik to help me repro on mac

I think this mac bot is running on a virtual machine or something, because the performance is in general awful.

Note that the other two graphs were from a windows bot, webrtc-win-large-tests.

I was able to repro regression on macbook air. However, if you look at the graph in the past, you will see, that there already was a period with the same sender_time metrics. It went worse and then back with no relevant commits in the vicinity of regression/improvement. I think it's some kind of uncontrollable architecture side effect, like executable size, memory locations and so on. The tests are very CPU intensive so they may be affected by that kind of things.

I added a few more alerts which seem to be at the same change. Multiple platforms.

OK, I think I have found what's changed: I accidentally fixed a race condition we didn't know we had.

In ScreenshareLayers::OnRatesUpdated(), we check if either the bitrate or framerate setting has changed, and if so the boolean |bitrate_updated_| to true.
On the next frame, ScreenshareLayers::UpdateConfiguration() will update the Vp8EncoderConfig only if bitrate_updated_ is true (and then reset it).

Previously, OnRatesUpdated() was called implicitly by SimulcastRateAllocator::GetAllocation(), which happens in VideoSender::UpdateEncoderParameters(). This can happen either as a response to a new bitrate stimate (VideoStreamEncoder::OnBitrateUpdated()) or as a periodic refresh (poll for new frame rate in VideoStreamEncoder::EncodeVideoFrame()).

Previously, the following could occur:
* A new bitrate is calculated
* VideoStreamEncoder::OnBitrateUpdated() is called, which indirectly calls ScreenshareLayers::OnRatesUpdated().
* The new rates are stored in ScreenshareLayers, and |bitrate_updated_| is set to true.
* A new frame arrives, and VideoStreamEncoder::EncodeVideoFrame() is called.
* The time since last periodic refresh has elapsed, so VideoSender::UpdateEncoderParameters() is called with the last available bitrate.
* The measured input frame rate remains unchanged.
* VideoStreamEncoder::OnBitrateUpdated() is called, which indirectly calls ScreenshareLayers::OnRatesUpdated().
* The new rates are checked in ScreenshareLayers:OnRatesUpdated(). Since they are unchanged, |bitrate_updated_| is set to false.
* The new frame is encoded, but the encoder is not reconfigured with the new target rates as  |bitrate_updated_| is set to false.

So now we're reacting more quickly to changes in bitrate estimate, which it seems is good in some cases but not as much in others?

In any case I think this new behavior is more correct, so I'll close this as working as intended.

Excellent, thanks for investigating.

It sounds to me like the |bitrate_updated_| flag should only ever be set to false by ScreenshareLayers::UpdateConfiguration(), is that how the current code works?