enable video capture to support sharing across multiple renderer processes

enable video capture to support sharing across multiple renderer processes 
To facilitate code review, this patch has been split into http://codereview.chromium.org/8400084/ for renderer process part, and http://codereview.chromium.org/8417046/ for browser process part.

1. VideoCaptureController is owned by VideoCaptureManager.
2. VideoCaptureHost obtains an instance of controller from manager.
3. Each renderer process has only one VC to serve multiple clients, while it is a client of browser process VC.
4. The largest resolution is used for the device capturing. Clients should expect frame buffers with larger resolution that they requsted. All cropping/resampling is done by clients.
5. Frame buffers are shared between renderer processes and browser process. The buffer received by clients are shared buffers. Clients are recommended to copy pixel data into their local storage. A client can slow down capture frame rate if it returns buffers slowly.
6. This patch only supported default device. More change in media stream is needed to support user selected device. That will be a different patch.

[perkj_chrome, 2011-10-17 08:39:43]

Looks very good. But I think you must make sure that the first client is allowed
to reconfigure the camera resolution.
Ie , first local preview then setup a call using 720P.

Can you please also update the design document to reflect this change?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:50: if (state_
== media::VideoCapture::kStarted) {
What if you run webrtc? First you start a local preview and then you set up a
call that want to run 720P but the local preview was started in cif?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:71: return;
DCHECK ?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:89:
video_capture_manager_->Stop(params_.session_id, base::Closure());
I think you will need to make sure that the first client is allowed to
reconfigure the camera. Ie change resolution.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:92: bool
report_ready_to_delete;
can you explain how report_ready_to_delete is used?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:120: // Modified
on only IO thread.
nit: sentence

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:163:
base::Bind(&VideoCaptureHost::DoControllerAddedOnIOThreead,
isn't this already on the io thread?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.h:146: EntryState
entry_state_;
why two maps? Combine?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:492:
VideoCaptureController* ctrller,
nit: name?

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
media/video/capture/linux/video_capture_device_linux.cc:195:
observer_->OnDeviceState(true);
What is the purpose of this? Is it possible to avoid. You know you will have to
change this on all platforms.

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/video_ca...
File media/video/capture/video_capture_device.h (right):

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/video_ca...
media/video/capture/video_capture_device.h:72: virtual void OnDeviceState(bool
in_use) = 0;
Why does the device need to know? Should be handled by VideoCaptureManager.

[scherkus (not reviewing), 2011-10-19 18:02:21]

few drive by nits

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:364: :
controller_id(id),
indent by 2 more spaces

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:77: struct
ControllerClient {
nit: you can forward declare this and move the impl into the .cc

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:13:
struct CONTENT_EXPORT VideoCaptureControllerID {
I forget the history of this, but why is this not a typedef of an int?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.h:144:
media::VideoCapture::State>EntryState;
nit: space between > and EntryState

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:43:
STLDeleteContainerPairSecondPointers(controllers_.begin(),
nit: STLDeleteValues() works here

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.h:91: struct
Controller {
nit: this can be forward declared and the impl moved to .cc

[mflodman_chromium_OOO, 2011-10-19 18:18:23]

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:89:
video_capture_manager_->Stop(params_.session_id, base::Closure());
On 2011/10/17 08:39:43, perkj wrote:
> I think you will need to make sure that the first client is allowed to
> reconfigure the camera. Ie change resolution.

Or  allowing changing settings as long as there is only one device user?

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:163:
base::Bind(&VideoCaptureHost::DoControllerAddedOnIOThreead,
On 2011/10/17 08:39:43, perkj wrote:
> isn't this already on the io thread?
This is called from the video manager thread.

Wei,
Q: VideoCaptureManager is always posting to IO thread internally before calling
MediaStreamManager. Do you think we should do the same for these callbacks or do
you want to keep it like this?

[wjia(left Chromium), 2011-10-21 00:56:13]

Thanks for the review! The new patch has resolution handling changed to take max
of all clients, and clients are supposed to handle resampling/cropping when
received buffers have larger resolution than they requested. The reason is that
almost every client (Pepper, preview, webrtc) is copying pixel data into local
storage. Therefore, it's more efficient to do resampling/cropping when clients
copy pixel data.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:50: if (state_
== media::VideoCapture::kStarted) {
On 2011/10/17 08:39:43, perkj wrote:
> What if you run webrtc? First you start a local preview and then you set up a
> call that want to run 720P but the local preview was started in cif?
The resolution change in webrtc is handled in render process. In the example you
mentioned, render process will stop capture, wait for stopped signal, then
restart capture. That should still work.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:71: return;
On 2011/10/17 08:39:43, perkj wrote:
> DCHECK ? 

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:89:
video_capture_manager_->Stop(params_.session_id, base::Closure());
On 2011/10/17 08:39:43, perkj wrote:
> I think you will need to make sure that the first client is allowed to
> reconfigure the camera. Ie change resolution.
In the latest design, the final capture resolution will be max(all requested
widths) and max(all request heights).

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:89:
video_capture_manager_->Stop(params_.session_id, base::Closure());
On 2011/10/19 18:18:23, mflodman wrote:
> On 2011/10/17 08:39:43, perkj wrote:
> > I think you will need to make sure that the first client is allowed to
> > reconfigure the camera. Ie change resolution.
> 
> Or  allowing changing settings as long as there is only one device user?
There are two levels of clients: VideoCaptureImpl in each render process serves
several clients, and VideoCaptureController serves VideoCaptureImpl in several
render processes. Each render process has only one request to
VideoCaptureController. So the setting change in render process is handled
inside render process. If they ask for resolution change, it will be a
stop-restart from browser process point of view.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:364: :
controller_id(id),
On 2011/10/19 18:02:21, scherkus wrote:
> indent by 2 more spaces

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:77: struct
ControllerClient {
On 2011/10/19 18:02:21, scherkus wrote:
> nit: you can forward declare this and move the impl into the .cc

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:92: bool
report_ready_to_delete;
On 2011/10/17 08:39:43, perkj wrote:
> can you explain how report_ready_to_delete is used?
When controller client calls StopCapture, but it has not returned all buffers,
report_ready_to_delete will be flagged. When all buffers are returned by that
client, OnReadyToDelete will be called to tell client that it's no longer
needed.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:120: // Modified
on only IO thread.
On 2011/10/17 08:39:43, perkj wrote:
> nit: sentence

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:13:
struct CONTENT_EXPORT VideoCaptureControllerID {
On 2011/10/19 18:02:21, scherkus wrote:
> I forget the history of this, but why is this not a typedef of an int?
Originally, there was a routing_id here. Since message is changed from routed to
control, the routing_id was gone. But I still keep it as struct in case we need
add more info here. If you are strongly against it, I can use typedef.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:163:
base::Bind(&VideoCaptureHost::DoControllerAddedOnIOThreead,
On 2011/10/17 08:39:43, perkj wrote:
> isn't this already on the io thread?
It's called from manager's device thread.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:163:
base::Bind(&VideoCaptureHost::DoControllerAddedOnIOThreead,
On 2011/10/19 18:18:23, mflodman wrote:
> On 2011/10/17 08:39:43, perkj wrote:
> > isn't this already on the io thread?
> This is called from the video manager thread.
> 
> Wei,
> Q: VideoCaptureManager is always posting to IO thread internally before
calling
> MediaStreamManager. Do you think we should do the same for these callbacks or
do
> you want to keep it like this?

I am fine with either way. VCManager will have lots of functions if it takes
care of thread switching. It's 1:3.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.h:144:
media::VideoCapture::State>EntryState;
On 2011/10/19 18:02:21, scherkus wrote:
> nit: space between > and EntryState

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.h:146: EntryState
entry_state_;
On 2011/10/17 08:39:43, perkj wrote:
> why two maps? Combine?
EntryState indicates that host has got StartCapture call. Since controller is
obtained via thread switching (from IO thread to manger's device thread, and
back to IO thread), host need to know the state based on which different action
is taken. An example is when StopCapture comes in before host obtains
controller.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:43:
STLDeleteContainerPairSecondPointers(controllers_.begin(),
On 2011/10/19 18:02:21, scherkus wrote:
> nit: STLDeleteValues() works here

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:492:
VideoCaptureController* ctrller,
On 2011/10/17 08:39:43, perkj wrote:
> nit: name?

Done.

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.h (right):

http://codereview.chromium.org/8304017/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.h:91: struct
Controller {
On 2011/10/19 18:02:21, scherkus wrote:
> nit: this can be forward declared and the impl moved to .cc

Done.

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
media/video/capture/linux/video_capture_device_linux.cc:195:
observer_->OnDeviceState(true);
On 2011/10/17 08:39:43, perkj wrote:
> What is the purpose of this? Is it possible to avoid. You know you will have
to
> change this on all platforms.
I am trying to handle the case where observer_ (i.e., VCController) has been
deleted, but device still calls its function. With this signal, manager knows
when to delete controller safely.

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/video_ca...
File media/video/capture/video_capture_device.h (right):

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/video_ca...
media/video/capture/video_capture_device.h:72: virtual void OnDeviceState(bool
in_use) = 0;
On 2011/10/17 08:39:43, perkj wrote:
> Why does the device need to know? Should be handled by VideoCaptureManager.
See explanation in video_capture_device_linux.cc

[wjia(left Chromium), 2011-10-21 00:57:49]

+ vtl@

[perkj_chrome, 2011-10-21 14:54:00]

Here are a some comments. Great with the support of many hosts to one
controller. 
Some cleaning up to do. I also would appreciate a lot more comments in the
controller since the logic looks very complicated.

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
media/video/capture/linux/video_capture_device_linux.cc:195:
observer_->OnDeviceState(true);
We had previously solved this in another way.
Also VideoCapture::Stop is syncronouse. So after calling Stop the device will
not call the VC controller.


On 2011/10/21 00:56:13, wjia wrote:
> On 2011/10/17 08:39:43, perkj wrote:
> > What is the purpose of this? Is it possible to avoid. You know you will have
> to
> > change this on all platforms.
> I am trying to handle the case where observer_ (i.e., VCController) has been
> deleted, but device still calls its function. With this signal, manager knows
> when to delete controller safely.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:7: #include
<stdio.h>
needed?

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:15: // The
number of DIBs VideoCaptureController allocate.
per host?

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:61: void
VideoCaptureController::StartCapture(
Can you please document the different cases this function handles and how?

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:76: return;
DCHECK? Is this a programming error?

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:86: if
(params.width > frame_info_.width ||
You might want to compare with what was actually requested previously (current
params). If the camera can only do vga there is no need to try several times to
open up in 720P.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:112: void
VideoCaptureController::StopCapture(
Can you please document what this function is doing and what different cases it
handles?

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:165: void
VideoCaptureController::ReturnBuffer(
Please document.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:312: int
buffer_id, base::Time timestamp) {
You say that this is of type int here but in OnBufferReady you say it is of type
 base::SharedMemoryHandle.
Please use the same type name everywhere. I prefer  base::SharedMemoryHandle if
that is actually what it is.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:414: for (cit =
controller_clients_.begin();
Use SendFrameInfoAndBuffers - avoid duplicate code.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:437: const
size_t needed_size = (frame_info_.width * frame_info_.height * 3) / 2;
not used

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.h:14: // It serves
multiple VideoCaptureControllerEventHandlers.
What is a VideoCaptureControllerEventHandler and how does it relate to the
classes mentioned above?

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:54:
BrowserThread::PostTask(
You are already on IO thread now.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:64:
BrowserThread::PostTask(
This is already on IO thread now.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:74:
BrowserThread::PostTask(
This is also on IO thread now.

http://codereview.chromium.org/8304017/diff/18012/media/video/capture/linux/v...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/18012/media/video/capture/linux/v...
media/video/capture/linux/video_capture_device_linux.cc:195:
observer_->OnDeviceState(true);
Still think you should remove this.

[wjia(left Chromium), 2011-10-21 23:54:16]

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/8001/media/video/capture/linux/vi...
media/video/capture/linux/video_capture_device_linux.cc:195:
observer_->OnDeviceState(true);
Could you clarify it's VideoCapture::StopCapture or VideoCaptureDevice::Stop,
since VideoCapture::Stop doesn't exist. Neither one is synchronous though. See
media/video/capture/linux/video_capture_device_linux.cc and
content/renderer/media/video_capture_impl.cc.


On 2011/10/21 14:54:00, perkj wrote:
> We had previously solved this in another way.
> Also VideoCapture::Stop is syncronouse. So after calling Stop the device will
> not call the VC controller.
> 
> 
> On 2011/10/21 00:56:13, wjia wrote:
> > On 2011/10/17 08:39:43, perkj wrote:
> > > What is the purpose of this? Is it possible to avoid. You know you will
have
> > to
> > > change this on all platforms.
> > I am trying to handle the case where observer_ (i.e., VCController) has been
> > deleted, but device still calls its function. With this signal, manager
knows
> > when to delete controller safely.
>

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:7: #include
<stdio.h>
On 2011/10/21 14:54:00, perkj wrote:
> needed?

Oops, this is leftover for debugging printf. Removed.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:15: // The
number of DIBs VideoCaptureController allocate.
On 2011/10/21 14:54:00, perkj wrote:
> per host?

No. The controller is one per device and allocates only this number of shared
buffers for the browser process. All hosts (i.e., renderer processes) share the
same buffers.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:61: void
VideoCaptureController::StartCapture(
On 2011/10/21 14:54:00, perkj wrote:
> Can you please document the different cases this function handles and how?

Comments have been added in each condition.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:76: return;
On 2011/10/21 14:54:00, perkj wrote:
> DCHECK? Is this a programming error?

I kind of allow this behaviors. Strictly speaking, there should be one
StartCapture per client.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:86: if
(params.width > frame_info_.width ||
On 2011/10/21 14:54:00, perkj wrote:
> You might want to compare with what was actually requested previously (current
> params). If the camera can only do vga there is no need to try several times
to
> open up in 720P.

That would be a todo to add code to query the max capability of device.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:112: void
VideoCaptureController::StopCapture(
On 2011/10/21 14:54:00, perkj wrote:
> Can you please document what this function is doing and what different cases
it
> handles?

Done.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:165: void
VideoCaptureController::ReturnBuffer(
On 2011/10/21 14:54:00, perkj wrote:
> Please document.

Done.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:312: int
buffer_id, base::Time timestamp) {
On 2011/10/21 14:54:00, perkj wrote:
> You say that this is of type int here but in OnBufferReady you say it is of
type
>  base::SharedMemoryHandle.
> Please use the same type name everywhere. I prefer  base::SharedMemoryHandle
if
> that is actually what it is. 
I guess you are confused with OnBufferReady and OnBufferCreated. OnBufferReady
is using "int", while OnBufferCreated is using both "int" and
"base::SharedMemoryHandle" since it needs to pass the pair to renderer process.
We are using buffer_id after that.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:414: for (cit =
controller_clients_.begin();
On 2011/10/21 14:54:00, perkj wrote:
> Use SendFrameInfoAndBuffers - avoid duplicate code.

Done.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:437: const
size_t needed_size = (frame_info_.width * frame_info_.height * 3) / 2;
On 2011/10/21 14:54:00, perkj wrote:
> not used

It's used in OnBufferCreated.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.h:14: // It serves
multiple VideoCaptureControllerEventHandlers.
On 2011/10/21 14:54:00, perkj wrote:
> What is a VideoCaptureControllerEventHandler and how does it relate to the
> classes mentioned above?
VideoCaptureController sends events to VideoCaptureControllerEventHandler when
it finishes some operation and need to notify its client (i.e., event handler).
VideoCaptureHost is derived from VideoCaptureControllerEventHandler. This
sentence is added just to state that controller has multiple clients now.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:54:
BrowserThread::PostTask(
On 2011/10/21 14:54:00, perkj wrote:
> You are already on IO thread now.

This PostTask can keep function call (from controller) executed in same sequence
as they are called.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:64:
BrowserThread::PostTask(
On 2011/10/21 14:54:00, perkj wrote:
> This is already on IO thread now.
same as above.

http://codereview.chromium.org/8304017/diff/18012/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:74:
BrowserThread::PostTask(
On 2011/10/21 14:54:00, perkj wrote:
> This is also on IO thread now.

same as above.

http://codereview.chromium.org/8304017/diff/18012/media/video/capture/linux/v...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/18012/media/video/capture/linux/v...
media/video/capture/linux/video_capture_device_linux.cc:195:
observer_->OnDeviceState(true);
On 2011/10/21 14:54:00, perkj wrote:
> Still think you should remove this.
We can discuss it after you confirm whether VideoCaptureDevice::Stop is sync or
async mode.

[wjia(left Chromium), 2011-10-25 17:07:55]

ping.

[mflodman_chromium_OOO, 2011-10-26 12:34:44]

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:61: const
VideoCaptureControllerID& id,
Q: What do you think about moving the resolution and start/stop logic to
VideoCaptureManager and let this class focus on the buffers? I haven't digged
into that, but maybe the logic can be simplified that way. Start/Stop calls can
be relayed to manager and this class allocates memory based on callbacks from
OnFrameInfo? VideoCaptureManager will need to know about all sessionId:s using
the same device and that can be extended to handle resolution and call device
Start/Stop when needed,

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:96:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
Q: When is this restarted? I didn't find a call to VideoCaptureManager::Start
after this to restart the device.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:182: if
(controller_clients_.size() == 0) {
Can there be pending request we should check for, like a new StartCapture has
been called with a larger resolution?

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:183:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
Can you explain how the event is used here? Previously was
VideoCaptureManager::Stop called and the controller/host waited until the event
triggered, which happens when the device is stopped and no more frames will be
delivered.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:411:
pending_clients_.erase(cit++);
Doesn't this invalidate the list iterator and shouldn't we use:
cit = pending_clients_.erase(cit);

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:413: if
(current_params_.width > info.width ||
I'd like a comment to explain when this is happening.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:515:
pending_clients_.erase(cit++);
Same comment about iterator as above.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.h:103:
ControllerClient* client, int needed_size);
Style: indentation.

http://codereview.chromium.org/8304017/diff/24001/media/video/capture/video_c...
File media/video/capture/video_capture_device.h (right):

http://codereview.chromium.org/8304017/diff/24001/media/video/capture/video_c...
media/video/capture/video_capture_device.h:72: virtual void OnDeviceState(bool
in_use) = 0;
Is this needed? Previously we used the event in stop to tell the device had been
stopped.

[wjia(left Chromium), 2011-10-26 21:17:33]

Thanks, Magnus!

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:61: const
VideoCaptureControllerID& id,
On 2011/10/26 12:34:45, mflodman wrote:
> Q: What do you think about moving the resolution and start/stop logic to
> VideoCaptureManager and let this class focus on the buffers? I haven't digged
> into that, but maybe the logic can be simplified that way. Start/Stop calls
can
> be relayed to manager and this class allocates memory based on callbacks from
> OnFrameInfo? VideoCaptureManager will need to know about all sessionId:s using
> the same device and that can be extended to handle resolution and call device
> Start/Stop when needed,
If we put start/stop logic in manager, we have to move almost everything from
controller to manager, including resolution, device status, buffer status, etc.
That will bloat manager. The current split between manager and controller is
good so that manager handles device open/close/stop/start and controllers, while
controller handles its client (hosts).

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:96:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
On 2011/10/26 12:34:45, mflodman wrote:
> Q: When is this restarted? I didn't find a call to VideoCaptureManager::Start
> after this to restart the device.
The restart happens in PostStopping() call. That's when all clients have
returned all buffers and device has been fully stopped.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:182: if
(controller_clients_.size() == 0) {
On 2011/10/26 12:34:45, mflodman wrote:
> Can there be pending request we should check for, like a new StartCapture has
> been called with a larger resolution?
StartCapture and StopCapture run on IO thread. There could be pending
StartCapture from another host, but controller doesn't know about it when this
host's StopCapture reaches this point.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:183:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
On 2011/10/26 12:34:45, mflodman wrote:
> Can you explain how the event is used here? Previously was
> VideoCaptureManager::Stop called and the controller/host waited until the
event
> triggered, which happens when the device is stopped and no more frames will be
> delivered.
Actually, this is a NULL callback. I looked at video_capture_device code.
VideoCaptureDevice::Stop is an asynchronous call (e.g., on linux). When
VideoCaptureManager finishes video_capture_device->Stop(), the device is not
really stopped. So the callback from VideoCaptureManager doesn't tell controller
that it's not in use. The device might call controller during stopping since
controller is the observer in device.

That's also why I added an event function OnDeviceState() in
media::VideoCaptureDevice::EventHandler. That function can tell controller that
the device is really stopped or not.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:411:
pending_clients_.erase(cit++);
On 2011/10/26 12:34:45, mflodman wrote:
> Doesn't this invalidate the list iterator and shouldn't we use:
> cit = pending_clients_.erase(cit);
This line is equivalent to
{
ControllerClients::iterator tmp = cit;
++cit;
pending_clients_.erase(tmp);
}
Your suggestion also works.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:413: if
(current_params_.width > info.width ||
On 2011/10/26 12:34:45, mflodman wrote:
> I'd like a comment to explain when this is happening.

Done.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:515:
pending_clients_.erase(cit++);
On 2011/10/26 12:34:45, mflodman wrote:
> Same comment about iterator as above.
Same explanation as above. :-)

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.h:103:
ControllerClient* client, int needed_size);
On 2011/10/26 12:34:45, mflodman wrote:
> Style: indentation.

Done.

http://codereview.chromium.org/8304017/diff/24001/media/video/capture/video_c...
File media/video/capture/video_capture_device.h (right):

http://codereview.chromium.org/8304017/diff/24001/media/video/capture/video_c...
media/video/capture/video_capture_device.h:72: virtual void OnDeviceState(bool
in_use) = 0;
On 2011/10/26 12:34:45, mflodman wrote:
> Is this needed? Previously we used the event in stop to tell the device had
been
> stopped.
in VideoCaptureDeviceLinux, its Stop() is asynchronous. The callback in
VideoCaptureManager doesn't really tell the status of device. During
VideoCaptureDeviceLinux::OnStop(), it might call observer (i.e., controller).

[perkj_chrome, 2011-10-27 07:27:32]

Added comments on why manager->start and manager->stop is asyncronouse.

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/24001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:183:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
On 2011/10/26 21:17:33, wjia wrote:
> On 2011/10/26 12:34:45, mflodman wrote:
> > Can you explain how the event is used here? Previously was
> > VideoCaptureManager::Stop called and the controller/host waited until the
> event
> > triggered, which happens when the device is stopped and no more frames will
be
> > delivered.
> Actually, this is a NULL callback. I looked at video_capture_device code.
> VideoCaptureDevice::Stop is an asynchronous call (e.g., on linux). When
> VideoCaptureManager finishes video_capture_device->Stop(), the device is not
> really stopped. So the callback from VideoCaptureManager doesn't tell
controller
> that it's not in use. The device might call controller during stopping since
> controller is the observer in device.
> 
> That's also why I added an event function OnDeviceState() in
> media::VideoCaptureDevice::EventHandler. That function can tell controller
that
> the device is really stopped or not.

Please check again. 
All platforms have synchronouse start /stop. On linux the device thread is
started and stopped.

http://codereview.chromium.org/8304017/diff/32001/media/video/capture/linux/v...
File media/video/capture/linux/video_capture_device_linux.cc (right):

http://codereview.chromium.org/8304017/diff/32001/media/video/capture/linux/v...
media/video/capture/linux/video_capture_device_linux.cc:143:
v4l2_thread_.Start();
This is what make manager->Start synchronouse.

http://codereview.chromium.org/8304017/diff/32001/media/video/capture/linux/v...
media/video/capture/linux/video_capture_device_linux.cc:176:
v4l2_thread_.Stop();
manager->Stop syncronouse.

[perkj_chrome, 2011-10-27 14:30:30]

more comments

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:91: // Need
restart catpuring.
nit: catpuring- capturing.

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:92: //
TODO(wjia): Query the max capability of device. If request is larger
Don't query the device. (it does not work on Mac and work poor on Linux) I meant
that you should remember what max have been requested before. You never decrease
the resolution so you can use that here instead of frame_info_ which seems to be
what the camera actually produces at the moment?

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:96:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
Use base::Closure instead of your callback.

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:415: if
(current_params_.width > info.width ||
You should not trust that the camera give you what you requested.
You are not guaranteed to get he frame size you request. It might be smaller, it
might bee larger. It is up to the camera drivers.

[wjia(left Chromium), 2011-10-28 01:27:39]

Thanks, Per!

The patch set 9 removed the state report function in video_capture_device.
Controller now uses current requested parameters (instead of frame_info coming
from device) to determine whether the new client deserves a restart or not.

Reviewers,
patch set 10 is just a rebase without any change.

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:91: // Need
restart catpuring.
On 2011/10/27 14:30:30, perkj wrote:
> nit: catpuring- capturing.

Done.

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:92: //
TODO(wjia): Query the max capability of device. If request is larger
On 2011/10/27 14:30:30, perkj wrote:
> Don't query the device. (it does not work on Mac and work poor on Linux) I
meant
> that you should remember what max have been requested before. You never
decrease
> the resolution so you can use that here instead of frame_info_ which seems to
be
> what the camera actually produces at the moment?
Done. For now, we don't decrease resolution. In the future, we might want to do
that for the sake of saving on computing and memeory.

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:96:
video_capture_manager_->Stop(current_params_.session_id, base::Closure());
On 2011/10/27 14:30:30, perkj wrote:
> Use base::Closure instead of your callback.

Done. I assume you mean "Use your callback, instead of base::Closure".

http://codereview.chromium.org/8304017/diff/32001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:415: if
(current_params_.width > info.width ||
On 2011/10/27 14:30:30, perkj wrote:
> You should not trust that the camera give you what you requested.
> You are not guaranteed to get he frame size you request. It might be smaller,
it
> might bee larger. It is up to the camera drivers.

Done.

[perkj_chrome, 2011-10-28 08:37:19]

Looks good. 
It would be great if you can make sure VideoCaptureManager always post events on
the IO thread.

Also some things int the video capture module I didn't understand the purpose
of.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:200: // after
this return, ready to delet this client.
nit delet -delete

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:399:
frame_info_= info;
nit: do you really need both frame_info and frame_info available?

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:437: void
VideoCaptureController::PostStopping() {
Add a comment about what this function do. And maybe rename since it does not
stop but rather restart the camera?

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:506:
BrowserThread::PostTask(BrowserThread::IO,
Make sure that this post in this class is moved to the capturemanager.
Ie,make sure that the capturemanager post this event on the io thread. Ie - all
input and output from the manager should be done on the io thread since that is
the only thread that it's clients know of.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:164:
BrowserThread::PostTask(
Make sure that this post in this class is moved to the capturemanager.
Ie,make sure that the capturemanager post this event on the io thread. Ie - all
input and output from the manager should be done on the io thread since that is
the only thread that it's clients now of.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:11: #include
"content/browser/renderer_host/media/video_capture_controller_event_handler.h"
can and should this be made 80 by breaking the line?

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:257:
stopped_cb.Run();
Do this on IO thread instead please.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:414:
added_cb.Run(ctrller);
Do added_cb.Run(ctrller); on IO thread instead please.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:67:
virtual void OnDeviceState(bool in_use) {}
remove

http://codereview.chromium.org/8304017/diff/43002/content/renderer/media/vide...
File content/renderer/media/video_capture_module_impl.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/renderer/media/vide...
content/renderer/media/video_capture_module_impl.cc:251: if (!got_first_frame_)
{
Why is this necessary? Why compensate the time? ViE don't care about the
timestamp of the first frame.

http://codereview.chromium.org/8304017/diff/43002/content/renderer/media/vide...
content/renderer/media/video_capture_module_impl.cc:260: if (buf->width ==
static_cast<int>(width_) &&
You should never have to care about the frame size. ViE will have to scale
before doing encoding. Just make sure the frameInfo is correct in each frame.

[wjia(left Chromium), 2011-10-29 02:34:39]

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:200: // after
this return, ready to delet this client.
On 2011/10/28 08:37:19, perkj wrote:
> nit delet -delete

Done.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:399:
frame_info_= info;
On 2011/10/28 08:37:19, perkj wrote:
> nit: do you really need both frame_info and frame_info available? 
Using frame_info_available is easy to read. Another way is to tap into
frame_info and use its members as indicator of availability.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:437: void
VideoCaptureController::PostStopping() {
On 2011/10/28 08:37:19, perkj wrote:
> Add a comment about what this function do. And maybe rename since it does not
> stop but rather restart the camera?
Comments added. This function indeed also puts controller in kStopped state when
there is no more client. So it is some post processing after kStopping state.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:506:
BrowserThread::PostTask(BrowserThread::IO,
On 2011/10/28 08:37:19, perkj wrote:
> Make sure that this post in this class is moved to the capturemanager.
> Ie,make sure that the capturemanager post this event on the io thread. Ie -
all
> input and output from the manager should be done on the io thread since that
is
> the only thread that it's clients know of.
Controller doesn't need to know what threads VCManager is operating on. It just
posts a task to IO thread whenever this function is called. This is more
flexible than requiring VCManager to call outgoing function on IO thread. The
callee decides which thread to run the function, instead of caller.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_host.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host.cc:164:
BrowserThread::PostTask(
On 2011/10/28 08:37:19, perkj wrote:
> Make sure that this post in this class is moved to the capturemanager.
> Ie,make sure that the capturemanager post this event on the io thread. Ie -
all
> input and output from the manager should be done on the io thread since that
is
> the only thread that it's clients now of.

Same explanation as in video_capture_controller.cc

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:11: #include
"content/browser/renderer_host/media/video_capture_controller_event_handler.h"
On 2011/10/28 08:37:19, perkj wrote:
> can and should this be made 80 by breaking the line?
Header files should be always on one line. This line breaking comes from
display.

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

http://codereview.chromium.org/8304017/diff/43002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:67:
virtual void OnDeviceState(bool in_use) {}
On 2011/10/28 08:37:19, perkj wrote:
> remove

Done.

http://codereview.chromium.org/8304017/diff/43002/content/renderer/media/vide...
File content/renderer/media/video_capture_module_impl.cc (right):

http://codereview.chromium.org/8304017/diff/43002/content/renderer/media/vide...
content/renderer/media/video_capture_module_impl.cc:251: if (!got_first_frame_)
{
On 2011/10/28 08:37:19, perkj wrote:
> Why is this necessary? Why compensate the time? ViE don't care about the
> timestamp of the first frame.
removed.

http://codereview.chromium.org/8304017/diff/43002/content/renderer/media/vide...
content/renderer/media/video_capture_module_impl.cc:260: if (buf->width ==
static_cast<int>(width_) &&
On 2011/10/28 08:37:19, perkj wrote:
> You should never have to care about the frame size. ViE will have to scale
> before doing encoding. Just make sure the frameInfo is correct in each frame.
reverted the change.