Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156713002:
[Chromoting] Implement DualBufferFrameConsumer (Closed)
|
Created:
4 years, 5 months ago by Yuwei Modified:
4 years, 5 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, posciak+watch_chromium.org, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromoting] Implement DualBufferFrameConsumer
This class uses two buffers to decode mutiple frames. It will be used
by the chromoting client OpenGL renderer.
BUG=385924
Committed: https://crrev.com/6d9bb4a011a2df6509c5d50ea7efc3e55f671980
Cr-Commit-Position: refs/heads/master@{#407233}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Use dual buffers for frame update #Patch Set 3 : Make SoftwareVideoRenderer able to own FrameConsumer #Patch Set 4 : Change last_desktop_size_ only when it is changed. #
Total comments: 17
Patch Set 5 : Reviewer's Feedback #Patch Set 6 : Remove deprecated SoftwareVideoRenderer ctor #
Total comments: 16
Patch Set 7 : Reviewer's Feedback #Patch Set 8 : Merge ToT #Patch Set 9 : Fix null closure bug #Patch Set 10 : Pass RenderCallback by const ref #
Total comments: 2
Patch Set 11 : Reviewer's Feedback #
Messages
Total messages: 43 (20 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
ptal https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:23: // There is no synchronization to prevent screen tearing if decoding and I'm not sure whether screen tearing is a concern I need to fix. Looks like the host capturer also occasionally produces screen tearing...
https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.cc:63: return underlying_video_renderer_->GetFrameStatsConsumer(); How will this class get any frames, if you return underlying_video_renderer_->GetFrameStatsConsumer() here instead of this? https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:23: // There is no synchronization to prevent screen tearing if decoding and On 2016/07/15 20:57:12, Yuwei wrote: > I'm not sure whether screen tearing is a concern I need to fix. Looks like the > host capturer also occasionally produces screen tearing... Ideally we want to avoid it. ScreenCaptures don't have this problem. They use a set of two buffers and the consumer is allowed to hold only on of them at a time. It should be possible to implement the same approach with two rotating buffers. https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:28: class SharedFrameVideoRenderer : public protocol::FrameConsumer, Do you actually need to implement VideoRenderer interface? As far as I can tell you just want to have a FrameConsumer that reuses the same buffer. SoftwareVideoRenderer can be used with any FrameConsumer.
https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.cc:63: return underlying_video_renderer_->GetFrameStatsConsumer(); On 2016/07/15 22:14:47, Sergey Ulanov wrote: > How will this class get any frames, if you return > underlying_video_renderer_->GetFrameStatsConsumer() here instead of this? I think you are talking about GetFrameConsumer. I'm assuming SoftwareVideoRenderer is being used. You will do SoftwareVideoRenderer(shared_frame_renderer_) when creating the SoftwareVideoRenderer and SoftwareVideoRenderer::GetFrameConsumer() will just return |this| in this case... https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:23: // There is no synchronization to prevent screen tearing if decoding and On 2016/07/15 22:14:48, Sergey Ulanov wrote: > On 2016/07/15 20:57:12, Yuwei wrote: > > I'm not sure whether screen tearing is a concern I need to fix. Looks like the > > host capturer also occasionally produces screen tearing... > > Ideally we want to avoid it. ScreenCaptures don't have this problem. They use a > set of two buffers and the consumer is allowed to hold only on of them at a > time. > It should be possible to implement the same approach with two rotating buffers. It will work if we just use the buffers to store updated regions and simply use glTexSubImage2d to upload only the updated regions. The weird problem is that in some cases like the phone get rotated during a session, the OpenGL surface and context will be destroyed and recreated, and the GPU memory will lose the current desktop frame. Not sure how we can restore current frame in that case... https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:28: class SharedFrameVideoRenderer : public protocol::FrameConsumer, On 2016/07/15 22:14:48, Sergey Ulanov wrote: > Do you actually need to implement VideoRenderer interface? > As far as I can tell you just want to have a FrameConsumer that reuses the same > buffer. SoftwareVideoRenderer can be used with any FrameConsumer. This is basically managing to give VideoRenderer and FrameConsumer the same lifecycle. The problem here is SoftwareVideoRenderer takes in a raw pointer to the FrameConsumer meaning that the renderer must not outlive FrameConsumer. When the caller calls DisplayUpdaterFactory::CreateVideoRenderer(), they would expect that the video renderer's lifecycle is independent on any unexposed object... I'm not sure whether is it possible or flexible enough to make SoftwareVideoRenderer own the FrameConsumer or take a WeakPtr of it...
https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:23: // There is no synchronization to prevent screen tearing if decoding and On 2016/07/15 22:55:32, Yuwei wrote: > On 2016/07/15 22:14:48, Sergey Ulanov wrote: > > On 2016/07/15 20:57:12, Yuwei wrote: > > > I'm not sure whether screen tearing is a concern I need to fix. Looks like > the > > > host capturer also occasionally produces screen tearing... > > > > Ideally we want to avoid it. ScreenCaptures don't have this problem. They use > a > > set of two buffers and the consumer is allowed to hold only on of them at a > > time. > > It should be possible to implement the same approach with two rotating > buffers. > > It will work if we just use the buffers to store updated regions and simply use > glTexSubImage2d to upload only the updated regions. > > The weird problem is that in some cases like the phone get rotated during a > session, the OpenGL surface and context will be destroyed and recreated, and the > GPU memory will lose the current desktop frame. Not sure how we can restore > current frame in that case... Hmm... I think it is possible to keep track of rectangle areas that are in buffer 2 not in buffer 1. In that way we can restore the current frame easily. Is there already efficient code for merging rectangles?
https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:23: // There is no synchronization to prevent screen tearing if decoding and On 2016/07/16 02:24:56, Yuwei wrote: > On 2016/07/15 22:55:32, Yuwei wrote: > > On 2016/07/15 22:14:48, Sergey Ulanov wrote: > > > On 2016/07/15 20:57:12, Yuwei wrote: > > > > I'm not sure whether screen tearing is a concern I need to fix. Looks like > > the > > > > host capturer also occasionally produces screen tearing... > > > > > > Ideally we want to avoid it. ScreenCaptures don't have this problem. They > use > > a > > > set of two buffers and the consumer is allowed to hold only on of them at a > > > time. > > > It should be possible to implement the same approach with two rotating > > buffers. > > > > It will work if we just use the buffers to store updated regions and simply > use > > glTexSubImage2d to upload only the updated regions. > > > > The weird problem is that in some cases like the phone get rotated during a > > session, the OpenGL surface and context will be destroyed and recreated, and > the > > GPU memory will lose the current desktop frame. Not sure how we can restore > > current frame in that case... > > Hmm... I think it is possible to keep track of rectangle areas that are in > buffer 2 not in buffer 1. In that way we can restore the current frame easily. > Is there already efficient code for merging rectangles? NVM... DesktopRegion::AddRect and DesktopRegion::Subtract should be good enough...
Description was changed from ========== [Chromoting] Implement SharedFrameSoftwareVideoRenderer This class uses single buffer to decode mutiple frames. It will be used by the chromoting client OpenGL renderer. BUG=385924 ========== to ========== [Chromoting] Implement DualBufferFrameConsumer This class uses two buffers to decode mutiple frames. It will be used by the chromoting client OpenGL renderer. BUG=385924 ==========
ptal https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.cc:63: return underlying_video_renderer_->GetFrameStatsConsumer(); On 2016/07/15 22:14:47, Sergey Ulanov wrote: > How will this class get any frames, if you return > underlying_video_renderer_->GetFrameStatsConsumer() here instead of this? Obsolete. https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:23: // There is no synchronization to prevent screen tearing if decoding and On 2016/07/15 22:14:48, Sergey Ulanov wrote: > On 2016/07/15 20:57:12, Yuwei wrote: > > I'm not sure whether screen tearing is a concern I need to fix. Looks like the > > host capturer also occasionally produces screen tearing... > > Ideally we want to avoid it. ScreenCaptures don't have this problem. They use a > set of two buffers and the consumer is allowed to hold only on of them at a > time. > It should be possible to implement the same approach with two rotating buffers. Done. Used dual buffers. https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.h:28: class SharedFrameVideoRenderer : public protocol::FrameConsumer, On 2016/07/15 22:14:48, Sergey Ulanov wrote: > Do you actually need to implement VideoRenderer interface? > As far as I can tell you just want to have a FrameConsumer that reuses the same > buffer. SoftwareVideoRenderer can be used with any FrameConsumer. Done. Made SoftwareVideoRenderer able to own a FrameConsumer. Now we are able to turn JniVideoRenderer back to JniFrameConsumer. I'll do a follow up CL for this.
https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:77: if ((reinterpret_cast<webrtc::SharedDesktopFrame*> (frame.get())) This assumes |frame| must come from AllocateFrame. Looks like there is no documented restrictions in FrameConsumer but I think this must be true?
https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.cc:63: return underlying_video_renderer_->GetFrameStatsConsumer(); On 2016/07/15 22:55:32, Yuwei wrote: > On 2016/07/15 22:14:47, Sergey Ulanov wrote: > > How will this class get any frames, if you return > > underlying_video_renderer_->GetFrameStatsConsumer() here instead of this? > > I think you are talking about GetFrameConsumer. I'm assuming > SoftwareVideoRenderer is being used. You will do > SoftwareVideoRenderer(shared_frame_renderer_) when creating the > SoftwareVideoRenderer and SoftwareVideoRenderer::GetFrameConsumer() will just > return |this| in this case... I see. Then this class needs to be just a FrameConsumer, not a VideoRenderer. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:58: buffer_0_ = webrtc::SharedDesktopFrame::Wrap( add comment that both buffers are reallocated whenever screen size changes. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:65: next_buffer_ = (next_buffer_ + 1) % 2; I think you want to synchronize buffers here (or maybe somewhere else). Otherwise you end up rendering changes between frames N+1 and N+2 on frame N, i.e. the changes between N and N+1 and not included in what you get in the end. And to implement it you need to save updated_region() from the frame you get in DrawFrame() somewhere. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:66: if (next_buffer_ == 0) { If you replace buffer_?_ with buffers_[] then this would be 'return buffers_[(next_buffer_+1)%2]->Share();' https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:77: if ((reinterpret_cast<webrtc::SharedDesktopFrame*> (frame.get())) On 2016/07/18 22:38:35, Yuwei wrote: > This assumes |frame| must come from AllocateFrame. Looks like there is no > documented restrictions in FrameConsumer but I think this must be true? Yes, this is a valid assumption. Caller must use AllocateFrame() to allocate frames. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.h (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.h:19: } // namespace webrtc https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... remoting/client/software_video_renderer.cc:55: : owned_consumer_(std::move(consumer)), nit: I think it's better to call the other constructor here and then initialize owner_consumer_ in the body of the constructor. That would allow to avoid duplication of the initializer list and the DetachFromThread() call. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... File remoting/client/software_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... remoting/client/software_video_renderer.h:47: // std::unique_ptr version constructor that owns the consumer. Suggest rewording: "Same as above, but takes ownership of the |consumer|." https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... remoting/client/software_video_renderer.h:58: SoftwareVideoRenderer( Please remove this constructor. It's used only in tests, which can be easily updated to use Initialize()
https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... File remoting/client/shared_frame_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/40001/remoting/client/shared_... remoting/client/shared_frame_video_renderer.cc:63: return underlying_video_renderer_->GetFrameStatsConsumer(); On 2016/07/20 18:39:26, Sergey Ulanov wrote: > On 2016/07/15 22:55:32, Yuwei wrote: > > On 2016/07/15 22:14:47, Sergey Ulanov wrote: > > > How will this class get any frames, if you return > > > underlying_video_renderer_->GetFrameStatsConsumer() here instead of this? > > > > I think you are talking about GetFrameConsumer. I'm assuming > > SoftwareVideoRenderer is being used. You will do > > SoftwareVideoRenderer(shared_frame_renderer_) when creating the > > SoftwareVideoRenderer and SoftwareVideoRenderer::GetFrameConsumer() will just > > return |this| in this case... > > I see. Then this class needs to be just a FrameConsumer, not a VideoRenderer. Yes. It has been renamed... https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:65: next_buffer_ = (next_buffer_ + 1) % 2; On 2016/07/20 18:39:26, Sergey Ulanov wrote: > I think you want to synchronize buffers here (or maybe somewhere else). > Otherwise you end up rendering changes between frames N+1 and N+2 on frame N, Do you mean that when rendering is slower than decoding, then when the next rendering happens, it may end up rendering the buffer when it is being updated? I think this may just cause screen tearing very rarely. We can add locks for this if we like... > i.e. the changes between N and N+1 and not included in what you get in the end. > And to implement it you need to save updated_region() from the frame you get in > DrawFrame() somewhere. I think this is fine. SharedDesktopFrame will only share the buffer, and each SharedDesktopFrame will have its own updated_region(). Uploading a region that is being updated between [N+1, N+2] will just cause screen tearing. Other than that I don't think having part of [N+1, N+2] image for the updated_region in [N, N+1] iteration will accumulate error.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
ptal https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:58: buffer_0_ = webrtc::SharedDesktopFrame::Wrap( On 2016/07/20 18:39:26, Sergey Ulanov wrote: > add comment that both buffers are reallocated whenever screen size changes. Done. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:66: if (next_buffer_ == 0) { On 2016/07/20 18:39:26, Sergey Ulanov wrote: > If you replace buffer_?_ with buffers_[] then this would be 'return > buffers_[(next_buffer_+1)%2]->Share();' Done. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:77: if ((reinterpret_cast<webrtc::SharedDesktopFrame*> (frame.get())) On 2016/07/20 18:39:26, Sergey Ulanov wrote: > On 2016/07/18 22:38:35, Yuwei wrote: > > This assumes |frame| must come from AllocateFrame. Looks like there is no > > documented restrictions in FrameConsumer but I think this must be true? > > Yes, this is a valid assumption. Caller must use AllocateFrame() to allocate > frames. Acknowledged. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.h (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.h:19: } On 2016/07/20 18:39:26, Sergey Ulanov wrote: > // namespace webrtc Done. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... remoting/client/software_video_renderer.cc:55: : owned_consumer_(std::move(consumer)), On 2016/07/20 18:39:26, Sergey Ulanov wrote: > nit: I think it's better to call the other constructor here and then initialize > owner_consumer_ in the body of the constructor. That would allow to avoid > duplication of the initializer list and the DetachFromThread() call. Done. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... File remoting/client/software_video_renderer.h (right): https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... remoting/client/software_video_renderer.h:47: // std::unique_ptr version constructor that owns the consumer. On 2016/07/20 18:39:26, Sergey Ulanov wrote: > Suggest rewording: "Same as above, but takes ownership of the |consumer|." Done. https://codereview.chromium.org/2156713002/diff/100001/remoting/client/softwa... remoting/client/software_video_renderer.h:58: SoftwareVideoRenderer( On 2016/07/20 18:39:26, Sergey Ulanov wrote: > Please remove this constructor. It's used only in tests, which can be easily > updated to use Initialize() Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.h (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.h:60: webrtc::DesktopRegion buffer_1_mask_; I'll add some comment here.
Thanks for explaining how RequestFullDesktopFrame() works. I think the code looks good. Can you please add a unittest? https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:39: std::unique_ptr<webrtc::DesktopFrame> full_frame( Add some comments to explain what happens here. https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:45: CopyRGB32Rect(buffers_[0]->data(), buffers_[1]->stride(), desktop_rect, Please use DesktopFrame::CopyPixelsFrom(). https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:84: // This may happen if the resolution is changed but a previous shared frame Currently SoftwareVideoRenderer never processes more than one frame at a time, so you can replace this with NOTREACHED(). (also if the renderer did try decoding multiple frames in parallel you wouldn't want to drop this frame here - it's still better to render it before the newer frames with updated size are decoded.) https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:109: if (done) { I don't think you need this check. DrawFrame() should only be called with a non-null done callback. https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:110: done.Run(); We cannot assume that the frame have been rendered here. You want to pass that done closure to callback_ so it's called when the frame is actually rendered (i.e. after eglSwapBuffer() has returned). That callback is used to measure rendering latency.
https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:110: done.Run(); On 2016/07/21 19:14:26, Sergey Ulanov wrote: > We cannot assume that the frame have been rendered here. You want to pass that > done closure to callback_ so it's called when the frame is actually rendered > (i.e. after eglSwapBuffer() has returned). That callback is used to measure > rendering latency. I guess the measurement will be messed up if another frame comes when the first frame is being uploaded and then eglSwapBuffer is run after the second frame is rendered?
https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:110: done.Run(); On 2016/07/21 20:00:41, Yuwei wrote: > On 2016/07/21 19:14:26, Sergey Ulanov wrote: > > We cannot assume that the frame have been rendered here. You want to pass that > > done closure to callback_ so it's called when the frame is actually rendered > > (i.e. after eglSwapBuffer() has returned). That callback is used to measure > > rendering latency. > > I guess the measurement will be messed up if another frame comes when the first > frame is being uploaded and then eglSwapBuffer is run after the second frame is > rendered? To make it work correctly the renderer will need to keep queue of callbacks for all pending frames and call them after eglSwapBuffer() completes. In other words if a frame dropped before being rendered then it's considered to be rendered only when the next frame is rendered. That's what we have in PepperVideoRenderer3D.
On 2016/07/21 20:22:48, Sergey Ulanov wrote: > https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... > File remoting/client/dual_buffer_frame_consumer.cc (right): > > https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... > remoting/client/dual_buffer_frame_consumer.cc:110: done.Run(); > On 2016/07/21 20:00:41, Yuwei wrote: > > On 2016/07/21 19:14:26, Sergey Ulanov wrote: > > > We cannot assume that the frame have been rendered here. You want to pass > that > > > done closure to callback_ so it's called when the frame is actually rendered > > > (i.e. after eglSwapBuffer() has returned). That callback is used to measure > > > rendering latency. > > > > I guess the measurement will be messed up if another frame comes when the > first > > frame is being uploaded and then eglSwapBuffer is run after the second frame > is > > rendered? > > To make it work correctly the renderer will need to keep queue of callbacks for > all pending frames and call them after eglSwapBuffer() completes. In other words > if a frame dropped before being rendered then it's considered to be rendered > only when the next frame is rendered. That's what we have in > PepperVideoRenderer3D. I think this works for average rendering time but a dropped frame will cause a spike in max rendering time. (Maybe this is expected?) Also I will expect some noise in the data since the display thread is also used for cursor shape updates and other UI things...
On 2016/07/21 20:49:33, Yuwei wrote: > I think this works for average rendering time but a dropped frame will cause a > spike in max rendering time. (Maybe this is expected?) > > Also I will expect some noise in the data since the display thread is also used > for cursor shape updates and other UI things... Not sure what's the problem here. We don't expect render time to be the same for all frames.
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
ptal On 2016/07/21 20:49:33, Yuwei wrote: > I think this works for average rendering time but a dropped frame will cause a > spike in max rendering time. (Maybe this is expected?) > > Also I will expect some noise in the data since the display thread is also used > for cursor shape updates and other UI things... Not sure what's the problem here. We don't expect render time to be the same for all frames. Just feel slightly confusing that the latency you get for a dropped frame will actually include latency of all other pending frames to be drawn... https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:39: std::unique_ptr<webrtc::DesktopFrame> full_frame( On 2016/07/21 19:14:25, Sergey Ulanov wrote: > Add some comments to explain what happens here. Done. https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:45: CopyRGB32Rect(buffers_[0]->data(), buffers_[1]->stride(), desktop_rect, On 2016/07/21 19:14:26, Sergey Ulanov wrote: > Please use DesktopFrame::CopyPixelsFrom(). Done. https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:84: // This may happen if the resolution is changed but a previous shared frame On 2016/07/21 19:14:25, Sergey Ulanov wrote: > Currently SoftwareVideoRenderer never processes more than one frame at a time, > so you can replace this with NOTREACHED(). (also if the renderer did try > decoding multiple frames in parallel you wouldn't want to drop this frame here - > it's still better to render it before the newer frames with updated size are > decoded.) Removed the else branch. The problem is that SoftwareVideoRenderer::ProcessVideoPacket calls AllocateFrame and DrawFrame asynchronously. NOTREACHED() will be problematic if this happens: 1. Allocate frame A 2. Allocate frame B, resolution changed 3. Draw frame A 4. Draw frame B Step 3 will hit NOTREACHED() since its underlying buffer is already replaced in DualBufferFrameConsumer... https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:109: if (done) { On 2016/07/21 19:14:26, Sergey Ulanov wrote: > I don't think you need this check. DrawFrame() should only be called with a > non-null done callback. Done. Removed. https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:110: done.Run(); On 2016/07/21 20:22:48, Sergey Ulanov wrote: > On 2016/07/21 20:00:41, Yuwei wrote: > > On 2016/07/21 19:14:26, Sergey Ulanov wrote: > > > We cannot assume that the frame have been rendered here. You want to pass > that > > > done closure to callback_ so it's called when the frame is actually rendered > > > (i.e. after eglSwapBuffer() has returned). That callback is used to measure > > > rendering latency. > > > > I guess the measurement will be messed up if another frame comes when the > first > > frame is being uploaded and then eglSwapBuffer is run after the second frame > is > > rendered? > > To make it work correctly the renderer will need to keep queue of callbacks for > all pending frames and call them after eglSwapBuffer() completes. In other words > if a frame dropped before being rendered then it's considered to be rendered > only when the next frame is rendered. That's what we have in > PepperVideoRenderer3D. > Done. https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.h (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.h:60: webrtc::DesktopRegion buffer_1_mask_; On 2016/07/21 19:00:32, Yuwei wrote: > I'll add some comment here. Done.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:84: // This may happen if the resolution is changed but a previous shared frame On 2016/07/22 03:38:22, Yuwei wrote: > On 2016/07/21 19:14:25, Sergey Ulanov wrote: > > Currently SoftwareVideoRenderer never processes more than one frame at a time, > > so you can replace this with NOTREACHED(). (also if the renderer did try > > decoding multiple frames in parallel you wouldn't want to drop this frame here > - > > it's still better to render it before the newer frames with updated size are > > decoded.) > > Removed the else branch. > > The problem is that SoftwareVideoRenderer::ProcessVideoPacket calls > AllocateFrame and DrawFrame asynchronously. NOTREACHED() will be problematic if > this happens: > > 1. Allocate frame A > 2. Allocate frame B, resolution changed > 3. Draw frame A > 4. Draw frame B > > Step 3 will hit NOTREACHED() since its underlying buffer is already replaced in > DualBufferFrameConsumer... Ugh. I was wrong when I wrote that SoftwareVideoRenderer doesn't process multiple frames parallel. Sorry. The way SoftwareVideoRenderer is actually implemented it may have multiple decode requests queued. It's possible to fix it in SoftwareVideoRenderer by adding a frame queue, but I don't think it's necessary. https://codereview.chromium.org/2156713002/diff/260001/remoting/client/gl_des... File remoting/client/gl_desktop.h (right): https://codereview.chromium.org/2156713002/diff/260001/remoting/client/gl_des... remoting/client/gl_desktop.h:28: // frame can be either a full frame or updated regions only frame. s/frame can/|frame| can/
https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... File remoting/client/dual_buffer_frame_consumer.cc (right): https://codereview.chromium.org/2156713002/diff/140001/remoting/client/dual_b... remoting/client/dual_buffer_frame_consumer.cc:84: // This may happen if the resolution is changed but a previous shared frame On 2016/07/22 18:04:19, Sergey Ulanov wrote: > On 2016/07/22 03:38:22, Yuwei wrote: > > On 2016/07/21 19:14:25, Sergey Ulanov wrote: > > > Currently SoftwareVideoRenderer never processes more than one frame at a > time, > > > so you can replace this with NOTREACHED(). (also if the renderer did try > > > decoding multiple frames in parallel you wouldn't want to drop this frame > here > > - > > > it's still better to render it before the newer frames with updated size are > > > decoded.) > > > > Removed the else branch. > > > > The problem is that SoftwareVideoRenderer::ProcessVideoPacket calls > > AllocateFrame and DrawFrame asynchronously. NOTREACHED() will be problematic > if > > this happens: > > > > 1. Allocate frame A > > 2. Allocate frame B, resolution changed > > 3. Draw frame A > > 4. Draw frame B > > > > Step 3 will hit NOTREACHED() since its underlying buffer is already replaced > in > > DualBufferFrameConsumer... > > Ugh. I was wrong when I wrote that SoftwareVideoRenderer doesn't process > multiple frames parallel. Sorry. The way SoftwareVideoRenderer is actually > implemented it may have multiple decode requests queued. It's possible to fix it > in SoftwareVideoRenderer by adding a frame queue, but I don't think it's > necessary. > I think the fact is that multiple frames can be processed by SoftwareVideoRenderer at the same time but no two frames can be in the same processing stage (receiving, decoding or rendering) nor their processing order in each stage will alter since tasks are queued up in each thread. I don't think we need to make any change to SoftwareVideoRenderer... https://codereview.chromium.org/2156713002/diff/260001/remoting/client/gl_des... File remoting/client/gl_desktop.h (right): https://codereview.chromium.org/2156713002/diff/260001/remoting/client/gl_des... remoting/client/gl_desktop.h:28: // frame can be either a full frame or updated regions only frame. On 2016/07/22 18:04:19, Sergey Ulanov wrote: > s/frame can/|frame| can/ Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2156713002/#ps280001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Implement DualBufferFrameConsumer This class uses two buffers to decode mutiple frames. It will be used by the chromoting client OpenGL renderer. BUG=385924 ========== to ========== [Chromoting] Implement DualBufferFrameConsumer This class uses two buffers to decode mutiple frames. It will be used by the chromoting client OpenGL renderer. BUG=385924 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Implement DualBufferFrameConsumer This class uses two buffers to decode mutiple frames. It will be used by the chromoting client OpenGL renderer. BUG=385924 ========== to ========== [Chromoting] Implement DualBufferFrameConsumer This class uses two buffers to decode mutiple frames. It will be used by the chromoting client OpenGL renderer. BUG=385924 Committed: https://crrev.com/6d9bb4a011a2df6509c5d50ea7efc3e55f671980 Cr-Commit-Position: refs/heads/master@{#407233} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6d9bb4a011a2df6509c5d50ea7efc3e55f671980 Cr-Commit-Position: refs/heads/master@{#407233} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
