New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 910576 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

CPU overuse caused by WebRTC event logs

Project Member Reported by eladalon@chromium.org, Nov 30

Issue description

WebRTC events are emitted to Chrome about 1000 times per second, instead of once every 5 seconds. This causes:
1. CPU overuse.
2. The logs are not compressed as well, since each event is compressed by itself, rather than 5000 being compressed together each time.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 30

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

commit de9c922fc0c4954feada059fdd8d4c3f12fbd37e
Author: Elad Alon <eladalon@chromium.org>
Date: Fri Nov 30 16:04:33 2018

Fix CPU overuse caused by WebRTC event logs not being batched

Use batches of 5 seconds instead of immediate log emission.
(An upcoming CL should make this configurable from JS.)

Bug:  910576 
Change-Id: Ib2b804e1e8c7798b744548fb343776fa8a315a65
Reviewed-on: https://chromium-review.googlesource.com/c/1356800
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Elad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612663}
[modify] https://crrev.com/de9c922fc0c4954feada059fdd8d4c3f12fbd37e/content/renderer/media/webrtc/rtc_peer_connection_handler.cc

Labels: Merge-Request-71
We need this merged into M71. This is a two-line change, replacing 0 by 5000, and is quite safe. It will prevent CPU overuse. (The value was intended to be set to 5000, but miscommunication has led two people to each believe that the other has already changed it.)
Cc: guidou@chromium.org
Project Member

Comment 4 by sheriffbot@chromium.org, Nov 30

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: We are only 3 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
We already cut M71 stable RC for next week release on Tuesday.

Change listed at #1 is not yet baked in canary, we can't take this merge in for M71 without canary coverage this late in release cycle. 

Is this merge super critical for M71 initial stable release? Can we pick it up for next M71 respin (if any)?
Stable RC is already cut for Desktop.
This feature is behind three gates, all remotely controlled; each one of them being closed would serve as a kill-switch for the feature:
1. The user must configure a specific Chrome policy, which, for now, is only configured by Googlers.
2. The user must visit Google Meet.
3. Google Meet must serve the user a page with JS code that tries to access the feature.
Therefore, I believe this merge to be of very low risk.
Labels: -Merge-Review-71 Merge-Rejected-71
Rejecting merge to M71 as change is not yet baked/verified in canary/dev per offline group chat with eladalon@ and guidou@.
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 5

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30600f99be877a4f7cf9850f407fb8b63211b4da

commit 30600f99be877a4f7cf9850f407fb8b63211b4da
Author: Elad Alon <eladalon@chromium.org>
Date: Wed Dec 05 10:52:41 2018

Fix CPU overuse caused by WebRTC event logs not being batched

Use batches of 5 seconds instead of immediate log emission.
(An upcoming CL should make this configurable from JS.)

Bug:  910576 ,  911514 
Change-Id: Ib2b804e1e8c7798b744548fb343776fa8a315a65
Reviewed-on: https://chromium-review.googlesource.com/c/1356800
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Elad Alon <eladalon@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612663}(cherry picked from commit de9c922fc0c4954feada059fdd8d4c3f12fbd37e)
Reviewed-on: https://chromium-review.googlesource.com/c/1362898
Reviewed-by: Elad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#71}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/30600f99be877a4f7cf9850f407fb8b63211b4da/content/renderer/media/webrtc/rtc_peer_connection_handler.cc

Status: Fixed (was: Started)
Note that merge approval to 72 is available in  bug 911514 .
Closing this bug as fixed.
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/30600f99be877a4f7cf9850f407fb8b63211b4da

Commit: 30600f99be877a4f7cf9850f407fb8b63211b4da
Author: eladalon@chromium.org
Commiter: eladalon@chromium.org
Date: 2018-12-05 10:52:41 +0000 UTC

Fix CPU overuse caused by WebRTC event logs not being batched

Use batches of 5 seconds instead of immediate log emission.
(An upcoming CL should make this configurable from JS.)

Bug:  910576 ,  911514 
Change-Id: Ib2b804e1e8c7798b744548fb343776fa8a315a65
Reviewed-on: https://chromium-review.googlesource.com/c/1356800
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Elad Alon <eladalon@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612663}(cherry picked from commit de9c922fc0c4954feada059fdd8d4c3f12fbd37e)
Reviewed-on: https://chromium-review.googlesource.com/c/1362898
Reviewed-by: Elad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#71}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Sign in to add a comment