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

Issue 762042 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

223.1% regression in content_browsertests at 499336:499354

Project Member Reported by niklase@chromium.org, Sep 5 2017

Issue description

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

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


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

chromium-webrtc-trunk-tot-rel-linux
I believe this test [1] measures the average duration of executing the method VideoCaptureDeviceClient::OnIncomingCapturedData() [2] over the course of a 10s video capture example run.

The reason why it takes longer after the suspected CL is probably because it now maps and unmaps the shared memory for each frame, while before the CL the shared memory was mapped only once when the buffer pool created the buffer.

miu@: Do you think we should consider adding functionality to allow clients to tell the buffer pool to "pin" the memory mapping for cases where we know that we are going to request in-process access over and over again?

[1] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_getusermedia_browsertest.cc?type=cs&l=662
[2] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_client.cc?l=143

Comment 3 by m...@chromium.org, Sep 5 2017

Status: Started (was: Assigned)
The regression is 150 microseconds, and is very likely being caused by the extra syscalls around mapping and unmapping for each frame...But, this does not seem like much of a regression...? It would add 150 microseconds to the latency in the video capture pipeline, but this is insignificant compared to downstream parts of the pipeline like video encoding and network transmission (20,000 to 500,000 microseconds).

However, there's a perf test measuring this, and so I feel like I'm missing something important. :) Since I don't have the time to investigate or monitor any other possible related fall-out from this, I'll go ahead and put together a patch to fix this regression: The "lazy map once" scheme--what the code in services/video_capture/public/cpp/receiver_media_to_mojo_adapter.cc did before--would work just fine for what I needed and it would be a fairly simple change that restores the "mmap once for the long term" behavior.

Comment 5 by m...@chromium.org, Sep 6 2017

Cc: briander...@chromium.org m...@chromium.org
 Issue 762229  has been merged into this issue.

Comment 6 by m...@chromium.org, Sep 6 2017

 Issue 762255  has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 6 2017

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

commit 121c12240e81599bcbcea88a87da740c641d69f2
Author: Yuri Wiitala <miu@chromium.org>
Date: Wed Sep 06 21:33:36 2017

Video Capture SharedMemoryHandleProvider: mmap() once, lazy

Fixes the performance regression caused by recent re-workings of the way
shared memory is managed in the Video Capture stack. Instead of only
mapping shared memory while it is being mutated (i.e., mapped and
unmapped for each video frame produced), this change restores the prior
behavior of mapping upon first use and then only unmapping way later,
when the buffer pool is destroyed.

Bug:  762042 
Change-Id: I18775d02696a24863dae3f21932c544cc10c2432
Reviewed-on: https://chromium-review.googlesource.com/651506
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500096}
[modify] https://crrev.com/121c12240e81599bcbcea88a87da740c641d69f2/media/capture/video/shared_memory_handle_provider.cc
[modify] https://crrev.com/121c12240e81599bcbcea88a87da740c641d69f2/media/capture/video/shared_memory_handle_provider.h

Comment 8 by m...@chromium.org, Sep 21 2017

Status: Fixed (was: Started)

Sign in to add a comment