Simplify CaptureRenderAdapter.

talk/media/base/capturerenderadapter.cc

Index: talk/media/base/capturerenderadapter.cc
diff --git a/talk/media/base/capturerenderadapter.cc b/talk/media/base/capturerenderadapter.cc
index 3fdb68be9bb2340ee9293db4d2720ad373236939..638dffd9ad1a91cb8af844943341c13f4e5b475a 100644
--- a/talk/media/base/capturerenderadapter.cc
+++ b/talk/media/base/capturerenderadapter.cc
@@ -69,7 +69,7 @@ bool CaptureRenderAdapter::AddRenderer(VideoRenderer* video_renderer) {
   if (IsRendererRegistered(*video_renderer)) {
     return false;
   }
-  video_renderers_.push_back(VideoRendererInfo(video_renderer));
+  video_renderers_.push_back(video_renderer);
   return true;
 }
 
@@ -78,9 +78,11 @@ bool CaptureRenderAdapter::RemoveRenderer(VideoRenderer* video_renderer) {
     return false;
   }
   rtc::CritScope cs(&capture_crit_);
+  // TODO(nisse): Switch to using std::set (like in
+  // VideoTrackRenderers), so we can use erase directly?

[tommi, 2016/01/20 13:21:24]

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.

[nisse-webrtc, 2016/01/20 13:45:45]

Maybe, is that readable to a typical C++ programmer? I don't understand it
without looking things up.
 
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.
  
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.

[tommi, 2016/01/20 15:38:15]

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.
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.

[nisse-webrtc, 2016/01/21 08:08:50]

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.
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.

   for (VideoRenderers::iterator iter = video_renderers_.begin();
        iter != video_renderers_.end(); ++iter) {
-    if (video_renderer == iter->renderer) {
+    if (video_renderer == *iter) {
       video_renderers_.erase(iter);
       return true;
     }
@@ -100,25 +102,10 @@ void CaptureRenderAdapter::OnVideoFrame(VideoCapturer* capturer,
   if (video_renderers_.empty()) {
     return;
   }
-  MaybeSetRenderingSize(video_frame);
 
   for (VideoRenderers::iterator iter = video_renderers_.begin();
        iter != video_renderers_.end(); ++iter) {

[tommi, 2016/01/20 13:21:24]

change to range based loop?

for (auto* renderer : video_renderers_)
  renderer->RenderFrame(video_frame);

[nisse-webrtc, 2016/01/20 13:45:45]

Done. Looks nice and concise.

-    VideoRenderer* video_renderer = iter->renderer;
-    video_renderer->RenderFrame(video_frame);
-  }
-}
-
-// The renderer_crit_ lock needs to be taken when calling this function.
-void CaptureRenderAdapter::MaybeSetRenderingSize(const VideoFrame* frame) {
-  for (VideoRenderers::iterator iter = video_renderers_.begin();
-       iter != video_renderers_.end(); ++iter) {
-    const bool new_resolution = iter->render_width != frame->GetWidth() ||
-        iter->render_height != frame->GetHeight();
-    if (new_resolution) {
-      iter->render_width = frame->GetWidth();
-      iter->render_height = frame->GetHeight();
-    }
+    (*iter)->RenderFrame(video_frame);
   }
 }
 
@@ -127,7 +114,7 @@ bool CaptureRenderAdapter::IsRendererRegistered(
     const VideoRenderer& video_renderer) const {
   for (VideoRenderers::const_iterator iter = video_renderers_.begin();
        iter != video_renderers_.end(); ++iter) {
-    if (&video_renderer == iter->renderer) {
+    if (&video_renderer == *iter) {
       return true;
     }
   }