Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2068703002:
Delete only call to cricket::VideoFrame::Copy. (Closed)
|
Created:
4 years, 6 months ago by nisse-chromium (ooo August 14) Modified:
4 years, 6 months ago Reviewers:
perkj_chrome CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, mkwst+moarreviews-renderer_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelete only call to cricket::VideoFrame::Copy.
BUG=webrtc:5682
Committed: https://crrev.com/c935fa235be2f6849222ce3b16ce31c0bca20e9a
Cr-Commit-Position: refs/heads/master@{#400649}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Avoid allocating a place-holder object for AddDestructionObserver, #Messages
Total messages: 17 (7 generated)
nisse@chromium.org changed reviewers: + perkj@webrtc.org
This cl gets rid of the only use of the Copy method. But there's probably some prettier way to do it. https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/media_stream_remote_video_source.cc:101: incoming_frame.GetCopyWithRotationApplied()->video_frame_buffer(); This looks a bit silly. Should there be a rotate method on VideoFrameBuffer instead? Something like buffer = buffer->Rotate(incoming_frame.rotation()) https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/media_stream_remote_video_source.cc:123: new scoped_refptr<webrtc::VideoFrameBuffer>(buffer))); There ought to be some better way to do this, without allocating a new placeholder object. How?
perkj@chromium.org changed reviewers: + perkj@chromium.org
https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/media_stream_remote_video_source.cc:123: new scoped_refptr<webrtc::VideoFrameBuffer>(buffer))); On 2016/06/14 13:06:58, nisse-chromium wrote: > There ought to be some better way to do this, without allocating a new > placeholder object. How? video_frame->AddDestructionObserver( base::Bind(&ReleaseOriginalFrame, frame)); video_frame->AddDestuctrionObserver([buffer] {});
nisse@chromium.org changed reviewers: - perkj@webrtc.org
https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/2068703002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/media_stream_remote_video_source.cc:123: new scoped_refptr<webrtc::VideoFrameBuffer>(buffer))); On 2016/06/14 13:59:28, perkj_chrome wrote: > On 2016/06/14 13:06:58, nisse-chromium wrote: > > There ought to be some better way to do this, without allocating a new > > placeholder object. How? > > video_frame->AddDestructionObserver( > base::Bind(&ReleaseOriginalFrame, frame)); > > video_frame->AddDestuctrionObserver([buffer] {}); The compiler didn't like a lambda here, but binding a DoNothing function seemed to work. Would make some sense to add to bind_helpers.h.
lgtm
lgtm
The CQ bit was checked by nisse@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068703002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was checked by nisse@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068703002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Delete only call to cricket::VideoFrame::Copy. BUG=webrtc:5682 ========== to ========== Delete only call to cricket::VideoFrame::Copy. BUG=webrtc:5682 Committed: https://crrev.com/c935fa235be2f6849222ce3b16ce31c0bca20e9a Cr-Commit-Position: refs/heads/master@{#400649} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c935fa235be2f6849222ce3b16ce31c0bca20e9a Cr-Commit-Position: refs/heads/master@{#400649} |
