Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1603423002:
Simplify CaptureRenderAdapter. (Closed)
|
Created:
4 years, 11 months ago by nisse-webrtc Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete CaptureRenderAdapter::VideoRenderInfo struct, it is unused since the recent deletion of SetSize.
Delete methods MaybeSetRenderingSize and IsRendererRegistered, the latter replaced by std::find.
Delete return values from AddRenderer and RemoveRenderer.
BUG=webrtc:5426
Committed: https://crrev.com/0b98cf72c6ca36edb96b8b8d1ab6dfd231032d5c
Cr-Commit-Position: refs/heads/master@{#11332}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Simplify OnVideoFrame iteration #Patch Set 3 : Address Tommi's comments. #
Total comments: 3
Patch Set 4 : Deleted CaptureRenderAdapter::IsRendererRegistered, use std::find instead #Patch Set 5 : Delete test of RemoveVideoRenderer return value #
Messages
Total messages: 27 (11 generated)
nisse@webrtc.org changed reviewers: + tommi@webrtc.org
The VideoRenderInfo struct and its width and height fields became unused with the recent deletion of SetSize.
https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... File talk/media/base/capturerenderadapter.cc (right): https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:65: if (!video_renderer) { can you add an RTC_DCHECK for this? Seems like this would be programmer error https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:82: // VideoTrackRenderers), so we can use erase directly? what about: video_renderers_.erase( std::remove(video_renderers_.begin(), video_renderers_.end(), video_renderer), video_renderers_.end()); std::set means more code which I'm not sure is justifiable just to make the call to erase() smaller. Btw, is the return value from RemoveRenderer ever handled? What can the caller do? I think we should probably change it to void. https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:107: iter != video_renderers_.end(); ++iter) { change to range based loop? for (auto* renderer : video_renderers_) renderer->RenderFrame(video_frame);
https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... File talk/media/base/capturerenderadapter.cc (right): https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:82: // VideoTrackRenderers), so we can use erase directly? On 2016/01/20 13:21:24, tommi-webrtc wrote: > what about: > > video_renderers_.erase( > std::remove(video_renderers_.begin(), video_renderers_.end(), > video_renderer), video_renderers_.end()); Maybe, is that readable to a typical C++ programmer? I don't understand it without looking things up. > std::set means more code which I'm not sure is justifiable just to make the call > to erase() smaller. std::set is already used in lots of places, including by VideoTrackRenderers which does more or less the same thing as CaptureRenderAdapter. It kind-of seems appropriate, since this code most likely doesn't care about the order. And with a set I think we could also delete the IsRendererRegistered method. > Btw, is the return value from RemoveRenderer ever handled? What can the caller > do? > I think we should probably change it to void. Return value is passed on to bool ChannelManager::RemoveVideoRenderer( VideoCapturer* capturer, VideoRenderer* renderer) { return initialized_ && worker_thread_->Invoke<bool>( Bind(&CaptureManager::RemoveVideoRenderer, capture_manager_.get(), capturer, renderer)); } called by VideoSource::RemoveSink, where it is finally ignored. You want me to eliminate it in this cl? And very similar for the AddRenderer return value. https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:107: iter != video_renderers_.end(); ++iter) { On 2016/01/20 13:21:24, tommi-webrtc wrote: > change to range based loop? > > for (auto* renderer : video_renderers_) > renderer->RenderFrame(video_frame); Done. Looks nice and concise.
On 2016/01/20 13:45:45, nisse-webrtc wrote: > called by VideoSource::RemoveSink, where it is finally ignored. You want me to > eliminate it in this cl? And very similar for the AddRenderer return value. (Replying to myself). After a second look, I think it's better to that as a separate cl eliminating the return value all the way up to where it's ignored. It affects a more files, $ git diff --stat talk/media/base/capturemanager.cc | 16 ++++++++-------- talk/media/base/capturemanager.h | 8 ++++---- talk/media/base/capturemanager_unittest.cc | 40 ++++++++++------------------------------ talk/media/base/capturerenderadapter.cc | 14 ++++++-------- talk/media/base/capturerenderadapter.h | 4 ++-- talk/media/base/fakecapturemanager.h | 12 ++++-------- talk/session/media/channelmanager.cc | 14 ++++++++------ talk/session/media/channelmanager.h | 4 ++-- 8 files changed, 44 insertions(+), 68 deletions(-) and it's not entirely clear that ignoring all return values there is the right thing.
The CQ bit was checked by nisse@webrtc.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/1603423002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1603423002/20001
https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... File talk/media/base/capturerenderadapter.cc (right): https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:82: // VideoTrackRenderers), so we can use erase directly? On 2016/01/20 13:45:45, nisse-webrtc wrote: > On 2016/01/20 13:21:24, tommi-webrtc wrote: > > what about: > > > > video_renderers_.erase( > > std::remove(video_renderers_.begin(), video_renderers_.end(), > > video_renderer), video_renderers_.end()); > > Maybe, is that readable to a typical C++ programmer? I don't understand it > without looking things up. > > > std::set means more code which I'm not sure is justifiable just to make the > call > > to erase() smaller. > > std::set is already used in lots of places, including by VideoTrackRenderers > which does more or less the same thing as CaptureRenderAdapter. It kind-of seems > appropriate, since this code most likely doesn't care about the order. And with > a set I think we could also delete the IsRendererRegistered method. Using erase+std::remove is readable to a C++ programmer that's familiar with STL I guess :) However I think that in this example it'll only be familiar to you if you've seen it before and know how it works. Ymmv. std::set has code to sort items and hash, maintain a binary tree of the elements etc. It's a lot more code (I'm thinking binary bloat btw) and overhead than std::vector. Just try googling for "std::set overhead" and you'll see what I mean. For holding an array of pointers, I think it's overkill and don't think that the justification of making the call to erase(), simpler. There is another option however, std::list. It has a remove() method that accepts values. > > > Btw, is the return value from RemoveRenderer ever handled? What can the > caller > > do? > > I think we should probably change it to void. > > Return value is passed on to > > bool ChannelManager::RemoveVideoRenderer( > VideoCapturer* capturer, VideoRenderer* renderer) { > return initialized_ && worker_thread_->Invoke<bool>( > Bind(&CaptureManager::RemoveVideoRenderer, > capture_manager_.get(), capturer, renderer)); > } > > called by VideoSource::RemoveSink, where it is finally ignored. You want me to > eliminate it in this cl? And very similar for the AddRenderer return value. If it's simple for you to do in this CL then that would be great of course but I'm also ok with a separate cl.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... File talk/media/base/capturerenderadapter.cc (right): https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:65: if (!video_renderer) { On 2016/01/20 13:21:24, tommi-webrtc wrote: > can you add an RTC_DCHECK for this? Seems like this would be programmer error Done. Callers (CaptureManager) checks for NULL. https://codereview.webrtc.org/1603423002/diff/1/talk/media/base/capturerender... talk/media/base/capturerenderadapter.cc:82: // VideoTrackRenderers), so we can use erase directly? On 2016/01/20 15:38:15, tommi-webrtc wrote: > For holding an array of pointers, I think it's overkill and don't think that the > justification of making the call to erase(), simpler. There is another option > however, std::list. It has a remove() method that accepts values. I'm editing the TODO to suggest std::list, but I'm not doing anything more about it now. It's unclear to me if the set semantics implemented by AddRenderer really is needed. The call chain is from VideoSource (which maintains it's own list of renderers; not sure if its identical to the list here, or if different source own different subsets), to ChannelManager, to CaptureManager, to CaptureRenderAdapter. I'm pretty sure some of this code can be eliminated. > If it's simple for you to do in this CL then that would be great of course but > I'm also ok with a separate cl. I'm doing it halfway in this cl. Dropping the return values for the CaptureRenderAdapter methods, but for now keeping the return values of its callers.
thanks. lgtm. https://codereview.webrtc.org/1603423002/diff/40001/talk/media/base/capturere... File talk/media/base/capturerenderadapter.cc (right): https://codereview.webrtc.org/1603423002/diff/40001/talk/media/base/capturere... talk/media/base/capturerenderadapter.cc:69: // added once. TODO(nisse): Is this really needed? nit: move TODO to a separate line. Agree that this shouldn't be needed. We could dcheck that std::find(video_renderers_.begin(), video_renderers_.end(), video_renderer) == vector.end() https://codereview.webrtc.org/1603423002/diff/40001/talk/media/base/capturere... talk/media/base/capturerenderadapter.cc:111: iter != video_renderers_.end(); ++iter) { nit: you could update this loop too, but this method will likely be going away. can be replaced by std::find if needed.
Still looks good? I'm not familiar with stl. I'm starting a cq dry run now, and will commit if no issues turn up. https://codereview.webrtc.org/1603423002/diff/40001/talk/media/base/capturere... File talk/media/base/capturerenderadapter.cc (right): https://codereview.webrtc.org/1603423002/diff/40001/talk/media/base/capturere... talk/media/base/capturerenderadapter.cc:69: // added once. TODO(nisse): Is this really needed? On 2016/01/21 08:45:34, tommi-webrtc wrote: > nit: move TODO to a separate line. > Agree that this shouldn't be needed. We could dcheck that > std::find(video_renderers_.begin(), video_renderers_.end(), video_renderer) == > vector.end() Done. I switched to using std::find, and deleted the IsRendererRegistered method.
The CQ bit was checked by nisse@webrtc.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/1603423002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1603423002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/11987)
tommi@chromium.org changed reviewers: + tommi@chromium.org
Thanks! Still lgtm. I guess you'll have to remove that test that seems to be the only thing checking the return value of the RemoveVideoRenderer method :)
Description was changed from ========== Delete CaptureRenderAdapter::VideoRenderInfo struct. BUG=webrtc:5426 ========== to ========== Delete CaptureRenderAdapter::VideoRenderInfo struct, it is unused since the recent deletion of SetSize. Delete methods MaybeSetRenderingSize and IsRendererRegistered, the latter replaced by std::find. Delete return values from AddRenderer and RemoveRenderer. BUG=webrtc:5426 ==========
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, tommi@chromium.org Link to the patchset: https://codereview.webrtc.org/1603423002/#ps80001 (title: "Delete test of RemoveVideoRenderer return value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1603423002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1603423002/80001
Message was sent while issue was closed.
Description was changed from ========== Delete CaptureRenderAdapter::VideoRenderInfo struct, it is unused since the recent deletion of SetSize. Delete methods MaybeSetRenderingSize and IsRendererRegistered, the latter replaced by std::find. Delete return values from AddRenderer and RemoveRenderer. BUG=webrtc:5426 ========== to ========== Delete CaptureRenderAdapter::VideoRenderInfo struct, it is unused since the recent deletion of SetSize. Delete methods MaybeSetRenderingSize and IsRendererRegistered, the latter replaced by std::find. Delete return values from AddRenderer and RemoveRenderer. BUG=webrtc:5426 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Delete CaptureRenderAdapter::VideoRenderInfo struct, it is unused since the recent deletion of SetSize. Delete methods MaybeSetRenderingSize and IsRendererRegistered, the latter replaced by std::find. Delete return values from AddRenderer and RemoveRenderer. BUG=webrtc:5426 ========== to ========== Delete CaptureRenderAdapter::VideoRenderInfo struct, it is unused since the recent deletion of SetSize. Delete methods MaybeSetRenderingSize and IsRendererRegistered, the latter replaced by std::find. Delete return values from AddRenderer and RemoveRenderer. BUG=webrtc:5426 Committed: https://crrev.com/0b98cf72c6ca36edb96b8b8d1ab6dfd231032d5c Cr-Commit-Position: refs/heads/master@{#11332} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0b98cf72c6ca36edb96b8b8d1ab6dfd231032d5c Cr-Commit-Position: refs/heads/master@{#11332} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
