Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2008513003:
Let VideoRenderFrames::FrameToRender return rtc::Optional (Closed)
|
Created:
4 years, 7 months ago by nisse-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLet VideoRenderFrames::FrameToRender return a rtc::Optional.
This eliminates one use of IsZeroSize.
BUG=webrtc:5682
Committed: https://crrev.com/e5e5292adb97c9dcbf45bfc85d030b0eeb25d5fd
Cr-Commit-Position: refs/heads/master@{#12891}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change to using std::unique_ptr. #Patch Set 3 : Revert unique_ptr change. #
Messages
Total messages: 26 (9 generated)
nisse@webrtc.org changed reviewers: + magjed@webrtc.org, pbos@webrtc.org
This cl eliminates one use of IsZeroSize, and an uninitialized webrtc::VideoFrame.
lgtm
https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... File webrtc/common_video/incoming_video_stream.cc (right): https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... webrtc/common_video/incoming_video_stream.cc:182: frame_to_render = render_buffers_->FrameToRender(); Should this be passed through an unique_ptr?
https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... File webrtc/common_video/incoming_video_stream.cc (right): https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... webrtc/common_video/incoming_video_stream.cc:182: frame_to_render = render_buffers_->FrameToRender(); On 2016/05/24 10:14:31, pbos-webrtc wrote: > Should this be passed through an unique_ptr? Do you prefer that over returning an optional by value? They're intended to be small and cheap objects. A unique_ptr would mean additional allocation inside FrameToRender. But on the other hand, if the incoming_frames_ list is changed from std::list<VideoFrame> to std::list<unique_ptr<VideoFrame>> it would make some sense. Even though the lack of a pop method in the stdc++ library becomes even more annoying in that case. BTW, my first plan was to use the more old-fashioned style bool FrameToRender(VideoFrame*) but using rtc:Optional seemed to be more in line with webrtc style.
On 2016/05/24 11:07:31, nisse-webrtc wrote: > https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... > File webrtc/common_video/incoming_video_stream.cc (right): > > https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... > webrtc/common_video/incoming_video_stream.cc:182: frame_to_render = > render_buffers_->FrameToRender(); > On 2016/05/24 10:14:31, pbos-webrtc wrote: > > Should this be passed through an unique_ptr? > > Do you prefer that over returning an optional by value? They're intended to be > small and cheap objects. A unique_ptr would mean additional allocation inside > FrameToRender. > > But on the other hand, if the incoming_frames_ list is changed from > std::list<VideoFrame> to std::list<unique_ptr<VideoFrame>> it would make some > sense. Even though the lack of a pop method in the stdc++ library becomes even > more annoying in that case. > > BTW, my first plan was to use the more old-fashioned style > > bool FrameToRender(VideoFrame*) > > but using rtc:Optional seemed to be more in line with webrtc style. I think preserving and passing ownership here is better, so I prefer unique_ptr.
On 2016/05/24 12:09:50, pbos-webrtc wrote: > On 2016/05/24 11:07:31, nisse-webrtc wrote: > > > https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... > > File webrtc/common_video/incoming_video_stream.cc (right): > > > > > https://codereview.webrtc.org/2008513003/diff/1/webrtc/common_video/incoming_... > > webrtc/common_video/incoming_video_stream.cc:182: frame_to_render = > > render_buffers_->FrameToRender(); > > On 2016/05/24 10:14:31, pbos-webrtc wrote: > > > Should this be passed through an unique_ptr? > > > > Do you prefer that over returning an optional by value? They're intended to be > > small and cheap objects. A unique_ptr would mean additional allocation inside > > FrameToRender. > > > > But on the other hand, if the incoming_frames_ list is changed from > > std::list<VideoFrame> to std::list<unique_ptr<VideoFrame>> it would make some > > sense. Even though the lack of a pop method in the stdc++ library becomes even > > more annoying in that case. > > > > BTW, my first plan was to use the more old-fashioned style > > > > bool FrameToRender(VideoFrame*) > > > > but using rtc:Optional seemed to be more in line with webrtc style. > > I think preserving and passing ownership here is better, so I prefer unique_ptr. This is also aligned with keeping an unique reference to the frame, in case someone cares about IsMutable.
Description was changed from ========== Let VideoRenderFrames::FrameToRender return an optional. This eliminates one use of IsZeroSize. BUG=webrtc:5682 ========== to ========== Let VideoRenderFrames::FrameToRender return an std::unique_ptr. This eliminates one use of IsZeroSize. BUG=webrtc:5682 ==========
Changed to unique_ptr now.
On 2016/05/24 13:47:58, nisse-webrtc wrote: > Changed to unique_ptr now. I don't like the change to store VideoFrames in std::unique_ptr. That's confusing because it implies VideoFrames are non-copyable with an unique owner, but that's not the case for VideoFrames. Also, it's less efficient because it creates an extra heap allocation.
On 2016/05/24 13:57:57, magjed_webrtc wrote: > On 2016/05/24 13:47:58, nisse-webrtc wrote: > > Changed to unique_ptr now. > > I don't like the change to store VideoFrames in std::unique_ptr. That's > confusing because it implies VideoFrames are non-copyable with an unique owner, > but that's not the case for VideoFrames. A heap-allocated VideoFrame needs an owner. > Also, it's less efficient because it creates an extra heap allocation. I don't have a very strong opinion, I just want to get rid of IsZeroSize... If you two can settle this by any means, argument, coin flip, whatever, and then I'll do as you agree?
On 2016/05/24 14:03:38, nisse-webrtc wrote: > On 2016/05/24 13:57:57, magjed_webrtc wrote: > > On 2016/05/24 13:47:58, nisse-webrtc wrote: > > > Changed to unique_ptr now. > > > > I don't like the change to store VideoFrames in std::unique_ptr. That's > > confusing because it implies VideoFrames are non-copyable with an unique > owner, > > but that's not the case for VideoFrames. > > A heap-allocated VideoFrame needs an owner. > > > Also, it's less efficient because it creates an extra heap allocation. > > I don't have a very strong opinion, I just want to get rid of IsZeroSize... > > If you two can settle this by any means, argument, coin flip, whatever, and then > I'll do as you agree? We have settled it - I win :)
Description was changed from ========== Let VideoRenderFrames::FrameToRender return an std::unique_ptr. This eliminates one use of IsZeroSize. BUG=webrtc:5682 ========== to ========== Let VideoRenderFrames::FrameToRender return a rtc::Optional. This eliminates one use of IsZeroSize. BUG=webrtc:5682 ==========
On 2016/05/24 14:40:11, magjed_webrtc wrote: > On 2016/05/24 14:03:38, nisse-webrtc wrote: > > On 2016/05/24 13:57:57, magjed_webrtc wrote: > > > On 2016/05/24 13:47:58, nisse-webrtc wrote: > > > > Changed to unique_ptr now. > > > > > > I don't like the change to store VideoFrames in std::unique_ptr. That's > > > confusing because it implies VideoFrames are non-copyable with an unique > > owner, > > > but that's not the case for VideoFrames. > > > > A heap-allocated VideoFrame needs an owner. > > > > > Also, it's less efficient because it creates an extra heap allocation. > > > > I don't have a very strong opinion, I just want to get rid of IsZeroSize... > > > > If you two can settle this by any means, argument, coin flip, whatever, and > then > > I'll do as you agree? > > We have settled it - I win :) Nice. Changed back now. pbos, do you agree?
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2008513003/#ps40001 (title: "Revert unique_ptr change.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008513003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5839)
lgtm
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008513003/40001
Message was sent while issue was closed.
Description was changed from ========== Let VideoRenderFrames::FrameToRender return a rtc::Optional. This eliminates one use of IsZeroSize. BUG=webrtc:5682 ========== to ========== Let VideoRenderFrames::FrameToRender return a rtc::Optional. This eliminates one use of IsZeroSize. 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 ========== Let VideoRenderFrames::FrameToRender return a rtc::Optional. This eliminates one use of IsZeroSize. BUG=webrtc:5682 ========== to ========== Let VideoRenderFrames::FrameToRender return a rtc::Optional. This eliminates one use of IsZeroSize. BUG=webrtc:5682 Committed: https://crrev.com/e5e5292adb97c9dcbf45bfc85d030b0eeb25d5fd Cr-Commit-Position: refs/heads/master@{#12891} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e5e5292adb97c9dcbf45bfc85d030b0eeb25d5fd Cr-Commit-Position: refs/heads/master@{#12891} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
