Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1153063002:
Initialize AVFoundation explicitly (Closed)
|
Created:
5 years, 7 months ago by tommi (sloooow) - chröme Modified:
5 years, 2 months ago Reviewers:
jam CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitialize AVFoundation explicitly instead of implicitly via IsAVFoundationSupported.
AVFoundation initialization is not thread safe and API calls can occur after initialization from arbitrary threads, so we need to do this step as part of browser init.
See bug for more details.
BUG=492064
Committed: https://crrev.com/354afdd334545b9dad76e6dcf16b1bbffba7bc0b
Cr-Commit-Position: refs/heads/master@{#331629}
Patch Set 1 #Patch Set 2 : Fix media_unittests #Patch Set 3 : Fix WebRTC content browser tests #Patch Set 4 : Revert added file #Patch Set 5 : Fix ppapi tests #
Total comments: 6
Patch Set 6 : Move initialization around #Patch Set 7 : Revert change in webrtc content browsertest #
Total comments: 4
Patch Set 8 : Add perf trace for InitializeAVFoundation and revert changes in browser_test_base.cc #
Messages
Total messages: 30 (3 generated)
Fix media_unittests
Fix WebRTC content browser tests
Revert added file
Fix ppapi tests
tommi@chromium.org changed reviewers: + jam@chromium.org
Hey John, Can you take a look please? If there's a better place/way to do this initialization I figured you would be the one to know about it.
https://codereview.chromium.org/1153063002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.mm (right): https://codereview.chromium.org/1153063002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.mm:126: AVFoundationGlue::InitializeAVFoundation(); i think we want this in src/content so that all content-based browsers get it? https://codereview.chromium.org/1153063002/diff/80001/content/test/ppapi/ppap... File content/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/1153063002/diff/80001/content/test/ppapi/ppap... content/test/ppapi/ppapi_test.cc:128: AVFoundationGlue::InitializeAVFoundation(); if we had this line in ContentBrowserTest::Setup, would we still need the change in the other ppapi_test.cc (chrome) and webrtc browser test file? https://codereview.chromium.org/1153063002/diff/80001/media/base/mac/avfounda... File media/base/mac/avfoundation_glue.h (right): https://codereview.chromium.org/1153063002/diff/80001/media/base/mac/avfounda... media/base/mac/avfoundation_glue.h:15: #if defined(__OBJC__) why are all these changes needed in this file? shouldn't __OBJC__ always be defined for mac?
https://codereview.chromium.org/1153063002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.mm (right): https://codereview.chromium.org/1153063002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.mm:126: AVFoundationGlue::InitializeAVFoundation(); On 2015/05/26 at 23:13:47, jam wrote: > i think we want this in src/content so that all content-based browsers get it? Yes, makes sense. Put it in BrowserMainLoop::PreCreateThreads? https://codereview.chromium.org/1153063002/diff/80001/content/test/ppapi/ppap... File content/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/1153063002/diff/80001/content/test/ppapi/ppap... content/test/ppapi/ppapi_test.cc:128: AVFoundationGlue::InitializeAVFoundation(); On 2015/05/26 at 23:13:47, jam wrote: > if we had this line in ContentBrowserTest::Setup, would we still need the change in the other ppapi_test.cc (chrome) and webrtc browser test file? Not sure, I'll investigate. I tried to limit the scope of the change to only the tests that explicitly needed it but maybe it makes more sense to go up to a higher level such as ContentBrowserTest. https://codereview.chromium.org/1153063002/diff/80001/media/base/mac/avfounda... File media/base/mac/avfoundation_glue.h (right): https://codereview.chromium.org/1153063002/diff/80001/media/base/mac/avfounda... media/base/mac/avfoundation_glue.h:15: #if defined(__OBJC__) On 2015/05/26 23:13:47, jam wrote: > why are all these changes needed in this file? shouldn't __OBJC__ always be > defined for mac? It's only defined when building .mm files, not when building .cc. This essentially just allows us to call non-ObjC functions declared in this file (InitializeAVFoundation) from C++ code. Here's another example: https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/mac_util....
Move initialization around
Revert change in webrtc content browsertest
lgtm https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... content/browser/browser_main_loop.cc:656: AVFoundationGlue::InitializeAVFoundation(); btw do you have timings on how long this takes to do? https://codereview.chromium.org/1153063002/diff/120001/content/public/test/br... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/1153063002/diff/120001/content/public/test/br... content/public/test/browser_test_base.cc:230: // Initialize AVFoundation if supported, for audio and video. I just remembered that content's browser's initialization runs the same way in browser tests, so you can revert changes to this file
Add perf trace for InitializeAVFoundation and revert changes in browser_test_base.cc
https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... content/browser/browser_main_loop.cc:656: AVFoundationGlue::InitializeAVFoundation(); On 2015/05/27 14:54:37, jam wrote: > btw do you have timings on how long this takes to do? Added perf trace. I'm building right now on my laptop (might take a while), will run tests and report back. https://codereview.chromium.org/1153063002/diff/120001/content/public/test/br... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/1153063002/diff/120001/content/public/test/br... content/public/test/browser_test_base.cc:230: // Initialize AVFoundation if supported, for audio and video. On 2015/05/27 14:54:37, jam wrote: > I just remembered that content's browser's initialization runs the same way in > browser tests, so you can revert changes to this file Thanks. Reverted.
On 2015/05/27 at 15:54:54, tommi wrote: > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > File content/browser/browser_main_loop.cc (right): > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > content/browser/browser_main_loop.cc:656: AVFoundationGlue::InitializeAVFoundation(); > On 2015/05/27 14:54:37, jam wrote: > > btw do you have timings on how long this takes to do? > > Added perf trace. I'm building right now on my laptop (might take a while), will run tests and report back. OK, I used --trace-startup --trace-startup-duration=5 to do the measurement with the current patch on my mbpro eng laptop. A cold start seems to take about 10ms and subsequent (warm) runs take about 2ms. > > https://codereview.chromium.org/1153063002/diff/120001/content/public/test/br... > File content/public/test/browser_test_base.cc (right): > > https://codereview.chromium.org/1153063002/diff/120001/content/public/test/br... > content/public/test/browser_test_base.cc:230: // Initialize AVFoundation if supported, for audio and video. > On 2015/05/27 14:54:37, jam wrote: > > I just remembered that content's browser's initialization runs the same way in > > browser tests, so you can revert changes to this file > > Thanks. Reverted.
The CQ bit was checked by tommi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/1153063002/#ps140001 (title: "Add perf trace for InitializeAVFoundation and revert changes in browser_test_base.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153063002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/354afdd334545b9dad76e6dcf16b1bbffba7bc0b Cr-Commit-Position: refs/heads/master@{#331629}
Message was sent while issue was closed.
On 2015/05/27 19:52:56, tommi wrote: > On 2015/05/27 at 15:54:54, tommi wrote: > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > File content/browser/browser_main_loop.cc (right): > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > content/browser/browser_main_loop.cc:656: > AVFoundationGlue::InitializeAVFoundation(); > > On 2015/05/27 14:54:37, jam wrote: > > > btw do you have timings on how long this takes to do? > > > > Added perf trace. I'm building right now on my laptop (might take a while), > will run tests and report back. > > OK, I used --trace-startup --trace-startup-duration=5 to do the measurement with > the current patch on my mbpro eng laptop. > A cold start seems to take about 10ms and subsequent (warm) runs take about 2ms. > A 10ms cost at cold startup seems like something we should avoid (i.e. death by a thousand cuts). Is there no way to ensure that it's initialized on the UI thread before first use? i.e. any chokepoints on objects that live on UI thread?
Message was sent while issue was closed.
On 2015/05/27 at 20:07:35, jabdelmalek wrote: > On 2015/05/27 19:52:56, tommi wrote: > > On 2015/05/27 at 15:54:54, tommi wrote: > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > > File content/browser/browser_main_loop.cc (right): > > > > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > > content/browser/browser_main_loop.cc:656: > > AVFoundationGlue::InitializeAVFoundation(); > > > On 2015/05/27 14:54:37, jam wrote: > > > > btw do you have timings on how long this takes to do? > > > > > > Added perf trace. I'm building right now on my laptop (might take a while), > > will run tests and report back. > > > > OK, I used --trace-startup --trace-startup-duration=5 to do the measurement with > > the current patch on my mbpro eng laptop. > > A cold start seems to take about 10ms and subsequent (warm) runs take about 2ms. > > > > A 10ms cost at cold startup seems like something we should avoid (i.e. death by a thousand cuts). > > Is there no way to ensure that it's initialized on the UI thread before first use? i.e. any chokepoints on objects that live on UI thread? There is a way actually. Objective C supports a way to execute a "block" (similar to a closure) on the UI thread. I was thinking about doing a follow up cl that would allow content/ to implement a callback that would help media/ do on demand initialization on the main thread (since media/ doesn't know about browser threads). But then I remembered this feature of Objective C. I'll whip up a patch to see if it works and if so, I can revert this one and land it instead. Btw, definitely agree on the startup cost.
Message was sent while issue was closed.
On 2015/05/27 20:23:49, tommi wrote: > On 2015/05/27 at 20:07:35, jabdelmalek wrote: > > On 2015/05/27 19:52:56, tommi wrote: > > > On 2015/05/27 at 15:54:54, tommi wrote: > > > > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > > > File content/browser/browser_main_loop.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > > > content/browser/browser_main_loop.cc:656: > > > AVFoundationGlue::InitializeAVFoundation(); > > > > On 2015/05/27 14:54:37, jam wrote: > > > > > btw do you have timings on how long this takes to do? > > > > > > > > Added perf trace. I'm building right now on my laptop (might take a > while), > > > will run tests and report back. > > > > > > OK, I used --trace-startup --trace-startup-duration=5 to do the measurement > with > > > the current patch on my mbpro eng laptop. > > > A cold start seems to take about 10ms and subsequent (warm) runs take about > 2ms. > > > > > > > A 10ms cost at cold startup seems like something we should avoid (i.e. death > by a thousand cuts). > > > > Is there no way to ensure that it's initialized on the UI thread before first > use? i.e. any chokepoints on objects that live on UI thread? > > There is a way actually. Objective C supports a way to execute a "block" > (similar to a closure) on the UI thread. > > I was thinking about doing a follow up cl that would allow content/ to implement > a callback that would help media/ do on demand initialization on the main thread > (since media/ doesn't know about browser threads). But then I remembered this > feature of Objective C. I'll whip up a patch to see if it works and if so, I > can revert this one and land it instead. Btw, definitely agree on the startup > cost. Thanks. I was wondering though, since content is what calls media, is there no place in content that can know that it'll need to initialize this on the UI thread? i.e. a media WebContentsObserver, an IPC dispatched in a message filter etc... then it can initialize this class there.
Message was sent while issue was closed.
At one time at least, that was the idea. The DeviceMonitor on mac triggered this initialization when constructed on the UI thread. However, as you can see from the data in the bug, that was racy. Somewhat recently, the DeviceMonitor was changed so that it delayed this initialization and that doubled the amount of times we hit that race condition. The race is with code that runs on the device thread, accessing devices etc. That code is independent of the monitor, runs on other threads etc, so at the moment I'm at least not aware of a reliable place we could wire initialization like that up. On Wed, May 27, 2015 at 10:30 PM <jabdelmalek@google.com> wrote: > On 2015/05/27 20:23:49, tommi wrote: > > On 2015/05/27 at 20:07:35, jabdelmalek wrote: > > > On 2015/05/27 19:52:56, tommi wrote: > > > > On 2015/05/27 at 15:54:54, tommi wrote: > > > > > > > > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > > > > File content/browser/browser_main_loop.cc (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... > > > > > content/browser/browser_main_loop.cc:656: > > > > AVFoundationGlue::InitializeAVFoundation(); > > > > > On 2015/05/27 14:54:37, jam wrote: > > > > > > btw do you have timings on how long this takes to do? > > > > > > > > > > Added perf trace. I'm building right now on my laptop (might take a > > while), > > > > will run tests and report back. > > > > > > > > OK, I used --trace-startup --trace-startup-duration=5 to do the > measurement > > with > > > > the current patch on my mbpro eng laptop. > > > > A cold start seems to take about 10ms and subsequent (warm) runs take > about > > 2ms. > > > > > > > > > > A 10ms cost at cold startup seems like something we should avoid (i.e. > > death > > by a thousand cuts). > > > > > > Is there no way to ensure that it's initialized on the UI thread before > first > > use? i.e. any chokepoints on objects that live on UI thread? > > > There is a way actually. Objective C supports a way to execute a "block" > > (similar to a closure) on the UI thread. > > > I was thinking about doing a follow up cl that would allow content/ to > implement > > a callback that would help media/ do on demand initialization on the main > thread > > (since media/ doesn't know about browser threads). But then I remembered > > this > > feature of Objective C. I'll whip up a patch to see if it works and if > > so, I > > can revert this one and land it instead. Btw, definitely agree on the > > startup > > cost. > > Thanks. > > I was wondering though, since content is what calls media, is there no > place in > content that can know that it'll need to initialize this on the UI thread? > i.e. > a media WebContentsObserver, an IPC dispatched in a message filter etc... > then > it can initialize this class there. > > https://codereview.chromium.org/1153063002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Wed, May 27, 2015 at 1:50 PM, Tommi <tommi@chromium.org> wrote: > At one time at least, that was the idea. The DeviceMonitor on mac > triggered this initialization when constructed on the UI thread. However, > as you can see from the data in the bug, that was racy. Somewhat recently, > the DeviceMonitor was changed so that it delayed this initialization and > that doubled the amount of times we hit that race condition. The race is > with code that runs on the device thread, accessing devices etc. That code > is independent of the monitor, runs on other threads etc, > what triggers this code? shouldn't it eventually lead to something on the blink side? i.e. specifically an IPC from renderer to browser? > so at the moment I'm at least not aware of a reliable place we could wire > initialization like that up. > > On Wed, May 27, 2015 at 10:30 PM <jabdelmalek@google.com> wrote: > >> On 2015/05/27 20:23:49, tommi wrote: >> > On 2015/05/27 at 20:07:35, jabdelmalek wrote: >> > > On 2015/05/27 19:52:56, tommi wrote: >> > > > On 2015/05/27 at 15:54:54, tommi wrote: >> > > > > >> > > > >> >> >> https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... >> > > > > File content/browser/browser_main_loop.cc (right): >> > > > > >> > > > > >> > > > >> >> >> https://codereview.chromium.org/1153063002/diff/120001/content/browser/browse... >> > > > > content/browser/browser_main_loop.cc:656: >> > > > AVFoundationGlue::InitializeAVFoundation(); >> > > > > On 2015/05/27 14:54:37, jam wrote: >> > > > > > btw do you have timings on how long this takes to do? >> > > > > >> > > > > Added perf trace. I'm building right now on my laptop (might take >> a >> > while), >> > > > will run tests and report back. >> > > > >> > > > OK, I used --trace-startup --trace-startup-duration=5 to do the >> measurement >> > with >> > > > the current patch on my mbpro eng laptop. >> > > > A cold start seems to take about 10ms and subsequent (warm) runs >> take >> about >> > 2ms. >> > > > >> > > >> > > A 10ms cost at cold startup seems like something we should avoid (i.e. >> > death >> > by a thousand cuts). >> > > >> > > Is there no way to ensure that it's initialized on the UI thread >> before >> first >> > use? i.e. any chokepoints on objects that live on UI thread? >> >> > There is a way actually. Objective C supports a way to execute a >> "block" >> > (similar to a closure) on the UI thread. >> >> > I was thinking about doing a follow up cl that would allow content/ to >> implement >> > a callback that would help media/ do on demand initialization on the >> main >> thread >> > (since media/ doesn't know about browser threads). But then I >> remembered >> > this >> > feature of Objective C. I'll whip up a patch to see if it works and if >> > so, I >> > can revert this one and land it instead. Btw, definitely agree on the >> > startup >> > cost. >> >> Thanks. >> >> I was wondering though, since content is what calls media, is there no >> place in >> content that can know that it'll need to initialize this on the UI thread? >> i.e. >> a media WebContentsObserver, an IPC dispatched in a message filter etc... >> then >> it can initialize this class there. >> >> https://codereview.chromium.org/1153063002/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/05/27 at 21:10:42, jam wrote: > On Wed, May 27, 2015 at 1:50 PM, Tommi <tommi@chromium.org> wrote: > > > At one time at least, that was the idea. The DeviceMonitor on mac > > triggered this initialization when constructed on the UI thread. However, > > as you can see from the data in the bug, that was racy. Somewhat recently, > > the DeviceMonitor was changed so that it delayed this initialization and > > that doubled the amount of times we hit that race condition. The race is > > with code that runs on the device thread, accessing devices etc. That code > > is independent of the monitor, runs on other threads etc, > > > > what triggers this code? shouldn't it eventually lead to something on the > blink side? i.e. specifically an IPC from renderer to browser? > There are three things: device access, enumeration and monitoring. If you take a look at code search now and look for call sites for IsAVFoundationSupported, you'll see where initialization could occur. These calls are triggered by calls from blink such as getUserMedia and enumerateDevices. It's possible that it could be triggered by other things such as extensions (I seem to remember ppapi doing device enumeration and possibly some other extensions... hotwords?)
Message was sent while issue was closed.
On Wed, May 27, 2015 at 2:31 PM, <tommi@chromium.org> wrote: > On 2015/05/27 at 21:10:42, jam wrote: > >> On Wed, May 27, 2015 at 1:50 PM, Tommi <tommi@chromium.org> wrote: >> > > > At one time at least, that was the idea. The DeviceMonitor on mac >> > triggered this initialization when constructed on the UI thread. >> However, >> > as you can see from the data in the bug, that was racy. Somewhat >> recently, >> > the DeviceMonitor was changed so that it delayed this initialization and >> > that doubled the amount of times we hit that race condition. The race >> is >> > with code that runs on the device thread, accessing devices etc. That >> code >> > is independent of the monitor, runs on other threads etc, >> > >> > > what triggers this code? shouldn't it eventually lead to something on the >> blink side? i.e. specifically an IPC from renderer to browser? >> > > > There are three things: device access, enumeration and monitoring. > If you take a look at code search now and look for call sites for > IsAVFoundationSupported, you'll see where initialization could occur. > These > calls are triggered by calls from blink such as getUserMedia and > enumerateDevices. ok, so can we trap these IPCs on the UI thread first and initialize AVF there? ditto for extensions > It's possible that it could be triggered by other things such > as extensions (I seem to remember ppapi doing device enumeration and > possibly > some other extensions... hotwords?) > > https://codereview.chromium.org/1153063002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Yes we should be able to do that. I'll see if I can track down where those places are and hopefully find a single one and then move the call out from PreCreateThreads. On Thu, May 28, 2015 at 12:29 AM John Abd-El-Malek <jam@chromium.org> wrote: > On Wed, May 27, 2015 at 2:31 PM, <tommi@chromium.org> wrote: > >> On 2015/05/27 at 21:10:42, jam wrote: >> >>> On Wed, May 27, 2015 at 1:50 PM, Tommi <tommi@chromium.org> wrote: >>> >> >> > At one time at least, that was the idea. The DeviceMonitor on mac >>> > triggered this initialization when constructed on the UI thread. >>> However, >>> > as you can see from the data in the bug, that was racy. Somewhat >>> recently, >>> > the DeviceMonitor was changed so that it delayed this initialization >>> and >>> > that doubled the amount of times we hit that race condition. The race >>> is >>> > with code that runs on the device thread, accessing devices etc. That >>> code >>> > is independent of the monitor, runs on other threads etc, >>> > >>> >> >> what triggers this code? shouldn't it eventually lead to something on the >>> blink side? i.e. specifically an IPC from renderer to browser? >>> >> >> >> There are three things: device access, enumeration and monitoring. >> If you take a look at code search now and look for call sites for >> IsAVFoundationSupported, you'll see where initialization could occur. >> These >> calls are triggered by calls from blink such as getUserMedia and >> enumerateDevices. > > > ok, so can we trap these IPCs on the UI thread first and initialize AVF > there? > ditto for extensions > > >> It's possible that it could be triggered by other things such >> as extensions (I seem to remember ppapi doing device enumeration and >> possibly >> some other extensions... hotwords?) >> >> https://codereview.chromium.org/1153063002/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/05/28 06:44:17, tommi (away) wrote: > Yes we should be able to do that. I'll see if I can track down where those > places are and hopefully find a single one and then move the call out from > PreCreateThreads. btw has there been a fix for this?
Message was sent while issue was closed.
On 2015/07/24 17:03:30, jam wrote: > On 2015/05/28 06:44:17, tommi (away) wrote: > > Yes we should be able to do that. I'll see if I can track down where those > > places are and hopefully find a single one and then move the call out from > > PreCreateThreads. > > btw has there been a fix for this? ping
Message was sent while issue was closed.
On 2015/09/16 20:11:57, jam wrote: > On 2015/07/24 17:03:30, jam wrote: > > On 2015/05/28 06:44:17, tommi (away) wrote: > > > Yes we should be able to do that. I'll see if I can track down where those > > > places are and hopefully find a single one and then move the call out from > > > PreCreateThreads. > > > > btw has there been a fix for this? > > ping http://crbug.com/541679 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
