Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1821153003:
Avoid using MakeExclusive, which is going to be deleted in webrtc. (Closed)
|
Created:
4 years, 9 months ago by nisse-chromium (ooo August 14) Modified:
4 years, 8 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org, pbos Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid using MakeExclusive, which is going to be deleted in webrtc.
Instead, keeps an I420Buffer, and uses HasOneRef to check if it needs to make a new copy.
BUG=webrtc:5682
Committed: https://crrev.com/37c564a4b8a2858049a6fdc4a8145593b2f89166
Cr-Commit-Position: refs/heads/master@{#383995}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address Sergey's comments. #
Total comments: 4
Patch Set 3 : Fixed to compile with latest webrtc. #
Total comments: 1
Messages
Total messages: 26 (10 generated)
nisse@chromium.org changed reviewers: + perkj@chromium.org, sergeyu@chromium.org
Do you think this is a reasonable approach to eliminate MakeExclusive? I'm considering adding a Copy method instead, and I think something like that, in the VideoFrameBuffer/I420Buffer classes, makes more sense than the current CopyToPlanes method in cricket::VideoFrame (which is used by MakeExclusive).
https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... File remoting/protocol/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:176: size_t height = frame->size().height(); change these two to int then you wouldn't need the casts below. https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:191: // Frame is still used, typically by the encoder. We have to make Please move this comment above the TODO or move the TODO after this comment. https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:222: yuv_frame_->stride(webrtc::kVPlane), width, height); nit: this ARGBToI420() call would be cleaner if you add variables for the strides above it, i.e. int y_stride = yuv_frame_->stride(webrtc::kYPlane); int u_stride = yuv_frame_->stride(webrtc::kUPlane); int v_stride = yuv_frame_->stride(webrtc::kVPlane); FWIW the stride() method should really be called Stride() with capital S. It's virtual, not inlineable and has a switch statement in it, i.e. calling it is much more expensive that accessing a local variable. https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:226: yuv_frame_, base::TimeTicks::Now().ToInternalValue() * This is not a proper way to convert TimeTicks to microseconds. You assume that internal value is in nanoseconds, but that may not be the case. I suggest changing this to (base::TimeTicks::Now() - base::TimeTicks()) / base::TimeDelta::FromMicroseconds(1) This expression doesn't depend on how the ticks represented internally.
Description was changed from ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Depends on the I420Buffer::Copy method, which doesn't exist yet. In my local tree I have the method declared, and then this cl compiles but fails at linking. Feedback on this approach is appreciated. BUG=webrtc:5426 ========== to ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Depends on the I420Buffer::Copy method, which doesn't exist yet. In my local tree I have the method declared, and then this cl compiles but fails at linking. Feedback on this approach is appreciated. BUG=webrtc:5682 ==========
pbos@chromium.org changed reviewers: + magjed@chromium.org
+r magjed@, can't we use any existing webrtc::VideoFrame copy methods here?
https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... File remoting/protocol/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:188: // TODO(sergeyu): This will copy the buffer if it's being used. Optimize it by About this todo: Is the whole yuv_frame updated below or just a part of it for each captured frame? If the complete frame is always updated, there is a webrtc::I420BufferPool we can use instead.
lgtm
https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... File remoting/protocol/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:188: // TODO(sergeyu): This will copy the buffer if it's being used. Optimize it by On 2016/03/23 14:43:54, perkj_chrome wrote: > About this todo: Is the whole yuv_frame updated below or just a part of it for > each captured frame? > If the complete frame is always updated, there is a webrtc::I420BufferPool we > can use instead. The for loop below performs YUV conversion only for parts that are changing.
https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... File remoting/protocol/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:176: size_t height = frame->size().height(); On 2016/03/22 17:27:09, Sergey Ulanov wrote: > change these two to int then you wouldn't need the casts below. Done. https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:191: // Frame is still used, typically by the encoder. We have to make On 2016/03/22 17:27:09, Sergey Ulanov wrote: > Please move this comment above the TODO or move the TODO after this comment. Done. https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:222: yuv_frame_->stride(webrtc::kVPlane), width, height); On 2016/03/22 17:27:09, Sergey Ulanov wrote: > nit: this ARGBToI420() call would be cleaner if you add variables for the > strides above it, i.e. > int y_stride = yuv_frame_->stride(webrtc::kYPlane); > int u_stride = yuv_frame_->stride(webrtc::kUPlane); > int v_stride = yuv_frame_->stride(webrtc::kVPlane); > > FWIW the stride() method should really be called Stride() with capital S. It's > virtual, not inlineable and has a switch statement in it, i.e. calling it is > much more expensive that accessing a local variable. > https://google.github.io/styleguide/cppguide.html#Function_Names Done. https://codereview.chromium.org/1821153003/diff/1/remoting/protocol/webrtc_vi... remoting/protocol/webrtc_video_capturer_adapter.cc:226: yuv_frame_, base::TimeTicks::Now().ToInternalValue() * On 2016/03/22 17:27:09, Sergey Ulanov wrote: > This is not a proper way to convert TimeTicks to microseconds. You assume that > internal value is in nanoseconds, but that may not be the case. I suggest > changing this to > (base::TimeTicks::Now() - base::TimeTicks()) / > base::TimeDelta::FromMicroseconds(1) > > This expression doesn't depend on how the ticks represented internally. Done.
lgtm https://codereview.chromium.org/1821153003/diff/20001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/1821153003/diff/20001/remoting/protocol/webrt... remoting/protocol/webrtc_video_capturer_adapter.cc:220: y_stride * top + left, Please run clang-format for this change (git cl format). I think it would format this code a bit differently. https://codereview.chromium.org/1821153003/diff/20001/remoting/protocol/webrt... remoting/protocol/webrtc_video_capturer_adapter.cc:231: scoped_ptr<cricket::VideoFrame> video_frame(new cricket::WebRtcVideoFrame( nit: the frame can be allocated on the stack instead of the heap since it's not passed anywhere: cricket::WebRtcVideoFrame video_frame(...);
The CQ bit was checked by nisse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821153003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821153003/40001
The I420Buffer::Copy has been landed in webrtc, and rolled into chrome. So this change can now be tested. https://codereview.chromium.org/1821153003/diff/20001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/1821153003/diff/20001/remoting/protocol/webrt... remoting/protocol/webrtc_video_capturer_adapter.cc:220: y_stride * top + left, On 2016/03/24 18:15:19, Sergey Ulanov wrote: > Please run clang-format for this change (git cl format). I think it would format > this code a bit differently. Done. https://codereview.chromium.org/1821153003/diff/20001/remoting/protocol/webrt... remoting/protocol/webrtc_video_capturer_adapter.cc:231: scoped_ptr<cricket::VideoFrame> video_frame(new cricket::WebRtcVideoFrame( On 2016/03/24 18:15:19, Sergey Ulanov wrote: > nit: the frame can be allocated on the stack instead of the heap since it's not > passed anywhere: > cricket::WebRtcVideoFrame video_frame(...); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Depends on the I420Buffer::Copy method, which doesn't exist yet. In my local tree I have the method declared, and then this cl compiles but fails at linking. Feedback on this approach is appreciated. BUG=webrtc:5682 ========== to ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Instead, keeps an I420Buffer, and uses HasOneRef to check if it needs to make a new copy. BUG=webrtc:5682 ==========
lgtm
https://codereview.chromium.org/1821153003/diff/40001/remoting/protocol/webrt... File remoting/protocol/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/1821153003/diff/40001/remoting/protocol/webrt... remoting/protocol/webrtc_video_capturer_adapter.h:86: rtc::scoped_refptr<webrtc::I420Buffer> yuv_frame_; nit: This should use Chromes scoped_refptr - not RTC.
The CQ bit was checked by nisse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1821153003/#ps40001 (title: "Fixed to compile with latest webrtc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821153003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821153003/40001
Message was sent while issue was closed.
Description was changed from ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Instead, keeps an I420Buffer, and uses HasOneRef to check if it needs to make a new copy. BUG=webrtc:5682 ========== to ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Instead, keeps an I420Buffer, and uses HasOneRef to check if it needs to make a new copy. BUG=webrtc:5682 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Instead, keeps an I420Buffer, and uses HasOneRef to check if it needs to make a new copy. BUG=webrtc:5682 ========== to ========== Avoid using MakeExclusive, which is going to be deleted in webrtc. Instead, keeps an I420Buffer, and uses HasOneRef to check if it needs to make a new copy. BUG=webrtc:5682 Committed: https://crrev.com/37c564a4b8a2858049a6fdc4a8145593b2f89166 Cr-Commit-Position: refs/heads/master@{#383995} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/37c564a4b8a2858049a6fdc4a8145593b2f89166 Cr-Commit-Position: refs/heads/master@{#383995} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
