for readability review.

for readability review.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146516

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

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:4: 
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.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:27: int references;
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)

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:64:
STLDeleteContainerPairSecondPointers(cached_dibs_.begin(),
STLDeleteValues(&cached_dibs_);

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:74: return;
I would use an 'else' instead of a 'return,' it makes the branched flow of
control clearer.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:102: base::Unretained(this),
handler, capability));
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)

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:161: ClientInfo::iterator it1 =
clients_pending_on_filter_.find(handler);
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)

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:184: return;
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?

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:230:
clients_pending_on_filter_.erase(it);
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?

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:251: for (it =
cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
++it !!

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:256: DCHECK(it !=
cached_dibs_.end());
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.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:258: it->second->references--;
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?

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:319: for (ClientInfo::iterator it =
clients_.begin();
You can now use the 'auto' keyword here.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:320: it != clients_.end(); it++) {
preincrement!!

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:384: int height = 0;
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?

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:387: if (it->second.width > width)
with = max(width, it->second.width);

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.h:24: 
What's the thread-safety of this class?

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.h:26: : public media::VideoCapture,
public VideoCaptureMessageFilter::Delegate {
I don't know if the Chromium codebase is somehow exempt from core bits of the
style guide, but the style guide forbids multiple inheritance except within
certain very narrowly defined guidelines. 

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Multiple_Inheritance#...

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.h:91: typedef std::map<int /*
buffer_id */, DIBBuffer*> CachedDIB;
/* */ comments forbidden.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl_unittest.cc (right):

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl_unittest.cc:74:
IPC_BEGIN_MESSAGE_MAP(MockVideoCaptureImpl, *message)
Do these helpers not want trailing semicolons for some reason?

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

Thanks! To clarify threading issue, the main working thread of VideoCaptureImpl
is capture thread represented by |capture_message_loop_proxy_|. Most of data
processing tasks are done on that thread by posting a task onto that thread.
Therefore, there is no need to use atomic operations. The API between
VideoCaptureImpl and VideoCaptureMessageFilter is mainly on IO thread.

I add "OnCaptureThread" to most worker functions in order to make threading more
clear.

Please take another look.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:27: int references;
On 2012/07/09 20:08:26, zunger wrote:
> 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)
Yes, DIBBuffer's are accessed only on capture thread. There should be no racing
to access its members.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:64:
STLDeleteContainerPairSecondPointers(cached_dibs_.begin(),
On 2012/07/09 20:08:26, zunger wrote:
> STLDeleteValues(&cached_dibs_);

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:74: return;
On 2012/07/09 20:08:26, zunger wrote:
> I would use an 'else' instead of a 'return,' it makes the branched flow of
> control clearer.

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:102: base::Unretained(this),
handler, capability));
On 2012/07/09 20:08:26, zunger wrote:
> 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)

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

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:161: ClientInfo::iterator it1 =
clients_pending_on_filter_.find(handler);
On 2012/07/09 20:08:26, zunger wrote:
> 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)

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.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:184: return;
On 2012/07/09 20:08:26, zunger wrote:
> 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?

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:230:
clients_pending_on_filter_.erase(it);
On 2012/07/09 20:08:26, zunger wrote:
> 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?
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.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:251: for (it =
cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
On 2012/07/09 20:08:26, zunger wrote:
> ++it !!

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:256: DCHECK(it !=
cached_dibs_.end());
On 2012/07/09 20:08:26, zunger wrote:
> 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.

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

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:258: it->second->references--;
On 2012/07/09 20:08:26, zunger wrote:
> 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?

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.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:319: for (ClientInfo::iterator it =
clients_.begin();
On 2012/07/09 20:08:26, zunger wrote:
> You can now use the 'auto' keyword here.

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

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:320: it != clients_.end(); it++) {
On 2012/07/09 20:08:26, zunger wrote:
> preincrement!!

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:384: int height = 0;
On 2012/07/09 20:08:26, zunger wrote:
> 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?

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

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.cc:387: if (it->second.width > width)
On 2012/07/09 20:08:26, zunger wrote:
> with = max(width, it->second.width);

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.h:24: 
On 2012/07/09 20:08:26, zunger wrote:
> What's the thread-safety of this class?
All data processing happens on capture thread which is represented by
|capture_message_loop_proxy_|. Therefore, no lock/semaphore is used in this
class.

Whenever a client calls StartCapture or StopCapture, or
VideoCaptureMessageFilter calls its delegate functions, VideoCaptureImpl will
post a task to capture thread. This makes data access safe.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.h:26: : public media::VideoCapture,
public VideoCaptureMessageFilter::Delegate {
On 2012/07/09 20:08:26, zunger wrote:
> I don't know if the Chromium codebase is somehow exempt from core bits of the
> style guide, but the style guide forbids multiple inheritance except within
> certain very narrowly defined guidelines. 
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Multiple_Inheritance#...

Both base classes are pure interfaces. Please refer to
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/video_...
and
http://src.chromium.org/viewvc/chrome/trunk/src/media/video/capture/video_cap...

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl.h:91: typedef std::map<int /*
buffer_id */, DIBBuffer*> CachedDIB;
On 2012/07/09 20:08:26, zunger wrote:
> /* */ comments forbidden.

Done.

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl_unittest.cc (right):

http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl_unittest.cc:74:
IPC_BEGIN_MESSAGE_MAP(MockVideoCaptureImpl, *message)
On 2012/07/09 20:08:26, zunger wrote:
> Do these helpers not want trailing semicolons for some reason?

That macro has a trailing semicolon. Please refer to
http://src.chromium.org/viewvc/chrome/trunk/src/ipc/ipc_message_macros.h?cont...

[zunger, 2012-07-10 23:34:30]

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:156: } else if
((clients_pending_on_filter_.find(handler) !=
Oh, wow. This function is a *lot* clearer now.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:188: DCHECK_EQ(clients_.size(),
1ul);
Expected value has to be on the left or the error messages won't come out right.
Also, what's with the 1ul business? Is '1' not compiling properly?

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:204: 
You may want to comment why this is an OR and we're short-circuiting.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:404: for (it =
cached_dibs_.begin(); it != cached_dibs_.end(); ++it) {
for (CachedDIB::iterator it ...)

or even

for (const auto& it : cached_dibs_) {
  if (it->second->references > 0) return true;
}

Keep variable scopes as tight as possible.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:413: ClientInfo& clients) {
No non-const references! Use a pointer.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:413: ClientInfo& clients) {
This function can be const. (Make every function const unless there's an amazing
reason for it not to be)

[zunger, 2012-07-10 23:34:44]

On Tue, Jul 10, 2012 at 10:32 AM, <wjia@chromium.org> wrote:

> Reviewers: zunger,
>
> Message:
> Thanks! To clarify threading issue, the main working thread of
> VideoCaptureImpl
> is capture thread represented by |capture_message_loop_proxy_|. Most of
> data
> processing tasks are done on that thread by posting a task onto that
> thread.
> Therefore, there is no need to use atomic operations. The API between
> VideoCaptureImpl and VideoCaptureMessageFilter is mainly on IO thread.
>
> I add "OnCaptureThread" to most worker functions in order to make
> threading more
> clear.
>
> Please take another look.


Implementing thread-safety via having functions run on specific threads is
very fragile, and
tends to scale poorly. (You can't make things faster by adding more
threads, for example)
I'd use this technique with great care.

If you do this, then you need to document each instance variable's
thread-access semantics:
which threads it's read from and written to. Furthermore, you need to deal
with memory
barriers, which is why I was suggesting that you need to use atomic ops --
for example, if an
int is written on one core and read on the other, there's absolutely no
guarantee of what value
the other one will see, even a surprising amount of time later.

So you can do this, but I'd note that once you've documented those
semantics, you've basically
documented the semantics of a mutex anyway and should consider simply doing
that.

Also, overall thread-safety of the class needs to be documented; that's a
property of how other
classes interact with this one. (e.g., if it's thread-safe then multiple
threads can call its methods
simultaneously without fear; if it's thread-compatible, then it needs to be
locked as though it were
an int, read locks for const functions and write locks for non-const ones)

>
>
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc>
> File content/renderer/media/video_**capture_impl.cc (right):
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode27<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode27>
> content/renderer/media/video_**capture_impl.cc:27: int references;
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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)
>>
> Yes, DIBBuffer's are accessed only on capture thread. There should be no
> racing to access its members.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode64<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode64>
> content/renderer/media/video_**capture_impl.cc:64:
> STLDeleteContainerPairSecondPo**inters(cached_dibs_.begin(),
> On 2012/07/09 20:08:26, zunger wrote:
>
>> STLDeleteValues(&cached_dibs_)**;
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode74<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode74>
> content/renderer/media/video_**capture_impl.cc:74: return;
> On 2012/07/09 20:08:26, zunger wrote:
>
>> I would use an 'else' instead of a 'return,' it makes the branched
>>
> flow of
>
>> control clearer.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode102<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode102>
> content/renderer/media/video_**capture_impl.cc:102:
> base::Unretained(this), handler, capability));
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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)
>>
>
> In Chromium, base::Bind does make a copy of const reference when it
> creates async task. So it's safe here.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode161<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode161>
> content/renderer/media/video_**capture_impl.cc:161: ClientInfo::iterator
> it1 = clients_pending_on_filter_.**find(handler);
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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)
>
> 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.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode184<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode184>
> content/renderer/media/video_**capture_impl.cc:184: return;
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode230<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode230>
> content/renderer/media/video_**capture_impl.cc:230:
> clients_pending_on_filter_.**erase(it);
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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?
>>
> 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.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode251<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode251>
> content/renderer/media/video_**capture_impl.cc:251: for (it =
> cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
> On 2012/07/09 20:08:26, zunger wrote:
>
>> ++it !!
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode256<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode256>
> content/renderer/media/video_**capture_impl.cc:256: DCHECK(it !=
> cached_dibs_.end());
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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.
>>
>
> 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_.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode258<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode258>
> content/renderer/media/video_**capture_impl.cc:258:
> it->second->references--;
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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?
>>
>
> 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.


That would be a very useful thing to comment. :)

>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode319<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode319>
> content/renderer/media/video_**capture_impl.cc:319: for
> (ClientInfo::iterator it = clients_.begin();
> On 2012/07/09 20:08:26, zunger wrote:
>
>> You can now use the 'auto' keyword here.
>>
>
> I tried to add "auto" here, but the compiler says:
> error: ‘auto’ will change meaning in C++0x; please remove it


Ah, it's possible that Chromium isn't yet allowing 'auto;' we just changed
the google3 style rules to allow it in
general code. It's a lovely, lovely keyword.

>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode320<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode320>
> content/renderer/media/video_**capture_impl.cc:320: it != clients_.end();
> it++) {
> On 2012/07/09 20:08:26, zunger wrote:
>
>> preincrement!!
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode384<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode384>
> content/renderer/media/video_**capture_impl.cc:384: int height = 0;
> On 2012/07/09 20:08:26, zunger wrote:
>
>> 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?
>>
>
> 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_.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.cc#newcode387<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.cc#newcode387>
> content/renderer/media/video_**capture_impl.cc:387: if (it->second.width >
> width)
> On 2012/07/09 20:08:26, zunger wrote:
>
>> with = max(width, it->second.width);
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.h<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.h>
> File content/renderer/media/video_**capture_impl.h (right):
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.h#newcode24<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.h#newcode24>
> content/renderer/media/video_**capture_impl.h:24:
> On 2012/07/09 20:08:26, zunger wrote:
>
>> What's the thread-safety of this class?
>>
> All data processing happens on capture thread which is represented by
> |capture_message_loop_proxy_|. Therefore, no lock/semaphore is used in
> this class.
>
> Whenever a client calls StartCapture or StopCapture, or
> VideoCaptureMessageFilter calls its delegate functions, VideoCaptureImpl
> will post a task to capture thread. This makes data access safe.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.h#newcode26<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.h#newcode26>
> content/renderer/media/video_**capture_impl.h:26: : public
> media::VideoCapture, public VideoCaptureMessageFilter::**Delegate {
> On 2012/07/09 20:08:26, zunger wrote:
>
>> I don't know if the Chromium codebase is somehow exempt from core bits
>>
> of the
>
>> style guide, but the style guide forbids multiple inheritance except
>>
> within
>
>> certain very narrowly defined guidelines.
>>
>
>
> http://www.corp.google.com/**eng/doc/cppguide.xml?showone=**
>
Multiple_Inheritance#Multiple_**Inheritance<http://www.corp.google.com/eng/doc/cppguide.xml?showone=Multiple_Inheritance#Multiple_Inheritance>
>
> Both base classes are pure interfaces. Please refer to
> http://src.chromium.org/**viewvc/chrome/trunk/src/**
> content/renderer/media/video_**capture_message_filter.h?**
>
revision=134446&view=markup<http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/video_capture_message_filter.h?revision=134446&view=markup>
> and
> http://src.chromium.org/**viewvc/chrome/trunk/src/media/**
>
video/capture/video_capture.h?**revision=144086&view=markup<http://src.chromium.org/viewvc/chrome/trunk/src/media/video/capture/video_capture.h?revision=144086&view=markup>
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl.h#newcode91<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl.h#newcode91>
> content/renderer/media/video_**capture_impl.h:91: typedef std::map<int /*
> buffer_id */, DIBBuffer*> CachedDIB;
> On 2012/07/09 20:08:26, zunger wrote:
>
>> /* */ comments forbidden.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl_unittest.cc<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl_unittest.cc>
> File content/renderer/media/video_**capture_impl_unittest.cc (right):
>
> http://codereview.chromium.**org/10451087/diff/1/content/**
>
renderer/media/video_capture_**impl_unittest.cc#newcode74<http://codereview.chromium.org/10451087/diff/1/content/renderer/media/video_capture_impl_unittest.cc#newcode74>
> content/renderer/media/video_**capture_impl_unittest.cc:74:
> IPC_BEGIN_MESSAGE_MAP(**MockVideoCaptureImpl, *message)
> On 2012/07/09 20:08:26, zunger wrote:
>
>> Do these helpers not want trailing semicolons for some reason?
>>
>
> That macro has a trailing semicolon. Please refer to
> http://src.chromium.org/**viewvc/chrome/trunk/src/ipc/**
>
ipc_message_macros.h?content-**type=text%2Fplain<http://src.chromium.org/viewvc/chrome/trunk/src/ipc/ipc_message_macros.h?content-type=text%2Fplain>
>


>
> Description:
> for readability review.
>
> Please review this at
http://codereview.chromium.**org/10451087/<http://codereview.chromium.org/104...
>
> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>
> Affected files:
>   M     content/renderer/media/video_**capture_impl.h
>   M     content/renderer/media/video_**capture_impl.cc
>   M     content/renderer/media/video_**capture_impl_unittest.cc
>
>
>

[wjia(left Chromium), 2012-07-12 22:57:30]

PTAL. More comments about threading have been added, such as how interfaces can
be called, how members are accessed. Also try to use const as much as possible,
including members.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:188: DCHECK_EQ(clients_.size(),
1ul);
On 2012/07/10 23:34:30, zunger wrote:
> Expected value has to be on the left or the error messages won't come out
right.
> Also, what's with the 1ul business? Is '1' not compiling properly?

I checked how DCHECK_EQ is defined in Chromium and couldn't find the difference
of putting expected value on the left or right. Anyhow, the expected value has
been moved to left.

The default type for "1" is int, while size() is uint. Therefore "1ul" is used.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:204: 
On 2012/07/10 23:34:30, zunger wrote:
> You may want to comment why this is an OR and we're short-circuiting.

I am not sure if I understand the comment. This function should be run on
capture thread. This DCHECK is a safety checking for that.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:404: for (it =
cached_dibs_.begin(); it != cached_dibs_.end(); ++it) {
On 2012/07/10 23:34:30, zunger wrote:
> for (CachedDIB::iterator it ...)
> 
> or even
> 
> for (const auto& it : cached_dibs_) {
>   if (it->second->references > 0) return true;
> }
> 
> Keep variable scopes as tight as possible.

Done. Can not use auto since the compiler doesn't support it.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:413: ClientInfo& clients) {
On 2012/07/10 23:34:30, zunger wrote:
> No non-const references! Use a pointer.

Done.

http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:413: ClientInfo& clients) {
On 2012/07/10 23:34:30, zunger wrote:
> This function can be const. (Make every function const unless there's an
amazing
> reason for it not to be)
This function RemoveClient() can't be const since handler->OnStopped() needs
non-const pointer. I made ClientHasDIB() const.

[zunger, 2012-07-13 00:45:27]

On Thu, Jul 12, 2012 at 3:57 PM, <wjia@chromium.org> wrote:

> PTAL. More comments about threading have been added, such as how
> interfaces can
> be called, how members are accessed. Also try to use const as much as
> possible,
> including members.
>
>
>
> http://codereview.chromium.**org/10451087/diff/5001/**
>
content/renderer/media/video_**capture_impl.cc<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc>
> File content/renderer/media/video_**capture_impl.cc (right):
>
> http://codereview.chromium.**org/10451087/diff/5001/**
>
content/renderer/media/video_**capture_impl.cc#newcode188<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode188>
> content/renderer/media/video_**capture_impl.cc:188:
> DCHECK_EQ(clients_.size(), 1ul);
> On 2012/07/10 23:34:30, zunger wrote:
>
>> Expected value has to be on the left or the error messages won't come
>>
> out right.
>
>> Also, what's with the 1ul business? Is '1' not compiling properly?
>>
>
> I checked how DCHECK_EQ is defined in Chromium and couldn't find the
> difference of putting expected value on the left or right. Anyhow, the
> expected value has been moved to left.
>
> The default type for "1" is int, while size() is uint. Therefore "1ul"
> is used.
>

The difference is in the error message that it outputs – it labels the
first argument as
"expected".

>
> http://codereview.chromium.**org/10451087/diff/5001/**
>
content/renderer/media/video_**capture_impl.cc#newcode204<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode204>
> content/renderer/media/video_**capture_impl.cc:204:
>
> On 2012/07/10 23:34:30, zunger wrote:
>
>> You may want to comment why this is an OR and we're short-circuiting.
>>
>
> I am not sure if I understand the comment. This function should be run
> on capture thread. This DCHECK is a safety checking for that.


Why it is that if one of those returns true, we don't bother running the
others. It's not going to be obvious to the casual reader.

>
>
> http://codereview.chromium.**org/10451087/diff/5001/**
>
content/renderer/media/video_**capture_impl.cc#newcode404<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode404>
> content/renderer/media/video_**capture_impl.cc:404: for (it =
> cached_dibs_.begin(); it != cached_dibs_.end(); ++it) {
> On 2012/07/10 23:34:30, zunger wrote:
>
>> for (CachedDIB::iterator it ...)
>>
>
>  or even
>>
>
>  for (const auto& it : cached_dibs_) {
>>    if (it->second->references > 0) return true;
>> }
>>
>
>  Keep variable scopes as tight as possible.
>>
>
> Done. Can not use auto since the compiler doesn't support it.
>
>
> http://codereview.chromium.**org/10451087/diff/5001/**
>
content/renderer/media/video_**capture_impl.cc#newcode413<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode413>
> content/renderer/media/video_**capture_impl.cc:413: ClientInfo& clients) {
> On 2012/07/10 23:34:30, zunger wrote:
>
>> No non-const references! Use a pointer.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10451087/diff/5001/**
>
content/renderer/media/video_**capture_impl.cc#newcode413<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode413>
> content/renderer/media/video_**capture_impl.cc:413: ClientInfo& clients) {
> On 2012/07/10 23:34:30, zunger wrote:
>
>> This function can be const. (Make every function const unless there's
>>
> an amazing
>
>> reason for it not to be)
>>
> This function RemoveClient() can't be const since handler->OnStopped()
> needs non-const pointer. I made ClientHasDIB() const.
>
>
http://codereview.chromium.**org/10451087/<http://codereview.chromium.org/104...
>

[wjia(left Chromium), 2012-07-13 01:06:54]

On 2012/07/13 00:45:27, zunger wrote:
> On Thu, Jul 12, 2012 at 3:57 PM, <mailto:wjia@chromium.org> wrote:
> 
> > PTAL. More comments about threading have been added, such as how
> > interfaces can
> > be called, how members are accessed. Also try to use const as much as
> > possible,
> > including members.
> >
> >
> >
> > http://codereview.chromium.**org/10451087/diff/5001/**
> >
>
content/renderer/media/video_**capture_impl.cc<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc>
> > File content/renderer/media/video_**capture_impl.cc (right):
> >
> > http://codereview.chromium.**org/10451087/diff/5001/**
> >
>
content/renderer/media/video_**capture_impl.cc#newcode188<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode188>
> > content/renderer/media/video_**capture_impl.cc:188:
> > DCHECK_EQ(clients_.size(), 1ul);
> > On 2012/07/10 23:34:30, zunger wrote:
> >
> >> Expected value has to be on the left or the error messages won't come
> >>
> > out right.
> >
> >> Also, what's with the 1ul business? Is '1' not compiling properly?
> >>
> >
> > I checked how DCHECK_EQ is defined in Chromium and couldn't find the
> > difference of putting expected value on the left or right. Anyhow, the
> > expected value has been moved to left.
> >
> > The default type for "1" is int, while size() is uint. Therefore "1ul"
> > is used.
> >
> 
> The difference is in the error message that it outputs – it labels the
> first argument as
> "expected".

Thanks!

> 
> >
> > http://codereview.chromium.**org/10451087/diff/5001/**
> >
>
content/renderer/media/video_**capture_impl.cc#newcode204<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode204>
> > content/renderer/media/video_**capture_impl.cc:204:
> >
> > On 2012/07/10 23:34:30, zunger wrote:
> >
> >> You may want to comment why this is an OR and we're short-circuiting.
> >>
> >
> > I am not sure if I understand the comment. This function should be run
> > on capture thread. This DCHECK is a safety checking for that.
> 
> 
> Why it is that if one of those returns true, we don't bother running the
> others. It's not going to be obvious to the casual reader.
> 

ah, now I see you are talking about the RemoveClient() calls. I thought you mean
DCHECK. That's why I got lost.

Yes, some comments have been added.

> >
> >
> > http://codereview.chromium.**org/10451087/diff/5001/**
> >
>
content/renderer/media/video_**capture_impl.cc#newcode404<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode404>
> > content/renderer/media/video_**capture_impl.cc:404: for (it =
> > cached_dibs_.begin(); it != cached_dibs_.end(); ++it) {
> > On 2012/07/10 23:34:30, zunger wrote:
> >
> >> for (CachedDIB::iterator it ...)
> >>
> >
> >  or even
> >>
> >
> >  for (const auto& it : cached_dibs_) {
> >>    if (it->second->references > 0) return true;
> >> }
> >>
> >
> >  Keep variable scopes as tight as possible.
> >>
> >
> > Done. Can not use auto since the compiler doesn't support it.
> >
> >
> > http://codereview.chromium.**org/10451087/diff/5001/**
> >
>
content/renderer/media/video_**capture_impl.cc#newcode413<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode413>
> > content/renderer/media/video_**capture_impl.cc:413: ClientInfo& clients) {
> > On 2012/07/10 23:34:30, zunger wrote:
> >
> >> No non-const references! Use a pointer.
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.**org/10451087/diff/5001/**
> >
>
content/renderer/media/video_**capture_impl.cc#newcode413<http://codereview.chromium.org/10451087/diff/5001/content/renderer/media/video_capture_impl.cc#newcode413>
> > content/renderer/media/video_**capture_impl.cc:413: ClientInfo& clients) {
> > On 2012/07/10 23:34:30, zunger wrote:
> >
> >> This function can be const. (Make every function const unless there's
> >>
> > an amazing
> >
> >> reason for it not to be)
> >>
> > This function RemoveClient() can't be const since handler->OnStopped()
> > needs non-const pointer. I made ClientHasDIB() const.
> >
> >
>
http://codereview.chromium.**org/10451087/%3Chttp://codereview.chromium.org/1...>
> >

[zunger, 2012-07-13 01:09:46]

lgtm

[commit-bot: I haz the power, 2012-07-13 01:09:57]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.