Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
292373002:
More reliable keep-alive video packets. (Closed)
|
Created:
6 years, 7 months ago by Sergey Ulanov Modified:
6 years, 7 months ago Reviewers:
Wez CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMore reliable keep-alive video packets.
Previously the host was not sending empty keep alive messages when
video stream is paused or when it's blocked on the encoder, this
triggers reconnect too often.
Refactored keep-alive logic in VideoScheduler so now it always
sends keep-alive messages when there is no other activity on the
stream.
BUG=376528, 375568
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272790
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 18 (0 generated)
This seems the wrong approach to me; looking at the MonitoringVideoStub it seems to declare the connection as broken if it see no frames for one second, which is a really aggressive timeout - can we address that rather than hack more frequent keep-alives in? https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:34: // when there are not other video frame. nit: other video frame -> real video frames being sent. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:35: static const int kKeepAlivePacketIntervalMs = 200; Five keep-alives per second seems an awful lot. Why do we need them so frequently? https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:85: // Send only non-empty frames. You mean "Skip encoding & sending empty frames."? I know this check was already in place, but do we actually want it? If we get to a point of sending lossy data initially and then "topping up" to lossless then this optimization will break that, since it prevents the encoder from being passed the frame to work on. Could defer this decision to the encoder implementation itself. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:276: keep_alive_timer_->Reset(); Do we need to check whether |keep_alive_timer_| is NULL here? https://codereview.chromium.org/292373002/diff/1/remoting/host/video_scheduler.h File remoting/host/video_scheduler.h (right): https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.h:182: // Timer used to insure that we occasionally send a frame to the client insure -> ensure https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.h:182: // Timer used to insure that we occasionally send a frame to the client nit: suggest "occasionally send a frame" -> "send empty keep-alive frames" https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.h:183: // even when the video stream is paused or encoder is busy. Why would we want to send frames when the video stream is paused? The point of pausing is to stop video packets causing consumption of bandwidth, CPU & battery.
This seems the wrong approach to me; looking at the MonitoringVideoStub it seems to declare the connection as broken if it see no frames for one second, which is a really aggressive timeout - can we address that rather than hack more frequent keep-alives in? I agree that current timeout is too aggressive. I sent you a separate change to increase timeout to 2 seconds. This change actually makes keep-alive packets less frequent, not more. Previously we were sending keep-alive messages 30 times per second. But now they will be sent even when the scheduler is blocked on the encoder or capturer (e.g. if OS blocks capturer for long time). Another difference is that now they are sent even stream is paused (see my comments for why its necessary). https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:34: // when there are not other video frame. On 2014/05/23 00:45:24, Wez wrote: > nit: other video frame -> real video frames being sent. Done. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:35: static const int kKeepAlivePacketIntervalMs = 200; On 2014/05/23 00:45:24, Wez wrote: > Five keep-alives per second seems an awful lot. Why do we need them so > frequently? Changed it to 2 per second. FWIW we were previously sending 30 keep-alive frames when nothing is not changing on the screen. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:85: // Send only non-empty frames. On 2014/05/23 00:45:24, Wez wrote: > You mean "Skip encoding & sending empty frames."? > > I know this check was already in place, but do we actually want it? If we get to > a point of sending lossy data initially and then "topping up" to lossless then > this optimization will break that, since it prevents the encoder from being > passed the frame to work on. Even when we have this feature implemented in the encode we will still need to stop sending frames at some point if nothing is changing. It's just condition for when to stop would be different. Also client currently ignores frames with empty updated_region() anyway. > > Could defer this decision to the encoder implementation itself. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:276: keep_alive_timer_->Reset(); On 2014/05/23 00:45:24, Wez wrote: > Do we need to check whether |keep_alive_timer_| is NULL here? No. video_stub_ and keep_alive_timer_ are reset in the same place, so if video_stub_ is not NULL then keep_alive_timer_ cannot be NULL. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_scheduler.h File remoting/host/video_scheduler.h (right): https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.h:182: // Timer used to insure that we occasionally send a frame to the client On 2014/05/23 00:45:24, Wez wrote: > insure -> ensure Done. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.h:182: // Timer used to insure that we occasionally send a frame to the client On 2014/05/23 00:45:24, Wez wrote: > nit: suggest "occasionally send a frame" -> "send empty keep-alive frames" Done. https://codereview.chromium.org/292373002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.h:183: // even when the video stream is paused or encoder is busy. On 2014/05/23 00:45:24, Wez wrote: > Why would we want to send frames when the video stream is paused? The point of > pausing is to stop video packets causing consumption of bandwidth, CPU & > battery. That is to fix crbug.com/376528. Libjingle has to send pings on each transport channel every 480ms anyway, so I don't think this change will have any measurable effect on resources consumption. Ideally we should be using state of the channel to detect when connection is broken, but that would require changes in libjingle to get more detailed notifications about state of the connection. That's why we decided to use frames stream to monitor status of connection. We could also change the client to disable reconnect logic in case when video is paused, but it would be more complicated change, not something we would want to merge to M36.
lgtm https://codereview.chromium.org/292373002/diff/20001/remoting/host/video_sche... File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/292373002/diff/20001/remoting/host/video_sche... remoting/host/video_scheduler.cc:34: // when there are no real video frame being sent. typo: frame -> frames
https://codereview.chromium.org/292373002/diff/20001/remoting/host/video_sche... File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/292373002/diff/20001/remoting/host/video_sche... remoting/host/video_scheduler.cc:34: // when there are no real video frame being sent. On 2014/05/24 05:57:08, Wez wrote: > typo: frame -> frames Done.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/292373002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/292373002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/292373002/40001
Message was sent while issue was closed.
Change committed as 272790 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
