Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
850983002:
Implement video frame acknowledgements in the chromoting protocol. (Closed)
|
Created:
5 years, 11 months ago by Sergey Ulanov Modified:
5 years, 10 months ago Reviewers:
Wez CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement video frame acknowledgements in the chromoting protocol.
Added frame_id field in the VideoPacket message. Client now acknowledges
every frame that has frame_id set by sending VideoAck messages after
the corresponding frame is decoded and rendered.
On the host the VideoAck messages are processed by the new
VideoFeedbackStub, which is implemented in CaptureScheduler.
CaptureScheduler limits number of unacknowledged frames to 4. This
number was chosen experimentally to minimize latency (using
remoting_perftests).
BUG=448838
Committed: https://crrev.com/97568a816ed5fe0a69a2c0f6d0987381899f58d8
Cr-Commit-Position: refs/heads/master@{#317824}
Patch Set 1 #
Total comments: 74
Patch Set 2 : #Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 32
Patch Set 5 : #
Total comments: 12
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Messages
Total messages: 37 (13 generated)
sergeyu@chromium.org changed reviewers: + wez@chromium.org
Is this ready for review, or are there updates based on your discussion w/ Hubbe?
On 2015/01/17 02:59:14, Wez wrote: > Is this ready for review, or are there updates based on your discussion w/ > Hubbe? Yes, it is ready for review. Based on the discussion with Hubbe chromecast uses nearly identical mechanism with frame-level acks. The main difference is that in case of chromecast latency is less important than smoothness, so they allow higher number of pending frames. The mechanism I've implemented here is that it allows to improve performance now, before we can move to QUIC. Once we move to QUIC we can try integrating with QUIC's congestion control somehow, but even then frame ACKs will be useful, e.g. to handle the case when decoding/rendering on the client is slow for some reason (currently we block receiver in that case, which is not efficient because it blows up number of frames in the buffer).
You're adding fields to the ACKs for the frame rendering delay, and for the "read delay", but it's not clear from this CL whether they are actually useful. Frame rendering delay is a function of decode delay (which essentially limits frame throughput) and v-blank synchronization (which is really only interesting if the host is going to try to get in-sync with the client v-blanks to reduce latency, say). Read delay is in principle the elapsed time between the first and final bytes of a packet being received, but is meaningless for small VideoPackets due to packetization leading to zero delays. It might be useful in helping calculate available bandwidth, given large VideoPackets, but I'd expect the underlying transport (e.g. PseudoTcp, QUIC, whatever) to be better placed to gauge that, surely? https://codereview.chromium.org/850983002/diff/1/remoting/host/video_schedule... File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:34: static const int kMaxPendingFrames = 4; kMaxPendingFrames controls the number of screen-capture operations pending, IIRC - our current capturers are only designed to cope with up to two capture frames in-flight, i.e. they don't necessarily cope with more outstanding frames. https://codereview.chromium.org/850983002/diff/1/remoting/proto/video.proto File remoting/proto/video.proto (right): https://codereview.chromium.org/850983002/diff/1/remoting/proto/video.proto#n... remoting/proto/video.proto:78: // the end. nit: to receive the VideoPacket. Also consider noting that for small VideoPackets this can be zero. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/channel_di... File remoting/protocol/channel_dispatcher_base.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/channel_di... remoting/protocol/channel_dispatcher_base.h:58: const ChannelConfig& channel_config() { return channel_config_; } Does this need to be public, or could it be protected? Do only derived classes need to poke at the config, or other code as well? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/channel_di... remoting/protocol/channel_dispatcher_base.h:66: void NotifyError(ErrorCode error); Do you need this, or could you just expose an event_handler() getter and let callers do it? If you feel this is cleaner then suggest comment: "Calls event_handler_->OnChannelError() with the supplied |error|" to make clear that that's all it does. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... File remoting/protocol/client_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.cc:33: VideoAck ack_message; Is creating and copying a protobuf super cheap? Consider just passing the two parameters directly to OnPacketProcessed and creating the ACK message there? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.cc:37: reader()->last_message_read_delay().InMicroseconds()); nit: This line is logically part of initializing ack_message, so move it up to make a since create-and-initialize-ack_message block? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.cc:51: if (channel_config().version >= kVideoWithAckStreamVersion) { The stream version never changes mid-session, so I'd suggest rearranging things to check before the Bind(), except that this will become the "normal" path in future. So, given that the normal path is to send the ack, I'd suggest rewriting this if() to be an early-exit. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... File remoting/protocol/client_video_dispatcher.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.h:26: void OnPacketProcessed(VideoAck ack_message, const& (Bind() should still copy the parameter into the closure, but you'll avoid an extra copy when it is Run()) https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.h:28: const base::Closure& done); Suggest more helpful naming, e.g. OnVideoPacket->ProcessVideoPacket and OnPacketProcessed->AcknowledgeVideoPacket. Or if you prefer to keep the names generic-sounding then add comments to explain their purpose. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... File remoting/protocol/client_video_dispatcher_unittest.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:42: void Initialize(int version) { nit: InitializeWithProtocolVersion? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:43: EXPECT_CALL(event_handler_, OnChannelInitialized(_)); Having this expectation in here immediately followed by code that calls it means that the caller has to know to create any _other_ expectations before the Initialize(). Consider moving the |event_handler_| expectation into SetUp() instead? Also, consider having a Times(0) expectation for OnChannelError()? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:57: const base::Closure& done) { Why is ChannelEventHandler a mock but VideoStub a base-class? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:76: BufferedSocketWriter writer_; nit: Can you break these up a bit to separate out the object-under-test from the mock host and client parts, respectively? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:85: TEST_F(ClientVideoDispatcherTest, WithoutAcks) { nit: Add a basic test to verify just that parsed VideoPackets reach the VideoStub? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:96: message_loop_.RunUntilIdle(); IIUC the New Way is: RunLoop().RunUntilIdle() for situations like this. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:97: ASSERT_TRUE(video_packet_); EXPECT_TRUE https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:103: ASSERT_FALSE(ack_message_); EXPECT_FALSE https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:106: // Verifies that the dispatchers sends Ack message with correct rendering delay. s/dispatchers/dispatcher https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:120: ASSERT_TRUE(video_packet_); EXPECT_TRUE https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:131: packet_processed_closure_.Run(); Comment this, e.g. "Fake completion of video packet decoding, to trigger the Ack." https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:133: base::TimeTicks::Now() - write_started_time; nit: Could we use a base::TickTimer "seam" to allow this to be tested more cleanly, perhaps? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:167: // one in a single network packet. nit: s/one/frame https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:167: // one in a single network packet. nit: There's no guarantee that this will result in a single packet, just that all of the data in this one buffer will be PSH'd to the receiver, IIUC? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:176: ASSERT_TRUE(video_packet_); nit: EXPECT_TRUE here https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:182: // field set to 0, because the whole frame was received in one message. Doesn't that make this test susceptible to flake e.g. if we start reading the bytes in only to have some other process scheduled, and don't get back to processing until the timer has rolled round? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:202: ASSERT_TRUE(video_packet_); nit: EXPECT_TRUE https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:203: video_packet_.reset(); Do you need to reset it here? You're not touching it later in the test, and earlier tests don't bother resetting it https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:215: base::TimeTicks::Now() - send_started_time; nit: Shouldn't this calculation take place immediately before the packet-processed closure is run? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/host_video... File remoting/protocol/host_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/host_video... remoting/protocol/host_video_dispatcher.cc:53: const base::Closure& done) { Need some kind of comment and bug # reference in here to explain that we'll have some flow-control logic in here eventually. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/host_video... remoting/protocol/host_video_dispatcher.cc:62: pending_frames_.front().done.Run(); Are you guaranteed that the callback won't delete |this|? If not then copy the callback and pop_front() before you Run() it. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_de... File remoting/protocol/message_decoder.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_de... remoting/protocol/message_decoder.h:38: bool is_buffer_empty() { return buffer_.total_bytes() == 0; } Needs a comment to clarify the difference between this and GetNextMessage() returning nullptr. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... File remoting/protocol/message_reader.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... remoting/protocol/message_reader.cc:97: last_message_started_time_ = now; nit: Blank line after this? This logic doesn't seem correct; you're not timing from the start of the latest message being received, but from the read buffer being empty, which is not the same thing. e.g. if I send three messages and received part of the first, then I start this timer, then suppose I receive the end of the first, and the beginning of the second, and then a delay and the rest of the second and all of the third? Then the time I'll report for the third message will be the total time to receive all three, which is wrong. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... File remoting/protocol/message_reader.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... remoting/protocol/message_reader.h:49: base::TimeDelta last_message_read_delay() { nit: Add a brief comment to explain what this means. nit: It's not so much a delay as a duration, or an elapsed time? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... remoting/protocol/message_reader.h:82: base::TimeDelta last_message_read_delay_; nit: Suggest adding a block comment e.g. "Used to track the time taken to receive each message." https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... File remoting/protocol/session_config.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... remoting/protocol/session_config.cc:190: ChannelConfig::CODEC_VP8)); This looks like you're enabling the new video-with-ack all the time. Rather than add kVideoWithAckStreamVersion, I'd suggest we switch to having separate kDefaultVideoVersion, kDefaultEventVersion, kDefaultControlVersion, and then in this CL an additional kNoAckVideoVersion that we can strip out once all endpoints have the change. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... File remoting/protocol/session_config.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... remoting/protocol/session_config.h:18: extern const int kControlStreamVersion; Why do we need a separat kControlStreamVersion..? https://codereview.chromium.org/850983002/diff/1/remoting/protocol/video_send... File remoting/protocol/video_sender.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/video_send... remoting/protocol/video_sender.h:14: class VideoSender : public VideoStub { This name is confusing - classes that implement this interface are not senders, per-se, but receivers/sinks. Given that the VideoStub::ProcessVideoPacket() interface already has |done| as a parameter, why not add the SupportsAck() there? Even though we don't care about the |done| callback behaviour at the client right now, in principle this indicator is just as valid there. e.g. it could be used to throttle if the client has a slow decoder.
I've uploaded another CL that cleans up VideoScheduler a bit: https://codereview.chromium.org/872433005/ . I'm going to rebase this CL on top of it.
1. Addressed all comments. 2. Rebased this CL on top of https://codereview.chromium.org/872433005/ 3. Updated VideoStub: now it uses frame progress notification, so it's possible to get notified when a packet is sent and when it's acked independently. That was necessary for keep-alive messages. https://codereview.chromium.org/850983002/diff/1/remoting/host/video_schedule... File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/host/video_schedule... remoting/host/video_scheduler.cc:34: static const int kMaxPendingFrames = 4; On 2015/01/21 01:35:38, Wez wrote: > kMaxPendingFrames controls the number of screen-capture operations pending, IIRC > - our current capturers are only designed to cope with up to two capture frames > in-flight, i.e. they don't necessarily cope with more outstanding frames. Fixed it now. CaptureScheduler now separately checks number of frames in the encoder's queue and total number of frames. https://codereview.chromium.org/850983002/diff/1/remoting/proto/video.proto File remoting/proto/video.proto (right): https://codereview.chromium.org/850983002/diff/1/remoting/proto/video.proto#n... remoting/proto/video.proto:78: // the end. On 2015/01/21 01:35:38, Wez wrote: > nit: to receive the VideoPacket. > > Also consider noting that for small VideoPackets this can be zero. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/channel_di... File remoting/protocol/channel_dispatcher_base.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/channel_di... remoting/protocol/channel_dispatcher_base.h:58: const ChannelConfig& channel_config() { return channel_config_; } On 2015/01/21 01:35:38, Wez wrote: > Does this need to be public, or could it be protected? Do only derived classes > need to poke at the config, or other code as well? Yes, it's better to make it protected. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/channel_di... remoting/protocol/channel_dispatcher_base.h:66: void NotifyError(ErrorCode error); On 2015/01/21 01:35:38, Wez wrote: > Do you need this, or could you just expose an event_handler() getter and let > callers do it? Yes, I think it's better to have this method instead of using event_handler_ directly. > > If you feel this is cleaner then suggest comment: > "Calls event_handler_->OnChannelError() with the supplied |error|" to make clear > that that's all it does. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... File remoting/protocol/client_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.cc:33: VideoAck ack_message; On 2015/01/21 01:35:38, Wez wrote: > Is creating and copying a protobuf super cheap? Consider just passing the two > parameters directly to OnPacketProcessed and creating the ACK message there? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.cc:37: reader()->last_message_read_delay().InMicroseconds()); On 2015/01/21 01:35:39, Wez wrote: > nit: This line is logically part of initializing ack_message, so move it up to > make a since create-and-initialize-ack_message block? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.cc:51: if (channel_config().version >= kVideoWithAckStreamVersion) { On 2015/01/21 01:35:39, Wez wrote: > The stream version never changes mid-session, so I'd suggest rearranging things > to check before the Bind(), except that this will become the "normal" path in > future. > > So, given that the normal path is to send the ack, I'd suggest rewriting this > if() to be an early-exit. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... File remoting/protocol/client_video_dispatcher.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.h:26: void OnPacketProcessed(VideoAck ack_message, On 2015/01/21 01:35:39, Wez wrote: > const& > > (Bind() should still copy the parameter into the closure, but you'll avoid an > extra copy when it is Run()) Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher.h:28: const base::Closure& done); On 2015/01/21 01:35:39, Wez wrote: > Suggest more helpful naming, e.g. OnVideoPacket->ProcessVideoPacket and > OnPacketProcessed->AcknowledgeVideoPacket. > > Or if you prefer to keep the names generic-sounding then add comments to explain > their purpose. Renamed OnVideoPacket() only. OnPacketProcessed() is now OnPacketProgress(). It doesn't always send ACK, only when the host expects it. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... File remoting/protocol/client_video_dispatcher_unittest.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:42: void Initialize(int version) { On 2015/01/21 01:35:39, Wez wrote: > nit: InitializeWithProtocolVersion? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:43: EXPECT_CALL(event_handler_, OnChannelInitialized(_)); On 2015/01/21 01:35:39, Wez wrote: > Having this expectation in here immediately followed by code that calls it means > that the caller has to know to create any _other_ expectations before the > Initialize(). Consider moving the |event_handler_| expectation into SetUp() > instead? > > Also, consider having a Times(0) expectation for OnChannelError()? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:57: const base::Closure& done) { On 2015/01/21 01:35:39, Wez wrote: > Why is ChannelEventHandler a mock but VideoStub a base-class? Removed MockChannelEventHandler. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:76: BufferedSocketWriter writer_; On 2015/01/21 01:35:39, Wez wrote: > nit: Can you break these up a bit to separate out the object-under-test from the > mock host and client parts, respectively? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:85: TEST_F(ClientVideoDispatcherTest, WithoutAcks) { On 2015/01/21 01:35:39, Wez wrote: > nit: Add a basic test to verify just that parsed VideoPackets reach the > VideoStub? This test already verifies it. Updated the comment. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:96: message_loop_.RunUntilIdle(); On 2015/01/21 01:35:39, Wez wrote: > IIUC the New Way is: > > RunLoop().RunUntilIdle() > > for situations like this. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:97: ASSERT_TRUE(video_packet_); On 2015/01/21 01:35:39, Wez wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:103: ASSERT_FALSE(ack_message_); On 2015/01/21 01:35:39, Wez wrote: > EXPECT_FALSE Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:106: // Verifies that the dispatchers sends Ack message with correct rendering delay. On 2015/01/21 01:35:39, Wez wrote: > s/dispatchers/dispatcher Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:120: ASSERT_TRUE(video_packet_); On 2015/01/21 01:35:39, Wez wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:131: packet_processed_closure_.Run(); On 2015/01/21 01:35:39, Wez wrote: > Comment this, e.g. "Fake completion of video packet decoding, to trigger the > Ack." Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:133: base::TimeTicks::Now() - write_started_time; On 2015/01/21 01:35:39, Wez wrote: > nit: Could we use a base::TickTimer "seam" to allow this to be tested more > cleanly, perhaps? There are two problems with base::TickClock 1. It's designed to be owned by the consumer, which means that it's hard to share it between multiple objects (in this case it needs to be shared between MessageReader and ClientVideoDispatcher) and different object may see different time values if you create multiple MockTickClock instances. I wish there was a global TickClock shared between all clock consumers :( 2. It makes it easier to miss bugs in this CL. E.g. MessageReader::last_message_read_delay() is supposed to return 0 for messages that are received in a single packet and to verify it the timer needs to be incremented between two consecutive tasks executed by MessageLoop, which is hard to achieve with MockTickClock. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:167: // one in a single network packet. On 2015/01/21 01:35:39, Wez wrote: > nit: s/one/frame Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:167: // one in a single network packet. On 2015/01/21 01:35:39, Wez wrote: > nit: There's no guarantee that this will result in a single packet, just that > all of the data in this one buffer will be PSH'd to the receiver, IIUC? I think we can make this assumption with the fake sockets. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:176: ASSERT_TRUE(video_packet_); On 2015/01/21 01:35:39, Wez wrote: > nit: EXPECT_TRUE here Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:182: // field set to 0, because the whole frame was received in one message. On 2015/01/21 01:35:39, Wez wrote: > Doesn't that make this test susceptible to flake e.g. if we start reading the > bytes in only to have some other process scheduled, and don't get back to > processing until the timer has rolled round? No. It's not possible with the current implementation in MessageReader: it calls base::TimeTicks::Now() only once per frame. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:202: ASSERT_TRUE(video_packet_); On 2015/01/21 01:35:39, Wez wrote: > nit: EXPECT_TRUE Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:203: video_packet_.reset(); On 2015/01/21 01:35:39, Wez wrote: > Do you need to reset it here? You're not touching it later in the test, and > earlier tests don't bother resetting it Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/client_vid... remoting/protocol/client_video_dispatcher_unittest.cc:215: base::TimeTicks::Now() - send_started_time; On 2015/01/21 01:35:39, Wez wrote: > nit: Shouldn't this calculation take place immediately before the > packet-processed closure is run? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/host_video... File remoting/protocol/host_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/host_video... remoting/protocol/host_video_dispatcher.cc:53: const base::Closure& done) { On 2015/01/21 01:35:40, Wez wrote: > Need some kind of comment and bug # reference in here to explain that we'll have > some flow-control logic in here eventually. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/host_video... remoting/protocol/host_video_dispatcher.cc:62: pending_frames_.front().done.Run(); On 2015/01/21 01:35:40, Wez wrote: > Are you guaranteed that the callback won't delete |this|? If not then copy the > callback and pop_front() before you Run() it. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_de... File remoting/protocol/message_decoder.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_de... remoting/protocol/message_decoder.h:38: bool is_buffer_empty() { return buffer_.total_bytes() == 0; } On 2015/01/21 01:35:40, Wez wrote: > Needs a comment to clarify the difference between this and GetNextMessage() > returning nullptr. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... File remoting/protocol/message_reader.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... remoting/protocol/message_reader.cc:97: last_message_started_time_ = now; On 2015/01/21 01:35:40, Wez wrote: > nit: Blank line after this? > > This logic doesn't seem correct; you're not timing from the start of the latest > message being received, but from the read buffer being empty, which is not the > same thing. > > e.g. if I send three messages and received part of the first, then I start this > timer, then suppose I receive the end of the first, and the beginning of the > second, and then a delay and the rest of the second and all of the third? Then > the time I'll report for the third message will be the total time to receive all > three, which is wrong. Yes, I noticed it myself too. Fixed now and updated the unittest for this case (3 message as you described). BTW, a cleaner way to implement this feature would be to add time tracking in CompoundBuffer, so that it would know the time each byte was received. But I don't think it's worth to add it there given that it's not likely to be useful in other places. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... File remoting/protocol/message_reader.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... remoting/protocol/message_reader.h:49: base::TimeDelta last_message_read_delay() { On 2015/01/21 01:35:40, Wez wrote: > nit: Add a brief comment to explain what this means. > nit: It's not so much a delay as a duration, or an elapsed time? Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/message_re... remoting/protocol/message_reader.h:82: base::TimeDelta last_message_read_delay_; On 2015/01/21 01:35:40, Wez wrote: > nit: Suggest adding a block comment e.g. "Used to track the time taken to > receive each message." Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... File remoting/protocol/session_config.cc (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... remoting/protocol/session_config.cc:190: ChannelConfig::CODEC_VP8)); On 2015/01/21 01:35:40, Wez wrote: > This looks like you're enabling the new video-with-ack all the time. > > Rather than add kVideoWithAckStreamVersion, I'd suggest we switch to having > separate kDefaultVideoVersion, kDefaultEventVersion, kDefaultControlVersion, and > then in this CL an additional kNoAckVideoVersion that we can strip out once all > endpoints have the change. Done. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... File remoting/protocol/session_config.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/session_co... remoting/protocol/session_config.h:18: extern const int kControlStreamVersion; On 2015/01/21 01:35:40, Wez wrote: > Why do we need a separat kControlStreamVersion..? IIRC it was updated a while ago when we added capabilities message. https://codereview.chromium.org/850983002/diff/1/remoting/protocol/video_send... File remoting/protocol/video_sender.h (right): https://codereview.chromium.org/850983002/diff/1/remoting/protocol/video_send... remoting/protocol/video_sender.h:14: class VideoSender : public VideoStub { On 2015/01/21 01:35:40, Wez wrote: > This name is confusing - classes that implement this interface are not senders, > per-se, but receivers/sinks. > > Given that the VideoStub::ProcessVideoPacket() interface already has |done| as a > parameter, why not add the SupportsAck() there? Even though we don't care about > the |done| callback behaviour at the client right now, in principle this > indicator is just as valid there. e.g. it could be used to throttle if the > client has a slow decoder. Done.
On 2015/01/21 01:35:40, Wez wrote: > You're adding fields to the ACKs for the frame rendering delay, and for the > "read delay", but it's not clear from this CL whether they are actually useful. > > Frame rendering delay is a function of decode delay (which essentially limits > frame throughput) and v-blank synchronization (which is really only interesting > if the host is going to try to get in-sync with the client v-blanks to reduce > latency, say). Decode delay also can be useful to limit framerate when we know that the client is slow. E.g. if the decoder always takes more than 50 ms then it doesn't makes sense to capture more frequently and if we try then we will always keep too many frames in the pipe, increasing latency. > > Read delay is in principle the elapsed time between the first and final bytes of > a packet being received, but is meaningless for small VideoPackets due to > packetization leading to zero delays. It might be useful in helping calculate > available bandwidth, given large VideoPackets, but I'd expect the underlying > transport (e.g. PseudoTcp, QUIC, whatever) to be better placed to gauge that, > surely? PseudoTcp can tell us it's current congestion window, but it's very rough estimate of the bandwidth. Also these two numbers allow to calculate round-trip latency of the network: roundtrip_latency = time_between_frame_and_ack - render_delay - read_delay but PseudoTCP and QUIC can also provide this information too. Overall I agree with you, I'm not sure these numbers will ever be useful, but I added them anyway because it would be harder to add them later if we decide that we need them. (the scheduler would need to handle the case when they are not available). I'll take them out if you still think they are useless.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
It's hard to review this without having gone over the overall design. That said, it seems that the additions to VideoStub complicate that interface somewhat, and the co-existence of ACK & non-ACK'd code paths complicates various of the class implementations. Can we simplify things e.g. by breaking VideoScheduler apart into per-thread sub-components for the encoder & capturer, with only the actual scheduling of captures then handled by the VideoScheduler itself? That would help keep the ACK handling better contained, especially if we can manage to break the progress callback out of VideoStub, into its own stub interface that VideoScheduler can implement. WDYT? https://codereview.chromium.org/850983002/diff/80001/remoting/client/software... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/850983002/diff/80001/remoting/client/software... remoting/client/software_video_renderer.cc:379: decode_start, progress_callback); nit: Note that because you're binding this via a WeakPtr to OnPacketDone, the |progress_callback| may not get called if the SWVideoRenderer is torn down between a decode and the OnPacketDone(); is calling code capable of coping with that? https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... remoting/host/capture_scheduler.h:47: // supported frames is considered to be pending until ACK message is received, s/is/are https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... remoting/host/capture_scheduler.h:48: // otherwise it's pending until it's sent (OnFrameSent() is called). If the two things are treated equivalently, simply dependent on whether ACKs are supported, then why not deal with the difference in the lower levels and avoid polluting the CaptureScheduler with both concepts? https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... File remoting/host/capture_scheduler_unittest.cc (right): https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... remoting/host/capture_scheduler_unittest.cc:165: capture_timer_->Fire(); Should you be checking that the timer is running before firing it? https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... File remoting/protocol/video_stub.h (right): https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... remoting/protocol/video_stub.h:24: SENT, Do we really need the "SENT" state? It ends up making this API uglier - "sent" has a reasonably specific meaning, whereas "done" is pretty generic, so it's not clear why we need both. https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... remoting/protocol/video_stub.h:31: const ProgressCallback& progress_callback) = 0; This complicates the VideoStub interface quite a bit; the model I had in mind for these stubs would have a VideoAckStub or VideoSendStatusStub (or some better name;) through which these acknowledgements would be received by the sender, without VideoStub implementations necessarily needing to know about that path - would that do what you need here?
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
PTAL. Now added VideoFeedbackStub instead of changing VideoStub. Also removed the time fields from VideoAcks to keep this CL simpler. Also note that new protocol version is not required anymore. Instead the client uses presence of the frame_id field as indication that the host expects Acks. The host assumes that the client doesn't support Acks unless it receives an Ack message. https://codereview.chromium.org/850983002/diff/80001/remoting/client/software... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/850983002/diff/80001/remoting/client/software... remoting/client/software_video_renderer.cc:379: decode_start, progress_callback); On 2015/02/03 00:54:32, Wez wrote: > nit: Note that because you're binding this via a WeakPtr to OnPacketDone, the > |progress_callback| may not get called if the SWVideoRenderer is torn down > between a decode and the OnPacketDone(); is calling code capable of coping with > that? Yes. These are not like Pepper callbacks that are guaranteed to be called even when the object is destroyed. https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... remoting/host/capture_scheduler.h:47: // supported frames is considered to be pending until ACK message is received, On 2015/02/03 00:54:32, Wez wrote: > s/is/are Removed this function now https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... remoting/host/capture_scheduler.h:48: // otherwise it's pending until it's sent (OnFrameSent() is called). On 2015/02/03 00:54:32, Wez wrote: > If the two things are treated equivalently, simply dependent on whether ACKs are > supported, then why not deal with the difference in the lower levels and avoid > polluting the CaptureScheduler with both concepts? Removed this function now https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... File remoting/host/capture_scheduler_unittest.cc (right): https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... remoting/host/capture_scheduler_unittest.cc:165: capture_timer_->Fire(); On 2015/02/03 00:54:32, Wez wrote: > Should you be checking that the timer is running before firing it? No. MockTimer::Fire() DCHECKs if the timer is not running. https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... File remoting/protocol/video_stub.h (right): https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... remoting/protocol/video_stub.h:24: SENT, On 2015/02/03 00:54:32, Wez wrote: > Do we really need the "SENT" state? It ends up making this API uglier - "sent" > has a reasonably specific meaning, whereas "done" is pretty generic, so it's not > clear why we need both. Reverted this change now. Using VideoFeedbackStub instead. https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... remoting/protocol/video_stub.h:31: const ProgressCallback& progress_callback) = 0; On 2015/02/03 00:54:32, Wez wrote: > This complicates the VideoStub interface quite a bit; the model I had in mind > for these stubs would have a VideoAckStub or VideoSendStatusStub (or some better > name;) through which these acknowledgements would be received by the sender, > without VideoStub implementations necessarily needing to know about that path - > would that do what you need here? Reverted this change now. Using VideoFeedbackStub instead.
On 2015/02/09 19:14:54, Sergey Ulanov wrote: > PTAL. Now added VideoFeedbackStub instead of changing VideoStub. Also removed > the time fields from VideoAcks to keep this CL simpler. > Also note that new protocol version is not required anymore. Instead the client > uses presence of the frame_id field as indication that the host expects Acks. > The host assumes that the client doesn't support Acks unless it receives an Ack > message. > > https://codereview.chromium.org/850983002/diff/80001/remoting/client/software... > File remoting/client/software_video_renderer.cc (right): > > https://codereview.chromium.org/850983002/diff/80001/remoting/client/software... > remoting/client/software_video_renderer.cc:379: decode_start, > progress_callback); > On 2015/02/03 00:54:32, Wez wrote: > > nit: Note that because you're binding this via a WeakPtr to OnPacketDone, the > > |progress_callback| may not get called if the SWVideoRenderer is torn down > > between a decode and the OnPacketDone(); is calling code capable of coping > with > > that? > > Yes. These are not like Pepper callbacks that are guaranteed to be called even > when the object is destroyed. > > https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... > File remoting/host/capture_scheduler.h (right): > > https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... > remoting/host/capture_scheduler.h:47: // supported frames is considered to be > pending until ACK message is received, > On 2015/02/03 00:54:32, Wez wrote: > > s/is/are > > Removed this function now > > https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... > remoting/host/capture_scheduler.h:48: // otherwise it's pending until it's sent > (OnFrameSent() is called). > On 2015/02/03 00:54:32, Wez wrote: > > If the two things are treated equivalently, simply dependent on whether ACKs > are > > supported, then why not deal with the difference in the lower levels and avoid > > polluting the CaptureScheduler with both concepts? > > Removed this function now > > https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... > File remoting/host/capture_scheduler_unittest.cc (right): > > https://codereview.chromium.org/850983002/diff/80001/remoting/host/capture_sc... > remoting/host/capture_scheduler_unittest.cc:165: capture_timer_->Fire(); > On 2015/02/03 00:54:32, Wez wrote: > > Should you be checking that the timer is running before firing it? > > No. MockTimer::Fire() DCHECKs if the timer is not running. > > https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... > File remoting/protocol/video_stub.h (right): > > https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... > remoting/protocol/video_stub.h:24: SENT, > On 2015/02/03 00:54:32, Wez wrote: > > Do we really need the "SENT" state? It ends up making this API uglier - "sent" > > has a reasonably specific meaning, whereas "done" is pretty generic, so it's > not > > clear why we need both. > > Reverted this change now. Using VideoFeedbackStub instead. > > https://codereview.chromium.org/850983002/diff/80001/remoting/protocol/video_... > remoting/protocol/video_stub.h:31: const ProgressCallback& progress_callback) = > 0; > On 2015/02/03 00:54:32, Wez wrote: > > This complicates the VideoStub interface quite a bit; the model I had in mind > > for these stubs would have a VideoAckStub or VideoSendStatusStub (or some > better > > name;) through which these acknowledgements would be received by the sender, > > without VideoStub implementations necessarily needing to know about that path > - > > would that do what you need here? > > Reverted this change now. Using VideoFeedbackStub instead. Looks like VideoFeedbackStub is missing from the CL?
https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:100: packet->set_frame_id(next_frame_id_); Do we want to do that if ACKs are disabled? https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:108: ++num_unacknowledged_frames_; This will grow unbounded if ACKs aren't enabled, right? https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:125: acks_supported_ = true; It seems strange to have this depend upon whether we ever receive an ACK, rather than on the version of the protocol that the client chose. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.h:30: // acknowledged (currently the limit is set to 4 frames). Otherwise it limits Explain this frame-count - e.g. is it chosen to allow some pipelining while maintaining no more than 132ms latency (132ms = 4 * 33ms, which seems like an awful lot of latency to allow) https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.h:112: uint32_t next_frame_id_; nit: Explain this member! https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.proto File remoting/proto/video.proto (right): https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.pr... remoting/proto/video.proto:77: // the corresponding VideoPackets were received. Note that if we move to an unordered transport then you can't really guarantee that, so consider whether you really need this guarantee! https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.pr... remoting/proto/video.proto:78: message VideoAck { nit: You could almost call this VideoFrameFeedback, since you were planning on adding all sorts of fun stuff to it. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:51: base::Unretained(this), pending_frame)); Why do we only want to ACK after the frame has been processed? What does that even mean in terms of client-side processing? https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:53: done.Run(); ScopedTaskRunner for this? https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:57: PendingFramesList::iterator pending_frame) { You're passing in an iterator into the list, but the loop below potentially pops multiple items out of the list, so you've no guarantee that the iterator is valid... https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:63: while (!pending_frames_.empty() && pending_frames_.front().done) { Why do you need this loop? Surely you'll already have ACK'd and pop'd those other frames if you reach here? https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher_unittest.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher_unittest.cc:26: ClientVideoDispatcherTest() nit: Move the function bodies out of the class declaration so they'll be more readable.
On 2015/02/11 01:43:25, Wez wrote: > Looks like VideoFeedbackStub is missing from the CL? Sorry about it - added it now. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:100: packet->set_frame_id(next_frame_id_); On 2015/02/11 02:22:55, Wez wrote: > Do we want to do that if ACKs are disabled? Yes. In the current version of this CL protocol version stays the same. The idea is that the host always sets this field to indicate to the client that it expects ACKs. And then it assumes that the client doesn't support ACKs until it receives the first ACK. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:108: ++num_unacknowledged_frames_; On 2015/02/11 02:22:55, Wez wrote: > This will grow unbounded if ACKs aren't enabled, right? Right. And then it's ignored in ScheduleNextCapture(), unless we know that client supports ACKs. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:125: acks_supported_ = true; On 2015/02/11 02:22:55, Wez wrote: > It seems strange to have this depend upon whether we ever receive an ACK, rather > than on the version of the protocol that the client chose. See my comment in OnFrameEncoded(). It makes this CL simpler. Added a comment here. If you are against this approach then I will add new video channel version back. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.h:30: // acknowledged (currently the limit is set to 4 frames). Otherwise it limits On 2015/02/11 02:22:55, Wez wrote: > Explain this frame-count - e.g. is it chosen to allow some pipelining while > maintaining no more than 132ms latency (132ms = 4 * 33ms, which seems like an > awful lot of latency to allow) This limit is for full roundtrip, including ACK message delivery, while we care about latency only in one direction. So 132ms will be observed in the case when you have low-bandwidth network with 0 latency, which is not realistic. I removed this comment from here and added comments in capture_scheduler.cc about this choice. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.h:112: uint32_t next_frame_id_; On 2015/02/11 02:22:56, Wez wrote: > nit: Explain this member! Done. https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.proto File remoting/proto/video.proto (right): https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.pr... remoting/proto/video.proto:77: // the corresponding VideoPackets were received. On 2015/02/11 02:22:56, Wez wrote: > Note that if we move to an unordered transport then you can't really guarantee > that, so consider whether you really need this guarantee! It will be easy to remove this requirement for unordered transport. https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.pr... remoting/proto/video.proto:78: message VideoAck { On 2015/02/11 02:22:56, Wez wrote: > nit: You could almost call this VideoFrameFeedback, since you were planning on > adding all sorts of fun stuff to it. I think VideoAck is fine for now - it can be renamed when adding the fun stuff. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:51: base::Unretained(this), pending_frame)); On 2015/02/11 02:22:56, Wez wrote: > Why do we only want to ACK after the frame has been processed? What does that > even mean in terms of client-side processing? If the renderer is slow/blocked we want the host to stop sending frames. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:53: done.Run(); On 2015/02/11 02:22:56, Wez wrote: > ScopedTaskRunner for this? Done. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:57: PendingFramesList::iterator pending_frame) { On 2015/02/11 02:22:56, Wez wrote: > You're passing in an iterator into the list, but the loop below potentially pops > multiple items out of the list, so you've no guarantee that the iterator is > valid... std::list<> iterators remain valid even when the list mutated, as long as the element the iterator refers to is still in the list. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:63: while (!pending_frames_.empty() && pending_frames_.front().done) { On 2015/02/11 02:22:56, Wez wrote: > Why do you need this loop? Surely you'll already have ACK'd and pop'd those > other frames if you reach here? VideoAck messages should be sent in the order VideoFrame messages are received, but VideoStub is allowed to process them in any order. So for Done callback called out of order we just set |done| flag and send the ACK only after all messages before it are marked as done https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher_unittest.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher_unittest.cc:26: ClientVideoDispatcherTest() On 2015/02/11 02:22:56, Wez wrote: > nit: Move the function bodies out of the class declaration so they'll be more > readable. Done.
ping
https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:125: acks_supported_ = true; On 2015/02/17 19:37:06, Sergey Ulanov wrote: > On 2015/02/11 02:22:55, Wez wrote: > > It seems strange to have this depend upon whether we ever receive an ACK, > rather > > than on the version of the protocol that the client chose. > > See my comment in OnFrameEncoded(). It makes this CL simpler. Added a comment > here. If you are against this approach then I will add new video channel version > back. It makes the CL simpler but arguably makes the resulting code more complex. :P I think this approach is OK, though, provided we have a bug & TODO for cleaning out the ACK-less paths once everything has been updated. https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.h:30: // acknowledged (currently the limit is set to 4 frames). Otherwise it limits On 2015/02/17 19:37:06, Sergey Ulanov wrote: > On 2015/02/11 02:22:55, Wez wrote: > > Explain this frame-count - e.g. is it chosen to allow some pipelining while > > maintaining no more than 132ms latency (132ms = 4 * 33ms, which seems like an > > awful lot of latency to allow) > > This limit is for full roundtrip, including ACK message delivery, while we care > about latency only in one direction. So 132ms will be observed in the case when > you have low-bandwidth network with 0 latency, which is not realistic. I removed > this comment from here and added comments in capture_scheduler.cc about this > choice. Comment still seems to be here? https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.proto File remoting/proto/video.proto (right): https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.pr... remoting/proto/video.proto:77: // the corresponding VideoPackets were received. On 2015/02/17 19:37:06, Sergey Ulanov wrote: > On 2015/02/11 02:22:56, Wez wrote: > > Note that if we move to an unordered transport then you can't really guarantee > > that, so consider whether you really need this guarantee! > > It will be easy to remove this requirement for unordered transport. Acknowledged. https://codereview.chromium.org/850983002/diff/140001/remoting/proto/video.pr... remoting/proto/video.proto:78: message VideoAck { On 2015/02/17 19:37:06, Sergey Ulanov wrote: > On 2015/02/11 02:22:56, Wez wrote: > > nit: You could almost call this VideoFrameFeedback, since you were planning on > > adding all sorts of fun stuff to it. > > I think VideoAck is fine for now - it can be renamed when adding the fun stuff. Acknowledged. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:51: base::Unretained(this), pending_frame)); On 2015/02/17 19:37:07, Sergey Ulanov wrote: > On 2015/02/11 02:22:56, Wez wrote: > > Why do we only want to ACK after the frame has been processed? What does that > > even mean in terms of client-side processing? > > If the renderer is slow/blocked we want the host to stop sending frames. Acknowledged. https://codereview.chromium.org/850983002/diff/140001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher.cc:63: while (!pending_frames_.empty() && pending_frames_.front().done) { On 2015/02/17 19:37:07, Sergey Ulanov wrote: > On 2015/02/11 02:22:56, Wez wrote: > > Why do you need this loop? Surely you'll already have ACK'd and pop'd those > > other frames if you reach here? > > VideoAck messages should be sent in the order VideoFrame messages are received, > but VideoStub is allowed to process them in any order. So for Done callback > called out of order we just set |done| flag and send the ACK only after all > messages before it are marked as done Ahhhh, OK, yes I see what you mean. SGTM https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... File remoting/host/capture_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:33: // Maximum number of frames that haven't been acknowledged. It's used only when nit: Suggest "Maximum number of unacknowledged frames. Ignored if the client does not support ACKs." https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:34: // the client support ACKs. This values was chosen experimentally, using nit: "This" doesn't go with "values" - "This value" or "These values" https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:39: // Maximum number of frames that can be processed simultaneously. It's used only Need to be clear about where this limit comes from, i.e. what does "processing" a frame mean? Is it "processed" once we get the send completion, or something else? https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.h:112: // frame_id to be assigned to the next outgoing video frame. nit: Either "Frame Id to be ..." or "|frame_id| to be ..." https://codereview.chromium.org/850983002/diff/160001/remoting/host/video_fra... File remoting/host/video_frame_pump.h (right): https://codereview.chromium.org/850983002/diff/160001/remoting/host/video_fra... remoting/host/video_frame_pump.h:120: base::ThreadChecker thread_checker_; nit: I'd suggest putting this just before the WeakPtrFactory, since it's not really part of the core logic of this class, just a tool for sanity-checking the class' behaviour. https://codereview.chromium.org/850983002/diff/160001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher_unittest.cc (right): https://codereview.chromium.org/850983002/diff/160001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher_unittest.cc:84: // VideoStub interface. No need to duplicate the class declaration comments for this or OnChannelInitialized.
https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:125: acks_supported_ = true; On 2015/02/21 03:12:02, Wez wrote: > On 2015/02/17 19:37:06, Sergey Ulanov wrote: > > On 2015/02/11 02:22:55, Wez wrote: > > > It seems strange to have this depend upon whether we ever receive an ACK, > > rather > > > than on the version of the protocol that the client chose. > > > > See my comment in OnFrameEncoded(). It makes this CL simpler. Added a comment > > here. If you are against this approach then I will add new video channel > version > > back. > > It makes the CL simpler but arguably makes the resulting code more complex. :P > > I think this approach is OK, though, provided we have a bug & TODO for cleaning > out the ACK-less paths once everything has been updated. Opened crbug.com/460963 https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/140001/remoting/host/capture_s... remoting/host/capture_scheduler.h:30: // acknowledged (currently the limit is set to 4 frames). Otherwise it limits On 2015/02/21 03:12:02, Wez wrote: > On 2015/02/17 19:37:06, Sergey Ulanov wrote: > > On 2015/02/11 02:22:55, Wez wrote: > > > Explain this frame-count - e.g. is it chosen to allow some pipelining while > > > maintaining no more than 132ms latency (132ms = 4 * 33ms, which seems like > an > > > awful lot of latency to allow) > > > > This limit is for full roundtrip, including ACK message delivery, while we > care > > about latency only in one direction. So 132ms will be observed in the case > when > > you have low-bandwidth network with 0 latency, which is not realistic. I > removed > > this comment from here and added comments in capture_scheduler.cc about this > > choice. > > Comment still seems to be here? Sorry. Cleaned up the comment now. https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... File remoting/host/capture_scheduler.cc (right): https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:33: // Maximum number of frames that haven't been acknowledged. It's used only when On 2015/02/21 03:12:02, Wez wrote: > nit: Suggest "Maximum number of unacknowledged frames. Ignored if the client > does not support ACKs." Done. https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:34: // the client support ACKs. This values was chosen experimentally, using On 2015/02/21 03:12:02, Wez wrote: > nit: "This" doesn't go with "values" - "This value" or "These values" Done. https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.cc:39: // Maximum number of frames that can be processed simultaneously. It's used only On 2015/02/21 03:12:02, Wez wrote: > Need to be clear about where this limit comes from, i.e. what does "processing" > a frame mean? Is it "processed" once we get the send completion, or something > else? This is a legacy value and will need to be removed in the future. Added TODO to remove it. https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/160001/remoting/host/capture_s... remoting/host/capture_scheduler.h:112: // frame_id to be assigned to the next outgoing video frame. On 2015/02/21 03:12:02, Wez wrote: > nit: Either "Frame Id to be ..." or "|frame_id| to be ..." Done. https://codereview.chromium.org/850983002/diff/160001/remoting/host/video_fra... File remoting/host/video_frame_pump.h (right): https://codereview.chromium.org/850983002/diff/160001/remoting/host/video_fra... remoting/host/video_frame_pump.h:120: base::ThreadChecker thread_checker_; On 2015/02/21 03:12:02, Wez wrote: > nit: I'd suggest putting this just before the WeakPtrFactory, since it's not > really part of the core logic of this class, just a tool for sanity-checking the > class' behaviour. Done. https://codereview.chromium.org/850983002/diff/160001/remoting/protocol/clien... File remoting/protocol/client_video_dispatcher_unittest.cc (right): https://codereview.chromium.org/850983002/diff/160001/remoting/protocol/clien... remoting/protocol/client_video_dispatcher_unittest.cc:84: // VideoStub interface. On 2015/02/21 03:12:02, Wez wrote: > No need to duplicate the class declaration comments for this or > OnChannelInitialized. Done.
lgtm https://codereview.chromium.org/850983002/diff/180001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/180001/remoting/host/capture_s... remoting/host/capture_scheduler.h:28: // - Limit CPU usage. nit: Specificy that it limits to less-than some target %age? Maximizing the frame rate isn't quite a goal; ideally we want a fixed frame rate that matches what the client can display, e.g. 60fps, but we also want to balance capture vs encode overhead. It might be clearer to say that the intention is to parallelise capture, encode and transmission, to achieve as close to a target frame rate as possible.
https://codereview.chromium.org/850983002/diff/180001/remoting/host/capture_s... File remoting/host/capture_scheduler.h (right): https://codereview.chromium.org/850983002/diff/180001/remoting/host/capture_s... remoting/host/capture_scheduler.h:28: // - Limit CPU usage. On 2015/02/23 21:52:17, Wez wrote: > nit: Specificy that it limits to less-than some target %age? Done. > > Maximizing the frame rate isn't quite a goal; ideally we want a fixed frame rate > that matches what the client can display, e.g. 60fps, but we also want to > balance capture vs encode overhead. > > It might be clearer to say that the intention is to parallelise capture, encode > and transmission, to achieve as close to a target frame rate as possible. Done (BTW currently we limit frame-rate to 30fps, not 60)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/850983002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/850983002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/850983002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/850983002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/850983002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/850983002/240001
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/97568a816ed5fe0a69a2c0f6d0987381899f58d8 Cr-Commit-Position: refs/heads/master@{#317824} |
