handle the case when device is closed before media pipeline is fully initialized.

handle the case when device is closed before media pipeline is fully initialized.
CaptureVideoDecoder needs a signal for pre-matured closing of device. Then it can fulfill preroll request from VideoRenderBase.
Add unit tests for VideoCaptureController.

BUG=127552
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=139559

[Ronghua Wu (Left Chromium), 2012-05-17 22:54:37]

Can you add unit tests for the new methods?

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

http://codereview.chromium.org/10391065/diff/1011/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:220:
client->event_handler->OnPaused(client->controller_id);
Should we call this be called OnStop?

http://codereview.chromium.org/10391065/diff/1011/content/renderer/media/capt...
File content/renderer/media/capture_video_decoder.cc (right):

http://codereview.chromium.org/10391065/diff/1011/content/renderer/media/capt...
content/renderer/media/capture_video_decoder.cc:87:
message_loop_proxy_->PostTask(
Add comment why we do OnPusedOnDecoderThread in the OnError callback.

[phoglund_chromium, 2012-05-21 09:39:31]

On 2012/05/17 22:54:37, Ronghua Wu wrote:
> Can you add unit tests for the new methods?
> 
>
http://codereview.chromium.org/10391065/diff/1011/content/browser/renderer_ho...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
http://codereview.chromium.org/10391065/diff/1011/content/browser/renderer_ho...
> content/browser/renderer_host/media/video_capture_controller.cc:220:
> client->event_handler->OnPaused(client->controller_id);
> Should we call this be called OnStop?
> 
>
http://codereview.chromium.org/10391065/diff/1011/content/renderer/media/capt...
> File content/renderer/media/capture_video_decoder.cc (right):
> 
>
http://codereview.chromium.org/10391065/diff/1011/content/renderer/media/capt...
> content/renderer/media/capture_video_decoder.cc:87:
> message_loop_proxy_->PostTask(
> Add comment why we do OnPusedOnDecoderThread in the OnError callback.

How's it going with this patch? It would be great to get it in since it
apparently fixes the bug which is breaking the media_stream_infobar.py PyAuto
test. You can run the WebRTC PyAuto tests like this from your chrome src/
directory:

./chrome/test/functional/media_stream_infobar.py
./chrome/test/functional/webrtc_call.py

[wjia(left Chromium), 2012-05-23 18:46:48]

PTAL.

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

http://codereview.chromium.org/10391065/diff/1011/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:220:
client->event_handler->OnPaused(client->controller_id);
On 2012/05/17 22:54:37, Ronghua Wu wrote:
> Should we call this be called OnStop?

|Stop| has been used StopCapture. |Pause| is used here to indicate that the
session has been stopped, and there is no frame sent out by
VideoCaptureController any more.

http://codereview.chromium.org/10391065/diff/1011/content/renderer/media/capt...
File content/renderer/media/capture_video_decoder.cc (right):

http://codereview.chromium.org/10391065/diff/1011/content/renderer/media/capt...
content/renderer/media/capture_video_decoder.cc:87:
message_loop_proxy_->PostTask(
On 2012/05/17 22:54:37, Ronghua Wu wrote:
> Add comment why we do OnPusedOnDecoderThread in the OnError callback.

This is the video capture protocol between browser process and renderer process.
When there is an error, browser process will no longer send frame over. It's
stated in VideoCaptureHost.

[wjia(left Chromium), 2012-05-24 00:18:14]

+mflodman@

[Ronghua Wu (Left Chromium), 2012-05-29 01:16:50]

LGTM with few minor comments. Nice tests.

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:30: 
Remove empty line.

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:60:
~MockVideoCaptureControllerEventHandler() {}
make the destructor virtual since you have virtual methods in this class.

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:145:
virtual ~MockVideoCaptureManager() {}
Usually virtual destructor shouldn't be private. Looks like you don't need a
virtual dctor in this case.

[wjia(left Chromium), 2012-05-29 03:51:12]

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:30: 
On 2012/05/29 01:16:50, Ronghua Wu wrote:
> Remove empty line.

Done.

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:60:
~MockVideoCaptureControllerEventHandler() {}
On 2012/05/29 01:16:50, Ronghua Wu wrote:
> make the destructor virtual since you have virtual methods in this class.

Done.

http://codereview.chromium.org/10391065/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:145:
virtual ~MockVideoCaptureManager() {}
On 2012/05/29 01:16:50, Ronghua Wu wrote:
> Usually virtual destructor shouldn't be private. Looks like you don't need a
> virtual dctor in this case.

MockVideoCaptureManager is ref-counted. Therefore, dtor needs to be either
private or protected.

[wjia(left Chromium), 2012-05-29 03:53:45]

+ jam@ for content/content_tests.gypi
  scherkus@ for media/video/capture/video_capture_device.h

[mflodman_chromium_OOO, 2012-05-29 09:11:03]

LG, just a minor style nit and one question.

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller.h:36: class
CONTENT_EXPORT VideoCaptureController
Is CONTENT_EXPORT really needed? I didn't find where that should be needed.

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:148: //
Test class
nit: end with '.'.

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host.h (right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host.h:141: void DoPaused(int
device_id);
Do we want a comment it should be called on IO-thread?

[wjia(left Chromium), 2012-05-29 16:59:14]

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller.h:36: class
CONTENT_EXPORT VideoCaptureController
On 2012/05/29 09:11:03, mflodman wrote:
> Is CONTENT_EXPORT really needed? I didn't find where that should be needed.

It's needed by VideoCaptureControllerTest.

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:148: //
Test class
On 2012/05/29 09:11:03, mflodman wrote:
> nit: end with '.'.

Done.

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host.h (right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host.h:141: void DoPaused(int
device_id);
On 2012/05/29 09:11:03, mflodman wrote:
> Do we want a comment it should be called on IO-thread?

All Do* functions have been renamed to DO*OnIOThread to make it clear that those
functions should be called on IO thread.

[jam, 2012-05-29 17:33:25]

On 2012/05/29 03:53:45, wjia wrote:
> + jam@ for content/content_tests.gypi

lgtm

[mflodman_chromium_OOO, 2012-05-29 18:19:53]

LGTM

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller.h:36: class
CONTENT_EXPORT VideoCaptureController
On 2012/05/29 16:59:14, wjia wrote:
> On 2012/05/29 09:11:03, mflodman wrote:
> > Is CONTENT_EXPORT really needed? I didn't find where that should be needed.
> 
> It's needed by VideoCaptureControllerTest.

Ah, got it.

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host.h (right):

http://codereview.chromium.org/10391065/diff/47001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host.h:141: void DoPaused(int
device_id);
On 2012/05/29 16:59:14, wjia wrote:
> On 2012/05/29 09:11:03, mflodman wrote:
> > Do we want a comment it should be called on IO-thread?
> 
> All Do* functions have been renamed to DO*OnIOThread to make it clear that
those
> functions should be called on IO thread.

Nice!