Simplify CaptureRenderAdapter.

talk/media/base/capturerenderadapter.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 44 matching lines @@
     VideoCapturer* video_capturer) {
   if (!video_capturer) {
     return NULL;
   }
   CaptureRenderAdapter* return_value = new CaptureRenderAdapter(video_capturer);
   return_value->Init();  // Can't fail.
   return return_value;
 }
 
 bool CaptureRenderAdapter::AddRenderer(VideoRenderer* video_renderer) {
   if (!video_renderer) {

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

can you add an RTC_DCHECK for this? Seems like this would be programmer error

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

Done. Callers (CaptureManager) checks for NULL.

     return false;
   }
   rtc::CritScope cs(&capture_crit_);
   if (IsRendererRegistered(*video_renderer)) {
     return false;
   }
-  video_renderers_.push_back(VideoRendererInfo(video_renderer));
+  video_renderers_.push_back(video_renderer);
   return true;
 }
 
 bool CaptureRenderAdapter::RemoveRenderer(VideoRenderer* video_renderer) {
   if (!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;
     }
   }
   return false;
 }
 
 void CaptureRenderAdapter::Init() {
   video_capturer_->SignalVideoFrame.connect(
       this,
       &CaptureRenderAdapter::OnVideoFrame);
 }
 
 void CaptureRenderAdapter::OnVideoFrame(VideoCapturer* capturer,
                                         const VideoFrame* video_frame) {
   rtc::CritScope cs(&capture_crit_);
   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);
+    (*iter)->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();
-    }
-  }
-}
-
 // The renderer_crit_ lock needs to be taken when calling this function.
 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;
     }
   }
   return false;
 }
 
 }  // namespace cricket