for readability review.

content/renderer/media/video_capture_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 

[zunger, 2012/07/09 20:08:26]

Overall, the flow of control in this file is spectacularly hard to follow. Part
of it is the fact that every single API function seems to be separated out into
the top-level call, which punts the real work to another thread, and a handler
function; that could be helped by separating out the two groups of functions
into different parts of the file, and visually distinguishing the parts with
comments. 

But perhaps more importantly, this file -- and its header file -- is lacking in
meta-comments that explain what this class is really doing, what the different
states for a client are, what a client even means in this context, etc. I'm only
starting to understand it from reading the code.

Given the extremely complex state machine that this class implies, both for the
capturer itself and for all of its clients, some very clear documentation is
needed -- or perhaps a refactoring which makes the dynamics clearer. I suspect
that the threading behavior in this class is actually deeply broken, and fixing
it may require the latter anyway.

 #include "content/renderer/media/video_capture_impl.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "content/common/child_process.h"
 #include "content/common/media/video_capture_messages.h"
 
 struct VideoCaptureImpl::DIBBuffer {
  public:
   DIBBuffer(
       base::SharedMemory* d,
       media::VideoCapture::VideoFrameBuffer* ptr)
       : dib(d),
         mapped_memory(ptr),
         references(0) {
   }
   ~DIBBuffer() {}
 
   scoped_ptr<base::SharedMemory> dib;
   scoped_refptr<media::VideoCapture::VideoFrameBuffer> mapped_memory;
 
-  // Number of clients which hold this DIB.
+  // Number of clients  which hold this DIB.
   int references;

[zunger, 2012/07/09 20:08:26]

Are you sure you want to use an unlocked int for a reference count? (cf
base/atomic_refcount.h for the right way to do those)

[wjia(left Chromium), 2012/07/10 17:32:28]

Yes, DIBBuffer's are accessed only on capture thread. There should be no racing
to access its members.

 };
 
 bool VideoCaptureImpl::CaptureStarted() {
   return state_ == video_capture::kStarted;
 }
 
 int VideoCaptureImpl::CaptureWidth() {
   return current_params_.width;
 }
 
@@ skipping 16 matching lines @@
       video_type_(media::VideoCaptureCapability::kI420),
       device_info_available_(false),
       state_(video_capture::kStopped) {
   DCHECK(filter);
   memset(&current_params_, 0, sizeof(current_params_));
   memset(&device_info_, 0, sizeof(device_info_));
   current_params_.session_id = id;
 }
 
 VideoCaptureImpl::~VideoCaptureImpl() {
   STLDeleteContainerPairSecondPointers(cached_dibs_.begin(),

[zunger, 2012/07/09 20:08:26]

STLDeleteValues(&cached_dibs_);

[wjia(left Chromium), 2012/07/10 17:32:28]

Done.

                                        cached_dibs_.end());
 }
 
 void VideoCaptureImpl::Init() {
   io_message_loop_proxy_ = ChildProcess::current()->io_message_loop_proxy();
   if (!io_message_loop_proxy_->BelongsToCurrentThread()) {
     io_message_loop_proxy_->PostTask(FROM_HERE,
         base::Bind(&VideoCaptureImpl::AddDelegateOnIOThread,
                    base::Unretained(this)));
     return;

[zunger, 2012/07/09 20:08:26]

I would use an 'else' instead of a 'return,' it makes the branched flow of
control clearer.

[wjia(left Chromium), 2012/07/10 17:32:28]

Done.

   }
 
   AddDelegateOnIOThread();
 }
 
 void VideoCaptureImpl::DeInit(base::Closure task) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
       base::Bind(&VideoCaptureImpl::DoDeInit,
                  base::Unretained(this), task));
 }
 
 void VideoCaptureImpl::DoDeInit(base::Closure task) {
   if (state_ == video_capture::kStarted)
     Send(new VideoCaptureHostMsg_Stop(device_id_));
 
   io_message_loop_proxy_->PostTask(FROM_HERE,
       base::Bind(&VideoCaptureImpl::RemoveDelegateOnIOThread,
                  base::Unretained(this), task));
 }
 
 void VideoCaptureImpl::StartCapture(
     media::VideoCapture::EventHandler* handler,
     const media::VideoCaptureCapability& capability) {
   DCHECK_EQ(capability.color, media::VideoCaptureCapability::kI420);
 
   capture_message_loop_proxy_->PostTask(FROM_HERE,
       base::Bind(&VideoCaptureImpl::DoStartCapture,
                  base::Unretained(this), handler, capability));

[zunger, 2012/07/09 20:08:26]

You've got a subtle segfault risk here: capability is a const&, so if it goes
out of scope in the caller for any reason before the asynchronous task gets
executed, you'll have unexpected released memory. The normal fix for this is for
the callback to take a non-reference argument, and actually pay the copy cost.
(Or to guarantee via your API that the caller can't release the reference before
everything returns, but that's much more error-prone)

[wjia(left Chromium), 2012/07/10 17:32:28]

In Chromium, base::Bind does make a copy of const reference when it creates
async task. So it's safe here.

 }
 
 void VideoCaptureImpl::StopCapture(media::VideoCapture::EventHandler* handler) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
       base::Bind(&VideoCaptureImpl::DoStopCapture,
                  base::Unretained(this), handler));
 }
 
 void VideoCaptureImpl::FeedBuffer(scoped_refptr<VideoFrameBuffer> buffer) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
@@ skipping 38 matching lines @@
     media::VideoCapture::EventHandler* handler,
     const media::VideoCaptureCapability& capability) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   if (state_ == video_capture::kError) {
     handler->OnError(this, 1);
     handler->OnRemoved(this);
     return;
   }
 
   ClientInfo::iterator it1 = clients_pending_on_filter_.find(handler);

[zunger, 2012/07/09 20:08:26]

Why are you creating these interim variables? (NB that it breaks the
short-circuiting of the OR below, and you'll do two map searches no matter what)

[wjia(left Chromium), 2012/07/10 17:32:28]

Removed interim variables. The original intention is to make "if" statement easy
to read since sub-condition can't fit in one line without interim variable. 

   ClientInfo::iterator it2 = clients_pending_on_restart_.find(handler);
   if (it1 != clients_pending_on_filter_.end() ||
       it2 != clients_pending_on_restart_.end() ||
       clients_.find(handler) != clients_.end() ) {
     // This client has started.
     return;
   }
 
   if (!device_id_) {
     clients_pending_on_filter_[handler] = capability;
     return;
   }
 
   handler->OnStarted(this);
   if (state_ == video_capture::kStarted) {
     if (capability.width > current_params_.width ||
         capability.height > current_params_.height) {
       StopDevice();
       DVLOG(1) << "StartCapture: Got client with higher resolution ("
                << capability.width << ", " << capability.height << ") "
                << "after started, try to restart.";
       clients_pending_on_restart_[handler] = capability;
       return;

[zunger, 2012/07/09 20:08:26]

Way too many 'returns' in this function. It suggests a hard-to-understand flow
of control; 'return' should be an exceptionally rare thing to call except at the
end of a function.

Restructure this to use else's?

[wjia(left Chromium), 2012/07/10 17:32:28]

Done.

     }
 
     if (device_info_available_) {
       handler->OnDeviceInfoReceived(this, device_info_);
     }
 
     clients_[handler] = capability;
     return;
   }
 
@@ skipping 25 matching lines @@
   if (it != clients_pending_on_filter_.end()) {
     handler->OnStopped(this);
     handler->OnRemoved(this);
     clients_pending_on_filter_.erase(it);
     return;
   }
   it = clients_pending_on_restart_.find(handler);
   if (it != clients_pending_on_restart_.end()) {
     handler->OnStopped(this);
     handler->OnRemoved(this);
     clients_pending_on_filter_.erase(it);

[zunger, 2012/07/09 20:08:26]

Don't you mean clients_pending_on_restart_?

Maybe this repeated code block should be a small helper function, because it
looks like the entire function is

MaybeRemove(handler, &clients_pending_on_filter_) ||
MaybeRemove(handler, &clients_pending_on_restart_) ||
MaybeRemove(handler, &clients_);

Also, the thread-safety of this class is now confusing me completely, since it
seems to be doing a lot of complexly asynchronous stuff but there's no locking
anywhere. How does this work?

[wjia(left Chromium), 2012/07/10 17:32:28]

Good catch! Thanks! It should be clients_pending_on_restart_. Hopefully, with
all worker functions added "OnCaptureThread" suffix, it's better to understand
the threading of this class.

     return;
   }
 
   if (clients_.find(handler) == clients_.end())
     return;
 
   clients_.erase(handler);
 
   if (clients_.empty()) {
     DVLOG(1) << "StopCapture: No more client, stopping ...";
     StopDevice();
   }
   handler->OnStopped(this);
   handler->OnRemoved(this);
 }
 
 void VideoCaptureImpl::DoFeedBuffer(scoped_refptr<VideoFrameBuffer> buffer) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   CachedDIB::iterator it;
   for (it = cached_dibs_.begin(); it != cached_dibs_.end(); it++) {

[zunger, 2012/07/09 20:08:26]

++it !!

[wjia(left Chromium), 2012/07/10 17:32:28]

Done.

     if (buffer == it->second->mapped_memory)
       break;
   }
 
   DCHECK(it != cached_dibs_.end());

[zunger, 2012/07/09 20:08:26]

NB that this will crash the address space in debug mode, and plow on to crashing
the address space on line 258 otherwise. I assume this is not what you actually
want to happen.

[wjia(left Chromium), 2012/07/10 17:32:28]

The clients are supposed to return only buffers that are sent to them. This is
the DCHECK for. But somehow, cached_dibs_ could be cleared before clients fully
return buffers. So I changed the logic to check if returned buffer is in
cached_dibs_.

   DCHECK_GT(it->second->references, 0);
   it->second->references--;

[zunger, 2012/07/09 20:08:26]

predecrement, but I'm pretty sure that this is not what you want -- you want a
real refcount here. Or maybe this should just be a shared_ptr?

[wjia(left Chromium), 2012/07/10 17:32:28]

This "references" means the number of clients which are holding a handle of this
buffer. VideoCaptureImpl can not return the buffer to browser process when
"references" is not zero. 

   if (it->second->references == 0) {
     Send(new VideoCaptureHostMsg_BufferReady(device_id_, it->first));
   }
 }
 
 void VideoCaptureImpl::DoBufferCreated(
     base::SharedMemoryHandle handle,
     int length, int buffer_id) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK(device_info_available_);
@@ skipping 40 matching lines @@
     case video_capture::kStarted:
       break;
     case video_capture::kStopped:
       state_ = video_capture::kStopped;
       DVLOG(1) << "OnStateChanged: stopped!, device_id = " << device_id_;
       STLDeleteValues(&cached_dibs_);
       if (!clients_.empty() || !clients_pending_on_restart_.empty())
         RestartCapture();
       break;
     case video_capture::kPaused:
       for (ClientInfo::iterator it = clients_.begin();

[zunger, 2012/07/09 20:08:26]

You can now use the 'auto' keyword here.

[wjia(left Chromium), 2012/07/10 17:32:28]

I tried to add "auto" here, but the compiler says:
error: ‘auto’ will change meaning in C++0x; please remove it

            it != clients_.end(); it++) {

[zunger, 2012/07/09 20:08:26]

preincrement!!

[wjia(left Chromium), 2012/07/10 17:32:28]

Done.

         it->first->OnPaused(this);
       }
       break;
     case video_capture::kError:
       DVLOG(1) << "OnStateChanged: error!, device_id = " << device_id_;
       for (ClientInfo::iterator it = clients_.begin();
            it != clients_.end(); it++) {
         // TODO(wjia): browser process would send error code.
         it->first->OnError(this, 1);
         it->first->OnRemoved(this);
@@ skipping 43 matching lines @@
     Send(new VideoCaptureHostMsg_Stop(device_id_));
     current_params_.width = current_params_.height = 0;
   }
 }
 
 void VideoCaptureImpl::RestartCapture() {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK_EQ(state_, video_capture::kStopped);
 
   int width = 0;
   int height = 0;

[zunger, 2012/07/09 20:08:26]

Shouldn't you do this loop second, so that if one client gets stomped by another
in the restart loop, you don't accidentally grab a width or height from a client
that's no longer in the list?

[wjia(left Chromium), 2012/07/10 17:32:28]

No client is supposed to have same address as another client. So in the restart
loop, all clients_pending_on_restart_ will be added as new clients into
clients_.

   for (ClientInfo::iterator it = clients_.begin();
        it != clients_.end(); it++) {
     if (it->second.width > width)

[zunger, 2012/07/09 20:08:26]

with = max(width, it->second.width);

[wjia(left Chromium), 2012/07/10 17:32:28]

Done.

       width = it->second.width;
     if (it->second.height > height)
       height = it->second.height;
   }
   for (ClientInfo::iterator it = clients_pending_on_restart_.begin();
        it != clients_pending_on_restart_.end(); ) {
     if (it->second.width > width)
       width = it->second.width;
     if (it->second.height > height)
       height = it->second.height;
@@ skipping 33 matching lines @@
 }
 
 bool VideoCaptureImpl::ClientHasDIB() {
   CachedDIB::iterator it;
   for (it = cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
     if (it->second->references > 0)
       return true;
   }
   return false;
 }