|
|
Created:
4 years, 10 months ago by torbjorng (webrtc) Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, andresp, the sun, pbos-webrtc, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionExperimental patch for adapting adaptation to CPU count on Mac.
BUG=
Committed: https://crrev.com/448468d575725f67de4eedb3519b46e2a2e9374f
Cr-Commit-Position: refs/heads/master@{#11553}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix typo in comment #Patch Set 3 : Address feedback from tommi #
Total comments: 13
Patch Set 4 : Rewrite changes #Patch Set 5 : Move include to .cc file #
Total comments: 4
Patch Set 6 : Address feedback #Patch Set 7 : Rewrite again. Use 1/4 of a physical core as goal. #Messages
Total messages: 30 (8 generated)
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
Thanks for uploading Torbjorn. This looks very interesting to me, so much actually that I wonder if we should give this a try for a few days via a few canaries? WDYT? Assuming that might be a possibility I left in a few minor comments. https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:45: : low_encode_usage_threshold_percent(42), Is 42 something that should be set for all platforms or only for mac? https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:50: #if WEBRTC_MAC nit: #if defined(WEBRTC_MAC) https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:58: // FIXME(torbjorng): Add syscall error checking, fallback code. nit: s/FIXME/TODO https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); does this work as expected inside the sandbox? https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:71: = 33 * low_encode_usage_threshold_percent / 16 + 1; nit: assignment operator on previous line (could also run git cl format)
Thanks, issues addressed. https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:45: : low_encode_usage_threshold_percent(42), On 2016/02/04 16:40:02, tommi-webrtc wrote: > Is 42 something that should be set for all platforms or only for mac? I decreased this to make it about 85/2; the interval should really reflect the scaling factors made available by libyuv (or more accurately, these factors squared!). Else we will presumably tend to oscillate. I deny that I pulled 42 out of a hat; it is the result of a huge computation. https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:50: #if WEBRTC_MAC On 2016/02/04 16:40:02, tommi-webrtc wrote: > nit: #if defined(WEBRTC_MAC) Done. https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:58: // FIXME(torbjorng): Add syscall error checking, fallback code. On 2016/02/04 16:40:02, tommi-webrtc wrote: > nit: s/FIXME/TODO Done. https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); On 2016/02/04 16:40:02, tommi-webrtc wrote: > does this work as expected inside the sandbox? Good question. I will verify that. I observed chrome://webrtc-internals, and it looked as it was effective. But timing reproducibility could as we know be better on this platform. We should use SystemInfo (base/systeminfo.h) but I did not because I couldn't get *physical* core count from it. https://codereview.webrtc.org/1665173002/diff/1/webrtc/video/overuse_frame_de... webrtc/video/overuse_frame_detector.h:71: = 33 * low_encode_usage_threshold_percent / 16 + 1; On 2016/02/04 16:40:02, tommi-webrtc wrote: > nit: assignment operator on previous line (could also run git cl format) Done.
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665173002/40001
more nits :) I ran the cq dryrun just in case https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:50: #if defined (WEBRTC_MAC) #if defined(WEBRTC_MAC) https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); Can you change the cast to a C++ cast? As is it looks like the cast is casting a pointer to an object... does host_info() accept a pointer or a by value? (is host_info_t a pointer type?) https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:68: #endif nit: #endif // WEBRTC_MAC https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:71: 33 * low_encode_usage_threshold_percent / 16 + 1; 4 space indent. (or just run git cl format?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/1665173002/diff/40001/webrtc/video/overuse_fr... File webrtc/video/overuse_frame_detector.h (right): https://codereview.chromium.org/1665173002/diff/40001/webrtc/video/overuse_fr... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); The result of mach_host_self must be deallocated. This is currently leaking a system resource.
tommi@chromium.org changed reviewers: + tommi@chromium.org
https://codereview.chromium.org/1665173002/diff/40001/webrtc/video/overuse_fr... File webrtc/video/overuse_frame_detector.h (right): https://codereview.chromium.org/1665173002/diff/40001/webrtc/video/overuse_fr... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); On 2016/02/04 22:37:15, Robert Sesek wrote: > The result of mach_host_self must be deallocated. This is currently leaking a > system resource. Ah, thanks for catching that! Now this makes more sense to me. I'd prefer to have the variable of the expected type too and only cast after error checking (and of course free too).
mflodman@webrtc.org changed reviewers: + asapersson@webrtc.org
Adding Åsa to the loop, since she has spent many hours looking at this.
This is less experimental and leaks less resources than the original CL. I moved the meat of the constructor to the .cc file and added logging to make sure things work as they should. https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:50: #if defined (WEBRTC_MAC) On 2016/02/04 20:32:44, tommi-webrtc wrote: > #if defined(WEBRTC_MAC) Done. https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); On 2016/02/04 20:32:44, tommi-webrtc wrote: > Can you change the cast to a C++ cast? As is it looks like the cast is casting > a pointer to an object... does host_info() accept a pointer or a by value? (is > host_info_t a pointer type?) Yes, host_info_t is a ptr (it is in fact int*). Fixing cast. https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); On 2016/02/04 22:37:15, Robert Sesek wrote: > The result of mach_host_self must be deallocated. This is currently leaking a > system resource. Robert: I changed the code to look more like in other places in WebRTC. This means calling mach_port_deallocate with mach_task_self(). Since the latter just like mach_host_self() returns a magic port number (an int) one might suspect that the deallocation could leak. I cannot find proper documentation, but IIUC, these port numbers are (small) refcounted kernel resources. Tommi: This is a message passing API. You are supposed to cast things like this to the generic message type. https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:68: #endif On 2016/02/04 20:32:44, tommi-webrtc wrote: > nit: > #endif // WEBRTC_MAC Done. https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:71: 33 * low_encode_usage_threshold_percent / 16 + 1; On 2016/02/04 20:32:44, tommi-webrtc wrote: > 4 space indent. (or just run git cl format?) Done.
lg - I think we should get Asa's feedback on this. https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/1665173002/diff/40001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); On 2016/02/05 16:31:05, torbjorng (webrtc) wrote: > On 2016/02/04 22:37:15, Robert Sesek wrote: > > The result of mach_host_self must be deallocated. This is currently leaking a > > system resource. > > Robert: I changed the code to look more like in other places in WebRTC. This > means calling mach_port_deallocate with mach_task_self(). Since the latter just > like mach_host_self() returns a magic port number (an int) one might suspect > that the deallocation could leak. I cannot find proper documentation, but IIUC, > these port numbers are (small) refcounted kernel resources. > > Tommi: This is a message passing API. You are supposed to cast things like this > to the generic message type. Thanks, I'm familiar with APIs like these :) I was thinking along these lines: host_info_t host_info = nullptr; host_info(mach_host_self(), HOST_BASIC_INFO, &host_info, &datasize); if (data_size == sizeof(host_basic_info)) { struct host_basic_info* basic_info = reintepret_cast<host_basic_info*>(host_info); // use |basic_info| } else { NOTREACHED() << "host_info returned something unexpected."; } However, what you have now is fine too. https://codereview.webrtc.org/1665173002/diff/80001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.cc (right): https://codereview.webrtc.org/1665173002/diff/80001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:66: host_basic_info hbi; zero initialize host_basic_info hbi = {}; https://codereview.webrtc.org/1665173002/diff/80001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:69: host_info(mach_host, HOST_BASIC_INFO, reinterpret_cast<host_info_t>(&hbi), Can you confirm that this works within the sandbox? If it fails, I think we'll need to do what I proposed offline.
https://codereview.chromium.org/1665173002/diff/40001/webrtc/video/overuse_fr... File webrtc/video/overuse_frame_detector.h (right): https://codereview.chromium.org/1665173002/diff/40001/webrtc/video/overuse_fr... webrtc/video/overuse_frame_detector.h:59: host_info(mach_host_self(), HOST_BASIC_INFO, (host_info_t)&hbi, &datasize); On 2016/02/05 16:31:05, torbjorng (webrtc) wrote: > On 2016/02/04 22:37:15, Robert Sesek wrote: > > The result of mach_host_self must be deallocated. This is currently leaking a > > system resource. > > Robert: I changed the code to look more like in other places in WebRTC. This > means calling mach_port_deallocate with mach_task_self(). Since the latter just > like mach_host_self() returns a magic port number (an int) one might suspect > that the deallocation could leak. I cannot find proper documentation, but IIUC, > these port numbers are (small) refcounted kernel resources. Thanks! This is now OK. mach_task_self() is actually a macro for an extern of the task port cached in userspace, so you don't need to deallocate it. mach_host_self() is a real kernel trap, so you do have to deallocate that. (Yes, this is more confusing than it needs to be).
Thanks for feedback! I will wait for Asa's feedback. And I also would like to generalise the code beyond Mac if we can confirm that is needed. https://codereview.chromium.org/1665173002/diff/80001/webrtc/video/overuse_fr... File webrtc/video/overuse_frame_detector.cc (right): https://codereview.chromium.org/1665173002/diff/80001/webrtc/video/overuse_fr... webrtc/video/overuse_frame_detector.cc:66: host_basic_info hbi; On 2016/02/05 22:56:35, tommi-webrtc wrote: > zero initialize > host_basic_info hbi = {}; Done. https://codereview.chromium.org/1665173002/diff/80001/webrtc/video/overuse_fr... webrtc/video/overuse_frame_detector.cc:69: host_info(mach_host, HOST_BASIC_INFO, reinterpret_cast<host_info_t>(&hbi), On 2016/02/05 22:56:35, tommi-webrtc wrote: > Can you confirm that this works within the sandbox? If it fails, I think we'll > need to do what I proposed offline. It works fine in the current use. (I confess that I haven't checked how it is called.) I think we may want to extend base/systeminfo.{cc,h} anyway at some point, but that'd require physical core count implementations for all supported platforms. I haven't done that since it is more work, and because I wanted to evaluate this approach specifically for Mac OS. (I have checked with the help of hbos@ that similar hardware running Windows does not exhibit system lag. I have yet to check iOS and GNU/Linux.)
The modification of the thresholds lgtm.
lgtm
On 2016/02/08 09:57:10, tommi-webrtc wrote: > lgtm Torbjorn - are we waiting for approval or are there more changes coming? I'd like to get this in early this week so we'll have some canaries to test toward the end of the week.
I have looked more into the CPU usage distribution. The overuse mechanism that this CL adjusts measures precisely the encoding time. Other parts of the send side video computations are not included. The encoding time is just ~15 ms @ 720p on a low-end Mac (for mostly static image, it goes up to 50 ms for non-static). 15 ms is about 1/4 of the total per-frame time available at 30 fps of 2/(30/s) = 66 ms. Decoding surprisingly uses as much as 1/2 and 2/3 of the encoding time, or about 8 to 10 ms per 720p frame, and here we have little control. Rendering of the incoming video takes lots of time too. These numbers are for VP9 encoding and decoding. Hangouts still uses VP8. The VP9 decoder is presumably better optimised. From my measurements, I expect this CL to improve things but not make 2 core Macs great at running Hangouts, in particular not with several incoming streams. But think we should lower the 1 core and 2 core overuse percentage numbers of this CL before landing. I am working out what these should really be. (Incidentally, using apprtc in loopback mode makes this CL's manipulation of the scaling unnaturally effective, as it lowers the decoder load too...)
PTAL asapersson and tommi.
lgtm
lgtm
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665173002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665173002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Experimental patch for adapting adaptation to CPU count on Mac. BUG= ========== to ========== Experimental patch for adapting adaptation to CPU count on Mac. BUG= Committed: https://crrev.com/448468d575725f67de4eedb3519b46e2a2e9374f Cr-Commit-Position: refs/heads/master@{#11553} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/448468d575725f67de4eedb3519b46e2a2e9374f Cr-Commit-Position: refs/heads/master@{#11553} |