Refactor video capturing code in the render process

Refactor video capturing code in the render process

This is a large refactoring to cleanup the code that handles video
capturing in the render process. The goal of this change is to:
* Simplify threading model for objects involved.
* Clarify ownership model.
* Remove extra complexity caused by media::VideoCapture.

Summary of this change:
* Interface media::VideoCapture is removed completely.
  This interface doesn't add much value. It fails to define threading
  model and ownership. Some of the methods are obsolete. 
* Pepper code that performs video capturing now do not inherit from
  media::VideoCapture.
  The inheritance is not necessary
* VideoCaptureImpl is now a purely IO thread object.
  VideoCaptureImpl can only be accessed on the IO thread. It now becomes
  and inner object of VideoCaptureImplManager. Client is not allowed to
  access this object directly. This helps remove code that accepts
  call from the render thread and hopping to the IO thread. This also
  makes cleanup much simpler.
* VideoCaptureHandle is removed.
  The function of VideoCaptureHandle, i.e. handle cleanup of video
  capture resource is now folded into VideoCaptureImplManager. It's
  function is now replaced by a closure.
* VideoCaptureImplManager becomes the public interface for accessing
  video capture device and start/stop capture.
  It takes VideoCaptureImpl as an internal object and sheild it from
  clients. We can now perform cleanup to prevent leak. Also ensures
  VideoCaptureImpl objects are deleted on the IO thread.
* VideoFrames delivery done using callback insteasd of an interface.
  Using callback to deliver VideoFrames and state changes make thread
  hopping much simpler. Clients no longer need to provide an
  EventHandler interface.
* Net deleted 450 lines of code.

Tested with apprtc.appspot.com and example pepper plugin.
Additional test to verify there's no leakage of VideoCaptureImpl objects.

BUG=335327, 362558
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266492

[Alpha Left Google, 2014-04-17 22:47:13]

ifh: Please review pepper related code.
fischman: Everything else.
perkj: cc'ed.

[Alpha Left Google, 2014-04-18 21:20:38]

Ping reviewers.

[Ami GONE FROM CHROMIUM, 2014-04-18 21:24:46]

On Fri, Apr 18, 2014 at 2:20 PM, <hclam@chromium.org> wrote:

> Ping reviewers.
>
>
Remember last night when I was leaving and I told you: "it's gonna be a
while before I can look at your CL"? :)
("a while" in this case meaning "probably not before next monday")


>
> https://codereview.chromium.org/242013002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-04-18 21:28:04]

> Remember last night when I was leaving and I told you: "it's gonna be a
> while before I can look at your CL"? :)
> ("a while" in this case meaning "probably not before next monday")

Okay. A unit for time would be useful. :)

Please let me know if there's conflicting changes that you see.

[ilja, 2014-04-21 21:39:58]

This looks much simpler, thank you. I did not see anything remarkable. But it
has been a very long time since I last looked at this code, sorry. I tried your
patch on Pixel with Flash and it worked as before.

[Ami GONE FROM CHROMIUM, 2014-04-21 23:42:52]

I <3 this CL.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:85:
RenderThreadImpl::current()->video_capture_impl_manager();
don't need to worry about RTI::current() being NULL in tests like l.44?  Maybe
l.44 is outdated now?

(ditto elsewhere below)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:121:
stop_capture_cb_.Reset();
base::ResetAndReturn(&stop_capture_cb_);
makes it easier to see that the Run() is safe even if stop_capture_cb_ would be
accessed during the Run().

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:145: if (state ==
VIDEO_CAPTURE_STATE_ERROR && !started_callback_.is_null()) {
what happens if error is delivered before StartCapture()?
(seems like a DCHECK is in order here instead of the if?)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:17: // and receive
I420 frames from Chrome's video capture implementation.
please doco lifecycle/threading.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:25:
SupportedFormatsCallback;
These typedefs have a non-trivial overlap with
content/common/media/video_capture.h
Can you use those instead?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:43: virtual void
StartDeliver(
VerbVerb is not a particularly readable choice (predates this CL).  How is this
different to StartCapture/StopCapture?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:89: } else if
((clients_pending_on_filter_.find(client_id) !=
map_.find(foo) != map_.end()
is shorter to write as
map_.count(foo)
(and avoids the pitfall of accidentally specifying a different map_ the second
time)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:94: // This client has started.
what about the callbacks?  This seems like it should be a DFATAL.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:114: DCHECK_EQ(1ul,
clients_.size());
'l' should not be necessary here.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:114: DCHECK_EQ(1ul,
clients_.size());
what prevents two clients from registering almost-simultaneously?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:137: RemoveClient(client_id,
&clients_);
ick to using boolean short-cutting like this (in a context where the
expression's value is dropped)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:304: case
VIDEO_CAPTURE_STATE_STARTED:
seems strange that this is a no-op.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:410: }
if you did 
clients_.insert(clients_pending_on_restart_.begin(),
clients_pending_on_restart_.end());
at l.395 then you could delete this loop.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:440: return found;
l.432-440 can be replaced with
return clients->erase(client_id) != 0;

(and, given the only callsite's ick comment above, if it were me I'd drop this
helper function altogether in favor of
size_t erased = clients_->erase(client_id) +
    clients_at_blah_->erase(client_id) +
    clients_other_foo->erase(client_id);
DCHECK_EQ(erased, 1);
)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:13: // VideoCaptureImpl is a IO
thread only object. See the comments in
s/a IO/an IO/

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:52: friend class
VideoCaptureImplManager;
why this?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:56: class ClientBuffer;
please doco all structs/classes.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:79: // |client_id| must be unique to
this object. It is used later to
"unique" in the tab/renderer/browser ?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:81: // |status_update_cb| will be
called when state changes.
s/status/state/

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:89: // Stop capturing. |client_id|
is an identifier used to call StartCapture.
s/an/the/

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:91: bool CaptureStarted();
why is this needed, given the state_updated_cb?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:91: bool CaptureStarted();
\n between methods, and doco all methods that aren't interface impls.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:172: // Bound to the thread where
Init() is called. It should be the IO thread.
should go above the weakptrfactory above (see the NOTE)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:38:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
ChildProcess::current()->io_message_loop_proxy()
is
MessageLoopProxy::current()

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:40:
base::Bind(&VideoCaptureImpl::DeInit,
why not DeInit+delete directly in-line here?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:41:
base::Unretained(impl)));
any "Unretained" use should be covered by an explanation for why it is safe.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:58:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
ditto why post instead of running in-line?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:61:
base::Unretained(impl)));
ditto Unretained

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:64: impl =
it->second.second;
unused?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:82:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
not reviewing the rest of this file since I hope you can simplify it per above
commetns.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:22: // video capture
resources after all clients are destroyed.
Is it possible to use WeakPtrs to avoid letting clients keep this class longer
than desired?

In the absence of this I worry about RTI giving up its ref, but this VCIM
outliving RTI and bad things happening.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:51: // Access a device
associated with the session ID.
s/Access/Open/
(or Use or something else more active)

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:54: // Returns a callback
that is used to release the acquired resources.
s/is/should be/

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:72: // callbacks are called
after capturing is instructed to stop.
s/./, typically by binding the passed callbacks on a WeakPtr./

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:79: // Get supported formats
by the device for the given session ID.
Get formats supported by the device...

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:102: virtual
VideoCaptureImpl* CreateVideoCaptureImpl(
s/Impl(/ImplForTesting(/

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_video_capture.cc:133: NOTREACHED() <<
"Unexpected state.";
s/."/: " << state/

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_video_capture.h:25: class
PepperPlatformVideoCapture {
Please doco lifecycle/threading assumptions of this class.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_capture_host.cc:121: if (alloc_size_ !=
frame->coded_size() || buffers_.empty()) {
What made this necessary?

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_capture_host.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_capture_host.h:44: void OnStarted();
It's strange to see methods like this not be OVERRIDEs or =0 or documented.  
I wonder whether you could get away with exposing only getters for the CB
(bound) versions of these methods?

[Alpha Left Google, 2014-04-23 02:29:20]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-23 02:29:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/80001

[commit-bot: I haz the power, 2014-04-23 02:29:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-23 02:29:37]

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.

[Alpha Left Google, 2014-04-23 03:03:12]

On 2014/04/23 02:29:37, I haz the power (commit-bot) wrote:
> 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.

Oh whoooops I checked the wrong box. Meant to run more tries. :)

[Alpha Left Google, 2014-04-23 03:03:12]

On 2014/04/23 02:29:37, I haz the power (commit-bot) wrote:
> 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.

Oh whoooops I checked the wrong box. Meant to run more tries. :)

[Alpha Left Google, 2014-04-23 18:48:32]

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:85:
RenderThreadImpl::current()->video_capture_impl_manager();
On 2014/04/21 23:42:53, Ami Fischman wrote:
> don't need to worry about RTI::current() being NULL in tests like l.44?  Maybe
> l.44 is outdated now?
> 
> (ditto elsewhere below)

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:121:
stop_capture_cb_.Reset();
On 2014/04/21 23:42:53, Ami Fischman wrote:
> base::ResetAndReturn(&stop_capture_cb_);
> makes it easier to see that the Run() is safe even if stop_capture_cb_ would
be
> accessed during the Run().

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:145: if (state ==
VIDEO_CAPTURE_STATE_ERROR && !started_callback_.is_null()) {
On 2014/04/21 23:42:53, Ami Fischman wrote:
> what happens if error is delivered before StartCapture()?
> (seems like a DCHECK is in order here instead of the if?)

Because stopping of VCIM is not synchronous and we reset started_callback_ in
StopDeliver(). In rare case it is possible that right after calling
StopDeliver() this object is notified of an error in capturing.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:17: // and receive
I420 frames from Chrome's video capture implementation.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> please doco lifecycle/threading.

Done. It is simple right now: it's render thread only.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:25:
SupportedFormatsCallback;
On 2014/04/21 23:42:53, Ami Fischman wrote:
> These typedefs have a non-trivial overlap with
> content/common/media/video_capture.h
> Can you use those instead?

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:43: virtual void
StartDeliver(
On 2014/04/21 23:42:53, Ami Fischman wrote:
> VerbVerb is not a particularly readable choice (predates this CL).  How is
this
> different to StartCapture/StopCapture?

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:89: } else if
((clients_pending_on_filter_.find(client_id) !=
On 2014/04/21 23:42:53, Ami Fischman wrote:
> map_.find(foo) != map_.end()
> is shorter to write as
> map_.count(foo)
> (and avoids the pitfall of accidentally specifying a different map_ the second
> time)

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:94: // This client has started.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> what about the callbacks?  This seems like it should be a DFATAL.

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:114: DCHECK_EQ(1ul,
clients_.size());
On 2014/04/21 23:42:53, Ami Fischman wrote:
> 'l' should not be necessary here.

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:114: DCHECK_EQ(1ul,
clients_.size());
On 2014/04/21 23:42:53, Ami Fischman wrote:
> what prevents two clients from registering almost-simultaneously?

I *think* the intention in this code is to verify that it doesn't start twice.
Added a check for state. Also a test for this.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:137: RemoveClient(client_id,
&clients_);
On 2014/04/21 23:42:53, Ami Fischman wrote:
> ick to using boolean short-cutting like this (in a context where the
> expression's value is dropped)

RemoveClient actually calls the client and give it a stopped state. I have made
this cleaner a bit.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:304: case
VIDEO_CAPTURE_STATE_STARTED:
On 2014/04/21 23:42:53, Ami Fischman wrote:
> seems strange that this is a no-op.

Added a comment.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:410: }
On 2014/04/21 23:42:53, Ami Fischman wrote:
> if you did 
> clients_.insert(clients_pending_on_restart_.begin(),
> clients_pending_on_restart_.end());
> at l.395 then you could delete this loop.

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:440: return found;
On 2014/04/21 23:42:53, Ami Fischman wrote:
> l.432-440 can be replaced with
> return clients->erase(client_id) != 0;
> 
> (and, given the only callsite's ick comment above, if it were me I'd drop this
> helper function altogether in favor of
> size_t erased = clients_->erase(client_id) +
>     clients_at_blah_->erase(client_id) +
>     clients_other_foo->erase(client_id);
> DCHECK_EQ(erased, 1);
> )

I still need it to send the client the stopped signal.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:13: // VideoCaptureImpl is a IO
thread only object. See the comments in
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/a IO/an IO/

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:52: friend class
VideoCaptureImplManager;
On 2014/04/21 23:42:53, Ami Fischman wrote:
> why this?

Probably excessive. Changed it to a more public interface.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:56: class ClientBuffer;
On 2014/04/21 23:42:53, Ami Fischman wrote:
> please doco all structs/classes.

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:79: // |client_id| must be unique to
this object. It is used later to
On 2014/04/21 23:42:53, Ami Fischman wrote:
> "unique" in the tab/renderer/browser ?

renderer. Documented.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:81: // |status_update_cb| will be
called when state changes.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/status/state/

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:89: // Stop capturing. |client_id|
is an identifier used to call StartCapture.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/an/the/

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:91: bool CaptureStarted();
On 2014/04/21 23:42:53, Ami Fischman wrote:
> why is this needed, given the state_updated_cb?

Removed.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:92: int CaptureFrameRate();
Removed too.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:172: // Bound to the thread where
Init() is called. It should be the IO thread.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> should go above the weakptrfactory above (see the NOTE)

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:38:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
On 2014/04/21 23:42:53, Ami Fischman wrote:
> ChildProcess::current()->io_message_loop_proxy()
> is
> MessageLoopProxy::current()

MessageLoopProxy::current() is the IO message loop. This object is render thread
only.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:40:
base::Bind(&VideoCaptureImpl::DeInit,
On 2014/04/21 23:42:53, Ami Fischman wrote:
> why not DeInit+delete directly in-line here?

VCI is an IO thread only object. It is accessed via a task on IO thread only. I
have added comments at the top of this file to clarify.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:41:
base::Unretained(impl)));
On 2014/04/21 23:42:53, Ami Fischman wrote:
> any "Unretained" use should be covered by an explanation for why it is safe.

I documented it at the top of this file.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:58:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
On 2014/04/21 23:42:53, Ami Fischman wrote:
> ditto why post instead of running in-line?

VCIM is render thread only. VideoCaptureImpl is IO thread only.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:61:
base::Unretained(impl)));
On 2014/04/21 23:42:53, Ami Fischman wrote:
> ditto Unretained

Please see comments at the top of this file.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:64: impl =
it->second.second;
On 2014/04/21 23:42:53, Ami Fischman wrote:
> unused?

Removed.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:82:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
On 2014/04/21 23:42:53, Ami Fischman wrote:
> not reviewing the rest of this file since I hope you can simplify it per above
> commetns.

I have changed the access pattern. Instead of VCI expecting calls from different
threads it is now explicitly accessed by VCIM through a task on IO thread. So
the PostTasks are necessary now.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:22: // video capture
resources after all clients are destroyed.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> Is it possible to use WeakPtrs to avoid letting clients keep this class longer
> than desired?
> 
> In the absence of this I worry about RTI giving up its ref, but this VCIM
> outliving RTI and bad things happening.

Good point. Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:51: // Access a device
associated with the session ID.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/Access/Open/
> (or Use or something else more active)

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:54: // Returns a callback
that is used to release the acquired resources.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/is/should be/

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:72: // callbacks are called
after capturing is instructed to stop.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/./, typically by binding the passed callbacks on a WeakPtr./

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:79: // Get supported formats
by the device for the given session ID.
On 2014/04/21 23:42:53, Ami Fischman wrote:
> Get formats supported by the device...

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.h:102: virtual
VideoCaptureImpl* CreateVideoCaptureImpl(
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/Impl(/ImplForTesting(/

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_video_capture.cc:133: NOTREACHED() <<
"Unexpected state.";
On 2014/04/21 23:42:53, Ami Fischman wrote:
> s/."/: " << state/

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_video_capture.h:25: class
PepperPlatformVideoCapture {
On 2014/04/21 23:42:53, Ami Fischman wrote:
> Please doco lifecycle/threading assumptions of this class.

Done.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_capture_host.cc:121: if (alloc_size_ !=
frame->coded_size() || buffers_.empty()) {
On 2014/04/21 23:42:53, Ami Fischman wrote:
> What made this necessary?

I tested this locally: once camera is stopped it cannot be started again.
Probably a bug that's there already.

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_capture_host.h (right):

https://codereview.chromium.org/242013002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_capture_host.h:44: void OnStarted();
On 2014/04/21 23:42:53, Ami Fischman wrote:
> It's strange to see methods like this not be OVERRIDEs or =0 or documented.  
> I wonder whether you could get away with exposing only getters for the CB
> (bound) versions of these methods?

I added documents for these methods. PepperVidoeCaptureHost is accessed by
PapperPlatformVideoCapture directly without callbacks, in other ways than video
frame delivery and state changes. I would like to keep it this way but get rid
of the inheritance.

[Ami GONE FROM CHROMIUM, 2014-04-24 21:04:30]

Yay for less refcounting!

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:48: if (manager)
how can this fail?
(here and elsewhere below)

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:83: if
(!RenderThreadImpl::current())
comment this is for tests (here and elsewhere below where you check for this)

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.h:42: virtual void
StartToDeliver(
From earlier: why is this not StartCapture?

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.h:74: // Accessed on
the thread where StartToDeliver is called.
l.20 told me that this class is only accessed on a single thread.  Is this
comment outdated?

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.h:92: class
CONTENT_EXPORT MediaStreamVideoCapturerSource
doco thread & lifecycle (who owns it, when is it destroyed, etc) for this class.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:87: const base::TimeTicks&
timestamp);
media::VideoFrame has a timestamp field.  Why is this timestamp param required
here in addition?
(requires a comment at least)

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:91: friend class
VideoCaptureImplManager;
Non-test friends are rare in chromium so should have an explanatory comment as
to why friendship is required.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:184: base::ThreadChecker
thread_checker_;
On 2014/04/23 18:48:33, Alpha wrote:
> On 2014/04/21 23:42:53, Ami Fischman wrote:
> > should go above the weakptrfactory above (see the NOTE)
> 
> Done.

You misunderstood my comment; weak_factory_ should be the last member of the
class.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:11: // there is no
reference to it.
This sentence makes me wonder what sort of "reference" you are thinking of here.

I think it would be clearer to say that VCI is deleted via a PostTask to the IO
thread and that immediately thereafter the pointer is dropped in the render
thread so no further posting can happen.
This is only safe if nothing else posts; e.g. VCI mustn't PostTask to itself
(seems like that should be doco'd in VCI).

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:40:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
On 2014/04/23 18:48:33, Alpha wrote:
> On 2014/04/21 23:42:53, Ami Fischman wrote:
> > ChildProcess::current()->io_message_loop_proxy()
> > is
> > MessageLoopProxy::current()
> 
> MessageLoopProxy::current() is the IO message loop. This object is render
thread
> only.

I don't follow.
I said that the construct you're using is equivalent to a shorter construct.
You said that the construct I proposed is the IO message loop.
I believe the construct you're using is also the IO message loop.
What are you saying, again?

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/video_destination_handler.cc:120: // Not all sinks
care about the TimeTicks so it's not a problem now.
ditto comment elsewhere, why wouldn't you use |timestamp| from l.98?

https://codereview.chromium.org/242013002/diff/140001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:25: // This is an render
thread only object. All methods must be accessed on
s/an/a/

https://codereview.chromium.org/242013002/diff/140001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:26: // the render thread
including cosntructor and destructor.
optional: could DCHECK this in the public methods.

[Alpha Left Google, 2014-04-24 22:50:34]

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:48: if (manager)
On 2014/04/24 21:04:31, Ami Fischman wrote:
> how can this fail?
> (here and elsewhere below)

Ownership:

RenderThreadImpl owns Blink owns WebMediaStreamSource owns
MediaStreamVideoCapturerSource owns VideoCaptureDelegate.

RenderThreadImpl also owns VideoCaptureImplManager.

In RenderThreadImpl::Shutdown(), VideoCaptureImplManager is destroyed before
Blink is destroyed. So VideoCaptureDelegate can be alive while manager is dead.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:83: if
(!RenderThreadImpl::current())
On 2014/04/24 21:04:31, Ami Fischman wrote:
> comment this is for tests (here and elsewhere below where you check for this)

Done.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.h:42: virtual void
StartToDeliver(
On 2014/04/24 21:04:31, Ami Fischman wrote:
> From earlier: why is this not StartCapture?

Done.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.h:74: // Accessed on
the thread where StartToDeliver is called.
On 2014/04/24 21:04:31, Ami Fischman wrote:
> l.20 told me that this class is only accessed on a single thread.  Is this
> comment outdated?

This comment is not necessary.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.h:92: class
CONTENT_EXPORT MediaStreamVideoCapturerSource
On 2014/04/24 21:04:31, Ami Fischman wrote:
> doco thread & lifecycle (who owns it, when is it destroyed, etc) for this
class.

Done.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:87: const base::TimeTicks&
timestamp);
On 2014/04/24 21:04:31, Ami Fischman wrote:
> media::VideoFrame has a timestamp field.  Why is this timestamp param required
> here in addition?
> (requires a comment at least)

Done.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:87: const base::TimeTicks&
timestamp);
On 2014/04/24 21:04:31, Ami Fischman wrote:
> media::VideoFrame has a timestamp field.  Why is this timestamp param required
> here in addition?
> (requires a comment at least)

It is there to allow synchronization with other sources. Added a comment.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:91: friend class
VideoCaptureImplManager;
On 2014/04/24 21:04:31, Ami Fischman wrote:
> Non-test friends are rare in chromium so should have an explanatory comment as
> to why friendship is required.

This is not needed any more. Forgot to remove.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:184: base::ThreadChecker
thread_checker_;
On 2014/04/24 21:04:31, Ami Fischman wrote:
> On 2014/04/23 18:48:33, Alpha wrote:
> > On 2014/04/21 23:42:53, Ami Fischman wrote:
> > > should go above the weakptrfactory above (see the NOTE)
> > 
> > Done.
> 
> You misunderstood my comment; weak_factory_ should be the last member of the
> class.

Sure. I still like the ThreadChecker in this class though.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:11: // there is no
reference to it.
On 2014/04/24 21:04:31, Ami Fischman wrote:
> This sentence makes me wonder what sort of "reference" you are thinking of
here.
> 
> I think it would be clearer to say that VCI is deleted via a PostTask to the
IO
> thread and that immediately thereafter the pointer is dropped in the render
> thread so no further posting can happen.
> This is only safe if nothing else posts; e.g. VCI mustn't PostTask to itself
> (seems like that should be doco'd in VCI).

Done.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:40:
ChildProcess::current()->io_message_loop_proxy()->PostTask(
You said:

> > > ChildProcess::current()->io_message_loop_proxy()
> > > is
> > > MessageLoopProxy::current()

MessageLoopProxy::current() returns the render thread. VideoCaptureImplManager
is destroyed on the render thread.

The reason I use ChildProcess here is because of unit testing. It creates an
instance of ChildProcess. It is much simpler to instantiate than a RenderProcess
or a RenderThreadImpl object.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/video_destination_handler.cc:120: // Not all sinks
care about the TimeTicks so it's not a problem now.
On 2014/04/24 21:04:31, Ami Fischman wrote:
> ditto comment elsewhere, why wouldn't you use |timestamp| from l.98?

The parameter is a TimeTicks. I would need to save a start time to compute the
correct TimeTicks here.

The reason for using TimeTicks: it is a Chrome wide clock that allows
synchronization of streams. Our camera implementations already provide this but
we drop it in VideoCaptureImpl. I'm adding this back to the interface.

Non-camera implementation of sources can just give TimeTicks::Now().

https://codereview.chromium.org/242013002/diff/140001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:25: // This is an render
thread only object. All methods must be accessed on
On 2014/04/24 21:04:31, Ami Fischman wrote:
> s/an/a/

Done.

https://codereview.chromium.org/242013002/diff/140001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:26: // the render thread
including cosntructor and destructor.
On 2014/04/24 21:04:31, Ami Fischman wrote:
> optional: could DCHECK this in the public methods.

Done.

[Ami GONE FROM CHROMIUM, 2014-04-24 23:05:17]

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/video_destination_handler.cc:120: // Not all sinks
care about the TimeTicks so it's not a problem now.
On 2014/04/24 22:50:34, Alpha wrote:
> On 2014/04/24 21:04:31, Ami Fischman wrote:
> > ditto comment elsewhere, why wouldn't you use |timestamp| from l.98?
> 
> The parameter is a TimeTicks. I would need to save a start time to compute the
> correct TimeTicks here.
> 
> The reason for using TimeTicks: it is a Chrome wide clock that allows
> synchronization of streams. Our camera implementations already provide this
but
> we drop it in VideoCaptureImpl. I'm adding this back to the interface.
> 
> Non-camera implementation of sources can just give TimeTicks::Now().

I'm unclear on why synchronization can't be done using media::VideoFrame's
TimeDeltas (and a per-stream start TimeTicks, which would have to be maintained
by whoever is doing the synchronizing).

https://codereview.chromium.org/242013002/diff/160001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/242013002/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:48: if (manager)
On 2014/04/24 22:50:34, Alpha wrote:
> On 2014/04/24 21:04:31, Ami Fischman wrote:
> > how can this fail?
> > (here and elsewhere below)
> 
> Ownership:
> 
> RenderThreadImpl owns Blink owns WebMediaStreamSource owns
> MediaStreamVideoCapturerSource owns VideoCaptureDelegate.
> 
> RenderThreadImpl also owns VideoCaptureImplManager.
> 
> In RenderThreadImpl::Shutdown(), VideoCaptureImplManager is destroyed before
> Blink is destroyed. So VideoCaptureDelegate can be alive while manager is
dead.

If that is possible then can StopCapture() fail to get called / have an effect,
leaving the camera in a bad state?

[Alpha Left Google, 2014-04-24 23:33:52]

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/242013002/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/video_destination_handler.cc:120: // Not all sinks
care about the TimeTicks so it's not a problem now.
On 2014/04/24 23:05:18, Ami Fischman wrote:
> On 2014/04/24 22:50:34, Alpha wrote:
> > On 2014/04/24 21:04:31, Ami Fischman wrote:
> > > ditto comment elsewhere, why wouldn't you use |timestamp| from l.98?
> > 
> > The parameter is a TimeTicks. I would need to save a start time to compute
the
> > correct TimeTicks here.
> > 
> > The reason for using TimeTicks: it is a Chrome wide clock that allows
> > synchronization of streams. Our camera implementations already provide this
> but
> > we drop it in VideoCaptureImpl. I'm adding this back to the interface.
> > 
> > Non-camera implementation of sources can just give TimeTicks::Now().
> 
> I'm unclear on why synchronization can't be done using media::VideoFrame's
> TimeDeltas (and a per-stream start TimeTicks, which would have to be
maintained
> by whoever is doing the synchronizing).

When a frame is captured in the browser. It is tagged with a TimeTicks (Moment
A). Current code in tree trims this to a TimeDelta and set it to VideoFrame in
VCI. Downstream code currently record a stream start TimeTicks (Moment B) and
add the TimeDelta back to obtain a time for synchronization. 

There is a latency between moment A and B. It would be better to just pass along
the TimeTicks untouched.

https://codereview.chromium.org/242013002/diff/160001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/242013002/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:48: if (manager)
> If that is possible then can StopCapture() fail to get called / have an
effect,
> leaving the camera in a bad state?

In the destructor of VCIM if there are clients it will call VCI::DeInit() and
destroy VCI on IO thread. This make sure all video capture devices are stopped.
The reference to VCIM is weak so clients can still call the stop callback.

[Ami GONE FROM CHROMIUM, 2014-04-25 00:44:08]

LGTM % the ticks-vs-delta issue.


On Thu, Apr 24, 2014 at 4:33 PM, <hclam@chromium.org> wrote:

>
> https://codereview.chromium.org/242013002/diff/140001/
> content/renderer/media/webrtc/video_destination_handler.cc
> File content/renderer/media/webrtc/video_destination_handler.cc (right):
>
> https://codereview.chromium.org/242013002/diff/140001/
> content/renderer/media/webrtc/video_destination_handler.cc#newcode120
> content/renderer/media/webrtc/video_destination_handler.cc:120: // Not
> all sinks care about the TimeTicks so it's not a problem now.
> On 2014/04/24 23:05:18, Ami Fischman wrote:
>
>> On 2014/04/24 22:50:34, Alpha wrote:
>> > On 2014/04/24 21:04:31, Ami Fischman wrote:
>> > > ditto comment elsewhere, why wouldn't you use |timestamp| from
>>
> l.98?
>
>> >
>> > The parameter is a TimeTicks. I would need to save a start time to
>>
> compute the
>
>> > correct TimeTicks here.
>> >
>> > The reason for using TimeTicks: it is a Chrome wide clock that
>>
> allows
>
>> > synchronization of streams. Our camera implementations already
>>
> provide this
>
>> but
>> > we drop it in VideoCaptureImpl. I'm adding this back to the
>>
> interface.
>
>> >
>> > Non-camera implementation of sources can just give TimeTicks::Now().
>>
>
>  I'm unclear on why synchronization can't be done using
>>
> media::VideoFrame's
>
>> TimeDeltas (and a per-stream start TimeTicks, which would have to be
>>
> maintained
>
>> by whoever is doing the synchronizing).
>>
>
> When a frame is captured in the browser. It is tagged with a TimeTicks
> (Moment A). Current code in tree trims this to a TimeDelta and set it to
> VideoFrame in VCI. Downstream code currently record a stream start
> TimeTicks (Moment B) and add the TimeDelta back to obtain a time for
> synchronization.
>
> There is a latency between moment A and B. It would be better to just
> pass along the TimeTicks untouched.


What I'm concerned about is that we seem to be synthesizing TimeDelta's
from Now()'s that will be in conflict with the associated TimeTicks.  Given
the reality of the storage of both types, does it not work to stamp the
frames with a TimeDelta equal to the TimeTicks (minus zero for type
conversion from delta to ticks, and TimeTicks()+delta for conversion the
other way around)?


> https://codereview.chromium.org/242013002/diff/160001/
> content/renderer/media/media_stream_video_capturer_source.cc
> File content/renderer/media/media_stream_video_capturer_source.cc
> (right):
>
> https://codereview.chromium.org/242013002/diff/160001/
> content/renderer/media/media_stream_video_capturer_source.cc#newcode48
> content/renderer/media/media_stream_video_capturer_source.cc:48: if
> (manager)
>
>> If that is possible then can StopCapture() fail to get called / have
>>
> an effect,
>
>> leaving the camera in a bad state?
>>
>
> In the destructor of VCIM if there are clients it will call
> VCI::DeInit() and destroy VCI on IO thread. This make sure all video
> capture devices are stopped. The reference to VCIM is weak so clients
> can still call the stop callback.
>


Ok.

>
> https://codereview.chromium.org/242013002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-04-25 19:43:11]

Removed TimeTicks as discussed. Please take another look.

[Ami GONE FROM CHROMIUM, 2014-04-25 19:56:16]

https://codereview.chromium.org/242013002/diff/200001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/242013002/diff/200001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:373: const base::TimeTicks&
timestamp) {
Why is this param still here?

[Alpha Left Google, 2014-04-25 20:15:40]

Had a problem with git. The last upload patch is correct now.

[Ami GONE FROM CHROMIUM, 2014-04-25 20:21:34]

PS#9->#13 LGTM

Please file a crbug detailing the delta/ticks discrepancy and point to it from a
TODO where you'd like the ticks to enter the flow for the first time in this CL.

[Alpha Left Google, 2014-04-25 20:23:38]

The CQ bit was checked by hclam@chromium.org

[Alpha Left Google, 2014-04-25 20:31:15]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-25 21:39:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/260001

[commit-bot: I haz the power, 2014-04-25 22:33:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:33:47]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Alpha Left Google, 2014-04-25 22:53:17]

+bbudge for OWNERS approval for content/renderer/pepper.

[bbudge, 2014-04-25 23:18:58]

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:32:
DCHECK(thread_checker_.CalledOnValidThread());
Isn't thread_checker_ constructed on this thread too? So this could never fail.
From the comments, I think you should get the render thread and make sure you're
on it.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:27: // the render thread
including cosntructor and destructor.
sp constructor

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:47: const
media::VideoCaptureFormat& format);
Have you run git cl format? I think we're starting to do that in Pepper
directories.

(or clang-format if you only want to format a few files)
clang-format -i -style=Chromium <file-name>

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_video_capture_host.h (right):

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:44: // Called when video
capturing is initialized. We can start
s/capturing/capture
here and below

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:52: // frame delivered.
s/frame/frames

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:58: // Called when video
capturing cannot be start because of an error.
s/start/started

Is this comment true? It seems like it can be called whenever there is an error,
not just when starting.

[Alpha Left Google, 2014-04-25 23:33:22]

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:32:
DCHECK(thread_checker_.CalledOnValidThread());
On 2014/04/25 23:18:58, bbudge wrote:
> Isn't thread_checker_ constructed on this thread too? So this could never
fail.
> From the comments, I think you should get the render thread and make sure
you're
> on it.

It's probably not needed here. I think a ThreadChecker is suffice. It ensures
this is a single thread object.

Removed.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:27: // the render thread
including cosntructor and destructor.
On 2014/04/25 23:18:58, bbudge wrote:
> sp constructor

Done.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:47: const
media::VideoCaptureFormat& format);
On 2014/04/25 23:18:58, bbudge wrote:
> Have you run git cl format? I think we're starting to do that in Pepper
> directories.
> 
> (or clang-format if you only want to format a few files)
> clang-format -i -style=Chromium <file-name>

Done.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_video_capture_host.h (right):

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:44: // Called when video
capturing is initialized. We can start
On 2014/04/25 23:18:58, bbudge wrote:
> s/capturing/capture
> here and below

Done.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:52: // frame delivered.
On 2014/04/25 23:18:58, bbudge wrote:
> s/frame/frames

Done.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:58: // Called when video
capturing cannot be start because of an error.
On 2014/04/25 23:18:58, bbudge wrote:
> s/start/started
> 
> Is this comment true? It seems like it can be called whenever there is an
error,
> not just when starting.

Realistically error only happens during start.

https://codereview.chromium.org/242013002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_video_capture_host.h:58: // Called when video
capturing cannot be start because of an error.
On 2014/04/25 23:18:58, bbudge wrote:
> s/start/started
> 
> Is this comment true? It seems like it can be called whenever there is an
error,
> not just when starting.

Done.

[bbudge, 2014-04-26 00:27:11]

content/renderer/pepper LGTM

https://codereview.chromium.org/242013002/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.h (right):

https://codereview.chromium.org/242013002/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.h:27: // same thread.
I don't really get this comment. I think it's assumed that a class is designed
to be used on a single thread unless the comments state otherwise.

Since another reviewer requested the comment, could you just say this?
// This object must only be used on the thread it's constructed on.

[Alpha Left Google, 2014-04-26 00:36:50]

On 2014/04/26 00:27:11, bbudge wrote:
> content/renderer/pepper LGTM
> 
>
https://codereview.chromium.org/242013002/diff/300001/content/renderer/pepper...
> File content/renderer/pepper/pepper_platform_video_capture.h (right):
> 
>
https://codereview.chromium.org/242013002/diff/300001/content/renderer/pepper...
> content/renderer/pepper/pepper_platform_video_capture.h:27: // same thread.
> I don't really get this comment. I think it's assumed that a class is designed
> to be used on a single thread unless the comments state otherwise.
> 
> Since another reviewer requested the comment, could you just say this?
> // This object must only be used on the thread it's constructed on.

Will do.

[Alpha Left Google, 2014-04-26 00:38:56]

> > Since another reviewer requested the comment, could you just say this?
> > // This object must only be used on the thread it's constructed on.
> 
> Will do.

Fixed the comments there.

[Alpha Left Google, 2014-04-26 00:39:06]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-26 00:40:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/320001

[commit-bot: I haz the power, 2014-04-26 08:33:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 08:33:09]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[Alpha Left Google, 2014-04-26 19:04:59]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-26 19:06:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/320001

[commit-bot: I haz the power, 2014-04-27 00:26:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-27 00:26:27]

Failed to apply patch for media/media.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/media.gyp
  Hunk #1 FAILED at 475.
  Hunk #2 succeeded at 1268 (offset 6 lines).
  1 out of 2 hunks FAILED -- saving rejects to file media/media.gyp.rej

Patch:       media/media.gyp
Index: media/media.gyp
diff --git a/media/media.gyp b/media/media.gyp
index
420e6fef1cc0c707aa061023976da31799c1973d..530014ed2a6615a310ab78e0f19d11ccdde41775
100644
--- a/media/media.gyp
+++ b/media/media.gyp
@@ -475,11 +475,8 @@
         'video/capture/mac/video_capture_device_mac.mm',
         'video/capture/mac/video_capture_device_qtkit_mac.h',
         'video/capture/mac/video_capture_device_qtkit_mac.mm',
-        'video/capture/video_capture.h',
         'video/capture/video_capture_device.cc',
         'video/capture/video_capture_device.h',
-        'video/capture/video_capture_proxy.cc',
-        'video/capture/video_capture_proxy.h',
         'video/capture/video_capture_types.cc',
         'video/capture/video_capture_types.h',
         'video/capture/win/capability_list_win.cc',
@@ -1262,8 +1259,6 @@
         'base/test_helpers.h',
         'filters/mock_gpu_video_accelerator_factories.cc',
         'filters/mock_gpu_video_accelerator_factories.h',
-        'video/capture/mock_video_capture_event_handler.cc',
-        'video/capture/mock_video_capture_event_handler.h',
         'video/mock_video_decode_accelerator.cc',
         'video/mock_video_decode_accelerator.h',
       ],

[Alpha Left Google, 2014-04-27 08:54:22]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-27 08:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/340001

[commit-bot: I haz the power, 2014-04-27 09:32:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-27 09:32:19]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Alpha Left Google, 2014-04-27 10:07:55]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-27 10:08:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/340001

[commit-bot: I haz the power, 2014-04-27 15:48:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-27 15:48:30]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Alpha Left Google, 2014-04-27 20:18:38]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-04-27 20:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/242013002/340001

[commit-bot: I haz the power, 2014-04-28 08:56:23]

Change committed as 266492

[tonyg, 2014-05-02 14:54:41]

Looks like this is causing flakiness on Mac due to:
[8498:13575:0501/170220:FATAL:video_capture_impl.cc(140)] Check failed: removed.
Removing a non-existent client.

See https://code.google.com/p/chromium/issues/detail?id=369129

Is this safe for me to revert?

[tonyg, 2014-05-05 23:16:32]

A revert of this CL has been created in
https://codereview.chromium.org/263323003/ by tonyg@chromium.org.

The reason for reverting is: "FATAL:video_capture_impl.cc(140)] Check failed:
removed. Removing a non-existent client."

BUG=369129.