Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2062843003:
Cleanup WebrtcVideoRendererAdapter (Closed)
|
Created:
4 years, 6 months ago by Sergey Ulanov Modified:
4 years, 6 months ago CC:
chromium-reviews, posciak+watch_chromium.org, chromoting-reviews_chromium.org, perkj_chrome Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup WebrtcVideoRendererAdapter
Fixed two issue:
- FrameConsumer::AllocateFrame() wasn't previusly called.
- The adapter was rendering frames out-of-order when the host didn't
have playout-delay extension enabled. Changed the adapter to just log
a warning when render delay is greater than 0, and otherwise ignore
render timestamp. This ensures that frames are emitted in the order
they are received. This is safe because host now sets playout delay
to 0 for all video frames.
Committed: https://crrev.com/bbae9485fb77ed72ec03621056b1ff99f7ce4d58
Cr-Commit-Position: refs/heads/master@{#399593}
Patch Set 1 #
Total comments: 6
Messages
Total messages: 16 (6 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
sergeyu@chromium.org changed reviewers: + jamiewalch@chromium.org
LGTM if my efficiency concerns are unfounded. https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_renderer_adapter.cc (right): https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_renderer_adapter.cc:80: base::Passed(base::WrapUnique(frame.Copy())))); Is the cost of this copy significant? https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_renderer_adapter.cc:94: LOG(WARNING) << "Received frame with playout delay greater than 0."; If this is never expected to happen, maybe it should be a DCHECK instead?
https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_renderer_adapter.cc (right): https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_renderer_adapter.cc:80: base::Passed(base::WrapUnique(frame.Copy())))); On 2016/06/13 21:22:51, Jamie wrote: > Is the cost of this copy significant? No. It doesn't copy the buffer. https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_renderer_adapter.cc:94: LOG(WARNING) << "Received frame with playout delay greater than 0."; On 2016/06/13 21:22:51, Jamie wrote: > If this is never expected to happen, maybe it should be a DCHECK instead? It's not supposed to happen, but it depends on the host, which is the other end of the connection, so DCHECK() wouldn't be appropriate here. (in the future we may relax playout delay, e.g. when detecting video playback, to make it smoother).
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062843003/40001
Message was sent while issue was closed.
Committed patchset #1 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup WebrtcVideoRendererAdapter Fixed two issue: - FrameConsumer::AllocateFrame() wasn't previusly called. - The adapter was rendering frames out-of-order when the host didn't have playout-delay extension enabled. Changed the adapter to just log a warning when render delay is greater than 0, and otherwise ignore render timestamp. This ensures that frames are emitted in the order they are received. This is safe because host now sets playout delay to 0 for all video frames. ========== to ========== Cleanup WebrtcVideoRendererAdapter Fixed two issue: - FrameConsumer::AllocateFrame() wasn't previusly called. - The adapter was rendering frames out-of-order when the host didn't have playout-delay extension enabled. Changed the adapter to just log a warning when render delay is greater than 0, and otherwise ignore render timestamp. This ensures that frames are emitted in the order they are received. This is safe because host now sets playout delay to 0 for all video frames. Committed: https://crrev.com/bbae9485fb77ed72ec03621056b1ff99f7ce4d58 Cr-Commit-Position: refs/heads/master@{#399593} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/bbae9485fb77ed72ec03621056b1ff99f7ce4d58 Cr-Commit-Position: refs/heads/master@{#399593}
Message was sent while issue was closed.
nisse@chromium.org changed reviewers: + nisse@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_renderer_adapter.cc (right): https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_renderer_adapter.cc:80: base::Passed(base::WrapUnique(frame.Copy())))); Hi, I'm trying to delete this Copy method (cl https://codereview.webrtc.org/2080253002/, which I just had to revert). I thought I had removed the last use in Chrome in https://codereview.chromium.org/2068703002/, but it seems I was wrong... After a first look, it seems like in this case, where you don't care about the timestamp and rotation info in the frame, you could do the timestamp check in OnFrame, and only pass on frame.video_frame_buffer() (a ref-counted object) to PostTask. Except that the ConvertToRgbBuffer method then isn't available. I'm about to leave for some summer vacation, but perhaps you and perkj@ can sort out the right way to handle this? I think VideoFrame::ConvertToRgbBuffer is another method we'd like to move/delete. Maybe a static helper method on VideoFrameBuffer would be more appropriate.
Message was sent while issue was closed.
https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_renderer_adapter.cc (right): https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_renderer_adapter.cc:80: base::Passed(base::WrapUnique(frame.Copy())))); On 2016/06/21 11:40:02, nisse-chromium wrote: > Hi, I'm trying to delete this Copy method (cl > https://codereview.webrtc.org/2080253002/, which I just had to revert). > > I thought I had removed the last use in Chrome in > https://codereview.chromium.org/2068703002/, but it seems I was wrong... > > After a first look, it seems like in this case, where you don't care about the > timestamp and rotation info in the frame, you could do the timestamp check in > OnFrame, and only pass on frame.video_frame_buffer() (a ref-counted object) to > PostTask. Except that the ConvertToRgbBuffer method then isn't available. > > I'm about to leave for some summer vacation, but perhaps you and perkj@ can sort > out the right way to handle this? > > I think VideoFrame::ConvertToRgbBuffer is another method we'd like to > move/delete. Maybe a static helper method on VideoFrameBuffer would be more > appropriate. Copy() isn't marked as deprecated, so I assumed it's safe to use. Anyway, this will be easy to fix.
Message was sent while issue was closed.
On 2016/06/21 17:38:23, Sergey Ulanov wrote: > https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... > File remoting/protocol/webrtc_video_renderer_adapter.cc (right): > > https://codereview.chromium.org/2062843003/diff/40001/remoting/protocol/webrt... > remoting/protocol/webrtc_video_renderer_adapter.cc:80: > base::Passed(base::WrapUnique(frame.Copy())))); > On 2016/06/21 11:40:02, nisse-chromium wrote: > > Hi, I'm trying to delete this Copy method (cl > > https://codereview.webrtc.org/2080253002/, which I just had to revert). > > > > I thought I had removed the last use in Chrome in > > https://codereview.chromium.org/2068703002/, but it seems I was wrong... > > > > After a first look, it seems like in this case, where you don't care about the > > timestamp and rotation info in the frame, you could do the timestamp check in > > OnFrame, and only pass on frame.video_frame_buffer() (a ref-counted object) to > > PostTask. Except that the ConvertToRgbBuffer method then isn't available. > > > > I'm about to leave for some summer vacation, but perhaps you and perkj@ can > sort > > out the right way to handle this? > > > > I think VideoFrame::ConvertToRgbBuffer is another method we'd like to > > move/delete. Maybe a static helper method on VideoFrameBuffer would be more > > appropriate. > > Copy() isn't marked as deprecated, so I assumed it's safe to use. Anyway, this > will be easy to fix. This is fixed in in crrev.com/401070
Message was sent while issue was closed.
On 2016/06/21 19:44:46, Sergey Ulanov wrote: > > Copy() isn't marked as deprecated, so I assumed it's safe to use. Anyway, this > > will be easy to fix. Good point. I'll try to go through the headers and add comments on the functions we want to get rid of, with a reference to the bug. > This is fixed in in crrev.com/401070 Great, thanks! Unfortunately, it turned out Copy is used in some other downstream projects too, so I can't reland my cl right away. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
