Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8342040:
Gather history of capture and encode time determine next recoring delay (Closed)
|
Created:
9 years, 2 months ago by Alpha Left Google Modified:
9 years, 1 month ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, Paweł Hajdan Jr., dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Visibility:
Public. |
DescriptionGather history of capture and encode time determine next recoring delay
The current logic of chromoting is to capture as fast as possible with
20fps as a hard limit. This creates a heavy load on the slower systems with
only one processor.
I measured 100% CPU load running on these systems, resulting in very
jerky performance. In addition to speed up capture and encode we should
use system load and configurations as an information to determine recording
frequency.
In this patch I used the number of processors and history of capture and
encode time to determine the next recording delay. This essentially lower
recoding frequency when system is under load or is incapable to catch up
with the current 20fps frequency.
I have tested this patch on a Linux system limited to single core and
intentionally added extra 70ms of while(1) loop in capture and encode to
simulate slow processing. The results are as follows:
CPU load Latency
Before 100% 440ms
After ~50% 360ms
So as a result both CPU load and latency improved.
BUG=100314
TEST=remoting_unittests and manually test with with heavily loaded system
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108871
Patch Set 1 #Patch Set 2 : tested and removed testing code #
Total comments: 50
Patch Set 3 : done #
Total comments: 32
Patch Set 4 : done #Patch Set 5 : done #
Total comments: 5
Messages
Total messages: 15 (0 generated)
On 2011/10/19 16:07:36, Alpha wrote: I didn't the down side of doing, it's that framerate will be halved. It's probably not worthwhile to run at such high framerate given high latency and cpu load..
I like that you separated frame rate control from ScreenRecorder! I have some comments and suggestions. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:19: const base::TimeDelta kMinimumRecordingDelay = This should be int64. Complex types are not allowed for statics, and it probably requires static initializer. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:20: base::TimeDelta::FromMilliseconds(50); IMO this is to high. It means that we cannot reach latency less than 50ms. maybe 20ms. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.h (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:17: class RecordingRateRegulator { maybe call it controller instead of regulator. I think the common term is "rate control", not "rate regulation". http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:26: void RecordCaptureTime(int64 ms); Use TimeDelta here and in the next method. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... File remoting/host/screen_recorder.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:30: static const int kMaxRecordings = 2; Can we move this to rate controller class? E.g. you can pass |recordings_| to NextRecordingDelay(), and it would return -1 if |recordings_| is to high and we need to wait for one next frame to be processed. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:57: max_recordings_ = 2; Don't get it. kMaxRecording = 2, so max_recordings_ is set to 2 ether way. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:149: StartCaptureTimer(); DoCapture() call below starts the timer, so I think we can remove this now. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:182: capture_timer_.Stop(); Do we need this? DoCapture() is supposed to be called by the timer, so the timer should not be running at this point because it is OneShotTimer.
http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:15: const int kStatisticsWindow = 3; What units is this in? http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:18: // value causes significant slow down on the system. That's true of current systems; why would it be true in future? http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:27: const double kRecordingDelayFactor = 0.5; Isn't that just a percentage processor usage, in effect? http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:59: // saturated there's still one left for doing other things. We have the special-case for (2)? Your scheme seems to assume that for 3 or more processors it's OK to use 2 of them entirely, and that for 2 it's OK to use one entirely, in effect. Why not just limit on the total %age usage across all cores, by summing the encode and capture times, dividing down by the number of cores, and basing the delay on that? http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.h (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:5: // This class is used to throttle the recoring rate of ScreenRecorder. typo: "recoring" http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:6: // In general this class gathers history of time spent on encoding and nit: "In general"? http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:8: // The specific algorithm is based on system configuration and/or system nit: That's not true, though, is it? The algorithm is the same, but takes into account those factors. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:17: class RecordingRateRegulator { Please rename this CaptureScheduler, FrameScheduler or VideoFrameScheduler, or something of that sort, since that describes what it does. Similarly, the NextRecordingDelay() method should probably become NextFrameInterval() or similar. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:23: base::TimeDelta NextRecordingDelay(); What is the start time for this "delay"? http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... File remoting/host/screen_recorder.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:54: // If the system has only one processor there's no use to have simutaneous typo: "simutaneous" http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:55: // recording sessions, it will just create excessive system load. The rate-limiting should result in this, surely? http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:149: StartCaptureTimer(); On 2011/10/20 00:17:00, sergeyu wrote: > DoCapture() call below starts the timer, so I think we can remove this now. StartCaptureTimer() doesn't seem to be required at all? http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorder.h File remoting/host/screen_recorder.h (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.h:188: // Maximum simutaneous recordings allowed. Note that this number typo: "simutaneous" http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... File remoting/host/screen_recorder_unittest.cc (left): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder_unittest.cc:149: } No replacement test?
http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:15: const int kStatisticsWindow = 3; On 2011/10/20 00:52:15, Wez wrote: > What units is this in? There's no unit, just a divider. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:18: // value causes significant slow down on the system. On 2011/10/20 00:52:15, Wez wrote: > That's true of current systems; why would it be true in future? I'm just gonna remove this comment. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:20: base::TimeDelta::FromMilliseconds(50); On 2011/10/20 00:17:00, sergeyu wrote: > IMO this is to high. It means that we cannot reach latency less than 50ms. maybe > 20ms. It's not just whether hardware can handle it.. but also bandwidth too, given the difficulty we have now I'd like to keep this until we get a better understand of network problems. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.h (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:5: // This class is used to throttle the recoring rate of ScreenRecorder. On 2011/10/20 00:52:15, Wez wrote: > typo: "recoring" Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:6: // In general this class gathers history of time spent on encoding and On 2011/10/20 00:52:15, Wez wrote: > nit: "In general"? Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:8: // The specific algorithm is based on system configuration and/or system On 2011/10/20 00:52:15, Wez wrote: > nit: That's not true, though, is it? The algorithm is the same, but takes into > account those factors. Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:17: class RecordingRateRegulator { On 2011/10/20 00:17:00, sergeyu wrote: > maybe call it controller instead of regulator. I think the common term is "rate > control", not "rate regulation". Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:23: base::TimeDelta NextRecordingDelay(); On 2011/10/20 00:52:15, Wez wrote: > What is the start time for this "delay"? Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.h:26: void RecordCaptureTime(int64 ms); On 2011/10/20 00:17:00, sergeyu wrote: > Use TimeDelta here and in the next method. Done.
http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:20: base::TimeDelta::FromMilliseconds(50); On 2011/10/20 14:18:20, Alpha wrote: > On 2011/10/20 00:17:00, sergeyu wrote: > > IMO this is to high. It means that we cannot reach latency less than 50ms. > maybe > > 20ms. > > It's not just whether hardware can handle it.. but also bandwidth too, I don't think this is related to bandwidth. We currently account for limited bandwidth by limiting number of in-flight frames (kMaxRecordings). > given the > difficulty we have now I'd like to keep this until we get a better understand of > network problems.
I think it's better to take smaller steps, i.e. find a way to make flow control work better (not buffer too much in host side) before we try to generate more data. Moreover we have a descent frame rate and there's no urgent need to improve it.
On 2011/10/21 16:32:59, Alpha wrote: > I think it's better to take smaller steps, i.e. find a way to make flow control > work better (not buffer too much in host side) before we try to generate more > data. Moreover we have a descent frame rate and there's no urgent need to > improve it. +1 to smaller steps, so long as we have data to support the decision to take them. :)
ping!
On 2011/10/24 10:32:28, Alpha wrote: > ping! Can you please address my and Wez's comments in screen_recorder.cc?
http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:15: const int kStatisticsWindow = 3; On 2011/10/20 14:18:20, Alpha wrote: > On 2011/10/20 00:52:15, Wez wrote: > > What units is this in? > > There's no unit, just a divider. Surely it's the time window over which the encode and capture statistics are averaged, in which case it has time units? http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:20: base::TimeDelta::FromMilliseconds(50); On 2011/10/20 17:19:11, sergeyu wrote: > On 2011/10/20 14:18:20, Alpha wrote: > > On 2011/10/20 00:17:00, sergeyu wrote: > > > IMO this is to high. It means that we cannot reach latency less than 50ms. > > maybe > > > 20ms. > > > > It's not just whether hardware can handle it.. but also bandwidth too, > I don't think this is related to bandwidth. We currently account for limited > bandwidth by limiting number of in-flight frames (kMaxRecordings). Limiting the number of in-flight frames will restrict the frame rate on LFNs, though. I think Sergey's right; the lower bound should represent our ideal target frame-rate and the capture throttling should rduce that down to a rate that doesn't saturate the machine. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:48: // More specifically this is seperated into three cases: nit: separated http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... File remoting/host/capture_scheduler.cc (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:16: // This constant controls how many recorded instances are averaged. Please word this more clearly. Intuitively one would expect the window to specify the time interval over which values are averaged, but this is actually the number of samples over which to average. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:17: const int kStatisticsWindow = 3; Why average over three previous samples? Why not just consider the most recent capture & encode; that way you can almost guarantee the CPU usage you require - if one frame encodes very quickly because not much has changed then you'll delay very little. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:20: const int64 kMinimumRecordingDelay = 50; This still seems unnecessarily low, given that this class should end up rate-limiting appropriately based on the time taken to capture and encode. Arguably, this should also be a parameter to the scheduler, not a hard-coded constant. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:23: // This is capped at 50%. I'd suggest rewording to comment to something describing what the range 0.0-1.0 mean, i.e. allow no CPU usage to allowing continuous use of an entire core. I think it'd be simpler to have this as a ratio of total CPU usage across all cores. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:31: : num_of_processors_(base::SysInfo::NumberOfProcessors()), nit: This means we don't cope properly with dynamic changes in the number of available cores... http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:41: // The algorithm only uses CPU time spent in capture, encode and typo: "... capture, encode and..." -> "... capture and encode, and the ..." http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:42: // number of processors to determine next capture time. nit: processors -> cores? nit: "determine next capture time" -> "choose the next capture interval" http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:44: // exceed a predefined value, i.e. kRecordingCpuConsumption. nit: This comment seems to reiterate the basic function of this component? http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:46: (capture_time_.Average() + encode_time_.Average()) / Note that the encode time will vary considerably depending on how much has changed on-screen, whereas the capture time average only will on platforms where we're using update notifications. Where we're polling for changes capture time will be pretty constant. Also bear in mind that capture time may have side-effects, e.g. if while capturing no other process can be rendering. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:47: kRecordingCpuConsumption / num_of_processors_; It's clearer to write this formula as: delay = (capture + encode) / (consumption * cores); This formula treats kRecordingCpuConsumption as a proportion of CPU usage across all cores, not a single core as was previously discussed. Usage across all cores makes more sense, I think, but you should make sure it's clear that that's what the proportion means. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... File remoting/host/capture_scheduler.h (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.h:5: // This class is used to determine the recoring rate of ScreenRecorder. typo: recoring http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.h:7: // determine the delay for next recoding cycle. typo: recoding nit: Please word this more clearly; this class chooses a capture interval so as to limit CPU usage to not exceed a specified %age. It bases this on the CPU usage of recent capture and encode operations, and on the number of available CPUs. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.h:27: // Recording time spend on capturing and encoding. typos: Recording->Record, spend->spent. http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... File remoting/host/screen_recorder.cc (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:57: max_recordings_ = 2; Is there evidence of excessive load, even with the throttling in place? http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorder.h File remoting/host/screen_recorder.h (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.h:188: // Maximum simutaneous recordings allowed. Note that this number typo: simutaneous http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.h:189: // cannot exceed 2 at any time. If this number cannot exceed 2, why do we even have it? We don't really need this number hard-coding, even - it falls out naturally from the fact that we have two parallelizable stages in our pipeline.
http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... File remoting/host/recording_rate_regulator.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:15: const int kStatisticsWindow = 3; On 2011/10/25 00:05:37, Wez wrote: > On 2011/10/20 14:18:20, Alpha wrote: > > On 2011/10/20 00:52:15, Wez wrote: > > > What units is this in? > > > > There's no unit, just a divider. > > Surely it's the time window over which the encode and capture statistics are > averaged, in which case it has time units? It's not a time window since it's not used for rate but a window to average encode and capture time over multiple iterations. This value means the iterations we take to average these values. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:19: const base::TimeDelta kMinimumRecordingDelay = On 2011/10/20 00:17:00, sergeyu wrote: > This should be int64. Complex types are not allowed for statics, and it probably > requires static initializer. Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:20: base::TimeDelta::FromMilliseconds(50); On 2011/10/25 00:05:37, Wez wrote: > On 2011/10/20 17:19:11, sergeyu wrote: > > On 2011/10/20 14:18:20, Alpha wrote: > > > On 2011/10/20 00:17:00, sergeyu wrote: > > > > IMO this is to high. It means that we cannot reach latency less than 50ms. > > > maybe > > > > 20ms. > > > > > > It's not just whether hardware can handle it.. but also bandwidth too, > > I don't think this is related to bandwidth. We currently account for limited > > bandwidth by limiting number of in-flight frames (kMaxRecordings). > > Limiting the number of in-flight frames will restrict the frame rate on LFNs, > though. > > I think Sergey's right; the lower bound should represent our ideal target > frame-rate and the capture throttling should rduce that down to a rate that > doesn't saturate the machine. Yes I agree that ideally this should be the ideal case we want to achieve but given that we have problems with the rate controlling, doing so will just pump more data to the send buffer to the stack which is not a good move until the flow control problem is solved. That being said, this change is about improving the situations due to system load, I don't want to introduce more uncertainties. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:27: const double kRecordingDelayFactor = 0.5; On 2011/10/20 00:52:15, Wez wrote: > Isn't that just a percentage processor usage, in effect? Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:48: // More specifically this is seperated into three cases: On 2011/10/25 00:05:37, Wez wrote: > nit: separated Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/recording_rate... remoting/host/recording_rate_regulator.cc:59: // saturated there's still one left for doing other things. On 2011/10/20 00:52:15, Wez wrote: > We have the special-case for (2)? > > Your scheme seems to assume that for 3 or more processors it's OK to use 2 of > them entirely, and that for 2 it's OK to use one entirely, in effect. > > Why not just limit on the total %age usage across all cores, by summing the > encode and capture times, dividing down by the number of cores, and basing the > delay on that? I changed the whole logic. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... File remoting/host/screen_recorder.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:30: static const int kMaxRecordings = 2; On 2011/10/20 00:17:00, sergeyu wrote: > Can we move this to rate controller class? E.g. you can pass |recordings_| to > NextRecordingDelay(), and it would return -1 if |recordings_| is to high and we > need to wait for one next frame to be processed. I think it'll be better to do it separately. It's not a requirement to this improvement. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:54: // If the system has only one processor there's no use to have simutaneous On 2011/10/20 00:52:15, Wez wrote: > typo: "simutaneous" Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:54: // If the system has only one processor there's no use to have simutaneous On 2011/10/20 00:52:15, Wez wrote: > typo: "simutaneous" Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:55: // recording sessions, it will just create excessive system load. On 2011/10/20 00:52:15, Wez wrote: > The rate-limiting should result in this, surely? Actually max_recordings_ should be 1 here. My bad here. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:57: max_recordings_ = 2; On 2011/10/20 00:17:00, sergeyu wrote: > Don't get it. kMaxRecording = 2, so max_recordings_ is set to 2 ether way. I set it to 1, mistake here. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:149: StartCaptureTimer(); On 2011/10/20 00:17:00, sergeyu wrote: > DoCapture() call below starts the timer, so I think we can remove this now. Done. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:182: capture_timer_.Stop(); On 2011/10/20 00:17:00, sergeyu wrote: > Do we need this? DoCapture() is supposed to be called by the timer, so the timer > should not be running at this point because it is OneShotTimer. We shouldn't hit this case but there's no harm doing this. http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorder.h File remoting/host/screen_recorder.h (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.h:188: // Maximum simutaneous recordings allowed. Note that this number On 2011/10/20 00:52:15, Wez wrote: > typo: "simutaneous" Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... File remoting/host/capture_scheduler.cc (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:16: // This constant controls how many recorded instances are averaged. On 2011/10/25 00:05:37, Wez wrote: > Please word this more clearly. Intuitively one would expect the window to > specify the time interval over which values are averaged, but this is actually > the number of samples over which to average. Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:17: const int kStatisticsWindow = 3; On 2011/10/25 00:05:37, Wez wrote: > Why average over three previous samples? Why not just consider the most recent > capture & encode; that way you can almost guarantee the CPU usage you require - > if one frame encodes very quickly because not much has changed then you'll delay > very little. My intention is to have a relatively steady frame rate. If we choose this based on the last sample the frame rate may changes quite a lot. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:20: const int64 kMinimumRecordingDelay = 50; On 2011/10/25 00:05:37, Wez wrote: > This still seems unnecessarily low, given that this class should end up > rate-limiting appropriately based on the time taken to capture and encode. > Arguably, this should also be a parameter to the scheduler, not a hard-coded > constant. You have a different opinion than Sergey here, he suggested to go lower to 20. Anyway I don't think it's good to change this value now unless we solved the flow control problems. I was hoping that we can extend this class to do stuff like key press interval to predict the best next capture start time, I would like to keep all the parameters in this class so it's easier to experiment with parameters or other algorithm (it will be in "some" class anyway, better keep it more searchable). http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:23: // This is capped at 50%. On 2011/10/25 00:05:37, Wez wrote: > I'd suggest rewording to comment to something describing what the range 0.0-1.0 > mean, i.e. allow no CPU usage to allowing continuous use of an entire core. I > think it'd be simpler to have this as a ratio of total CPU usage across all > cores. Yes it's a ratio across all corres already. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:31: : num_of_processors_(base::SysInfo::NumberOfProcessors()), On 2011/10/25 00:05:37, Wez wrote: > nit: This means we don't cope properly with dynamic changes in the number of > available cores... Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:42: // number of processors to determine next capture time. On 2011/10/25 00:05:37, Wez wrote: > nit: processors -> cores? I think we should follow the naming of base::SysInfo::NumberOfProcessors(). > nit: "determine next capture time" -> "choose the next capture interval" done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:44: // exceed a predefined value, i.e. kRecordingCpuConsumption. On 2011/10/25 00:05:37, Wez wrote: > nit: This comment seems to reiterate the basic function of this component? Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:47: kRecordingCpuConsumption / num_of_processors_; On 2011/10/25 00:05:38, Wez wrote: > It's clearer to write this formula as: > > delay = (capture + encode) / (consumption * cores); > > This formula treats kRecordingCpuConsumption as a proportion of CPU usage across > all cores, not a single core as was previously discussed. Usage across all > cores makes more sense, I think, but you should make sure it's clear that that's > what the proportion means. Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... File remoting/host/capture_scheduler.h (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.h:5: // This class is used to determine the recoring rate of ScreenRecorder. On 2011/10/25 00:05:38, Wez wrote: > typo: recoring Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.h:7: // determine the delay for next recoding cycle. On 2011/10/25 00:05:38, Wez wrote: > typo: recoding > nit: Please word this more clearly; this class chooses a capture interval so as > to limit CPU usage to not exceed a specified %age. It bases this on the CPU > usage of recent capture and encode operations, and on the number of available > CPUs. Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.h:27: // Recording time spend on capturing and encoding. On 2011/10/25 00:05:38, Wez wrote: > typos: Recording->Record, spend->spent. Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... File remoting/host/screen_recorder.cc (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:57: max_recordings_ = 2; On 2011/10/25 00:05:38, Wez wrote: > Is there evidence of excessive load, even with the throttling in place? The original design was assuming multiple core we can run with a higher frame rate. Yes I agree we shouldn't have multiple things in this patch, I'll remove this. http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorder.h File remoting/host/screen_recorder.h (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.h:188: // Maximum simutaneous recordings allowed. Note that this number On 2011/10/25 00:05:38, Wez wrote: > typo: simutaneous Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.h:188: // Maximum simutaneous recordings allowed. Note that this number On 2011/10/25 00:05:38, Wez wrote: > typo: simutaneous Done. http://codereview.chromium.org/8342040/diff/5002/remoting/host/screen_recorde... remoting/host/screen_recorder.h:189: // cannot exceed 2 at any time. On 2011/10/25 00:05:38, Wez wrote: > If this number cannot exceed 2, why do we even have it? > We don't really need this number hard-coding, even - it falls out naturally from > the fact that we have two parallelizable stages in our pipeline. removed.
lgtm http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... File remoting/host/screen_recorder.cc (right): http://codereview.chromium.org/8342040/diff/2001/remoting/host/screen_recorde... remoting/host/screen_recorder.cc:30: static const int kMaxRecordings = 2; On 2011/10/31 18:47:24, Alpha wrote: > On 2011/10/20 00:17:00, sergeyu wrote: > > Can we move this to rate controller class? E.g. you can pass |recordings_| to > > NextRecordingDelay(), and it would return -1 if |recordings_| is to high and > we > > need to wait for one next frame to be processed. > > I think it'll be better to do it separately. It's not a requirement to this > improvement. > add TODO for this?
LGTM, but please see my comments re comments, below! http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... File remoting/host/capture_scheduler.cc (right): http://codereview.chromium.org/8342040/diff/5002/remoting/host/capture_schedu... remoting/host/capture_scheduler.cc:20: const int64 kMinimumRecordingDelay = 50; On 2011/10/31 18:47:24, Alpha wrote: > On 2011/10/25 00:05:37, Wez wrote: > > This still seems unnecessarily low, given that this class should end up > > rate-limiting appropriately based on the time taken to capture and encode. > > Arguably, this should also be a parameter to the scheduler, not a hard-coded > > constant. > > You have a different opinion than Sergey here, he suggested to go lower to 20. > Anyway I don't think it's good to change this value now unless we solved the > flow control problems. > > I was hoping that we can extend this class to do stuff like key press interval > to predict the best next capture start time, I would like to keep all the > parameters in this class so it's easier to experiment with parameters or other > algorithm (it will be in "some" class anyway, better keep it more searchable). Sorry, I meant that this value would lead to a low frame rate... http://codereview.chromium.org/8342040/diff/17001/remoting/host/capture_sched... File remoting/host/capture_scheduler.cc (right): http://codereview.chromium.org/8342040/diff/17001/remoting/host/capture_sched... remoting/host/capture_scheduler.cc:15: // Number of samples to average the most recent capture and encode time. nit: You need "over" at the end of the sentence. :) http://codereview.chromium.org/8342040/diff/17001/remoting/host/capture_sched... remoting/host/capture_scheduler.cc:22: // Range of this value is between 0 to 1. This comment still doesn't make it clear whether the value 1 means "use all of one CPU" or "use all of all CPUs". The comment should indicate what the value _means_, not just the syntax. http://codereview.chromium.org/8342040/diff/17001/remoting/host/capture_sched... remoting/host/capture_scheduler.cc:30: // change dynamically. Is the comment to address my nit? If so then please reword to something like "We assume that the number of available cores is constant". http://codereview.chromium.org/8342040/diff/17001/remoting/host/capture_sched... remoting/host/capture_scheduler.cc:43: // and number of processors to determine the next capture interval. Could this comment indicate the intended behaviour of the algorithm, rather than just telling me which factors it takes into account? e.g. Delay by an amount chosen such that if capture and encode times continue to follow the averages, then we'll consume the target fraction of CPU across all cores.
http://codereview.chromium.org/8342040/diff/17001/remoting/host/screen_record... File remoting/host/screen_recorder_unittest.cc (left): http://codereview.chromium.org/8342040/diff/17001/remoting/host/screen_record... remoting/host/screen_recorder_unittest.cc:107: TEST_F(ScreenRecorderTest, OneRecordCycle) { What's the reason to remove this test? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
