add components for integration test which will detect breakage of media pipeline with video captu...

add components for integration test which will detect breakage of media pipeline with video capture as video decoder.

This patch does NOT depend on the WebKit patch (https://bugs.webkit.org/show_bug.cgi?id=74882) any longer, since a utility class MediaStreamUtil has been added in webkit_support. On the opposite, that WebKit patch depends on this one.

There are 2 API's for webkit_support::CreateMediaPlayer in order to be backward compatible with WebKit. Once WebKit patch is landed, the old API (with 2 arguments) can be removed.

In that WebKit patch, WebViewHost uses WebKit::WebUserMediaClientMock to return a userMediaClient.
A mocked webkit_media::MediaStreamClient is used in webkit_support::CreateMediaPlayer.

BUG=none
TEST=run video-capture-preview.html
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116749

[scherkus (not reviewing), 2011-12-21 20:26:45]

cool!

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
File media/video/capture/fake_capture_video_decoder.cc (right):

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:18: const
VideoCapture::VideoCaptureCapability& capability)
if you passed in a gfx::Rect and base::TimeDelta for duration you wouldn't need
to depend on VideoCapture code at all -- in fact you could move this class to
media/filters/ and name it something like "VideoFrameGenerator"

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:20: capability_(capability),
this isn't used -- remove?

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:27: frame_interval_ms_ =
base::TimeDelta::FromMilliseconds(interval);
nit: why not name this duration?

also ms suffix isn't needed since it's a TimeDelta

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:90: statistics_callback_ =
stat_callback;
if you're not using this you don't have to save it

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:99:
GenerateFrameOnDecoderThread();
don't even bother saving the callback -- just generate the frame here
immediately run unless stopped

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:102: void
FakeCaptureVideoDecoder::PauseOnDecoderThread(
you shouldn't have to do anything on Pause() -- delete the code and let the
default impl take care of running the callback

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:110: void
FakeCaptureVideoDecoder::FlushOnDecoderThread(
you shouldn't have to do anything on Flush() -- delete the code and let the
default impl take care of running the callback

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:128:
VideoDecoder::Stop(callback);
just run the callback yourself :)

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:131: void
FakeCaptureVideoDecoder::SeekOnDecoderThread(
you shouldn't have to do anything on Seek() -- delete the code and let the
default impl take care of running the callback

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:138:
host()->SetNaturalVideoSize(natural_size_);
this shouldn't be needed as VideoRendererBase calls this method

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:155: base::TimeDelta(),
every timestamp is zero?

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
File media/video/capture/fake_capture_video_decoder.h (right):

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.h:47: enum DecoderState {
you can replace all of state with a bool for stopped_

this decoder in particular doesn't need to worry about any of the other states

http://codereview.chromium.org/8887004/diff/12004/webkit/support/test_media_s...
File webkit/support/test_media_stream_client.h (right):

http://codereview.chromium.org/8887004/diff/12004/webkit/support/test_media_s...
webkit/support/test_media_stream_client.h:23: WebKit::WebUserMediaClientMock*
userMediaClientMock);
fix to unix_hacker style here + below

http://codereview.chromium.org/8887004/diff/12004/webkit/support/test_media_s...
webkit/support/test_media_stream_client.h:24: virtual ~TestMediaStreamClient() {
}
{ } -> {}

http://codereview.chromium.org/8887004/diff/12004/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/8887004/diff/12004/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:68: WebKit::WebMediaPlayer* CreateMediaPlayer(
is there another part to this patch that calls this method?

having non-WebKit namespace code here (webkit_support) seems suspicious and
might be a layering violation but I'm not completely familiar with this code

[wjia(left Chromium), 2012-01-03 17:08:12]

PTAL.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
File media/video/capture/fake_capture_video_decoder.cc (right):

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:18: const
VideoCapture::VideoCaptureCapability& capability)
On 2011/12/21 20:26:45, scherkus wrote:
> if you passed in a gfx::Rect and base::TimeDelta for duration you wouldn't
need
> to depend on VideoCapture code at all -- in fact you could move this class to
> media/filters/ and name it something like "VideoFrameGenerator"

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:20: capability_(capability),
On 2011/12/21 20:26:45, scherkus wrote:
> this isn't used -- remove?

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:27: frame_interval_ms_ =
base::TimeDelta::FromMilliseconds(interval);
On 2011/12/21 20:26:45, scherkus wrote:
> nit: why not name this duration?
> 
> also ms suffix isn't needed since it's a TimeDelta

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:90: statistics_callback_ =
stat_callback;
On 2011/12/21 20:26:45, scherkus wrote:
> if you're not using this you don't have to save it

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:99:
GenerateFrameOnDecoderThread();
On 2011/12/21 20:26:45, scherkus wrote:
> don't even bother saving the callback -- just generate the frame here
> immediately run unless stopped

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:102: void
FakeCaptureVideoDecoder::PauseOnDecoderThread(
On 2011/12/21 20:26:45, scherkus wrote:
> you shouldn't have to do anything on Pause() -- delete the code and let the
> default impl take care of running the callback

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:110: void
FakeCaptureVideoDecoder::FlushOnDecoderThread(
On 2011/12/21 20:26:45, scherkus wrote:
> you shouldn't have to do anything on Flush() -- delete the code and let the
> default impl take care of running the callback

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:128:
VideoDecoder::Stop(callback);
On 2011/12/21 20:26:45, scherkus wrote:
> just run the callback yourself :)

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:131: void
FakeCaptureVideoDecoder::SeekOnDecoderThread(
On 2011/12/21 20:26:45, scherkus wrote:
> you shouldn't have to do anything on Seek() -- delete the code and let the
> default impl take care of running the callback

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:138:
host()->SetNaturalVideoSize(natural_size_);
On 2011/12/21 20:26:45, scherkus wrote:
> this shouldn't be needed as VideoRendererBase calls this method

Done.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.cc:155: base::TimeDelta(),
On 2011/12/21 20:26:45, scherkus wrote:
> every timestamp is zero?

I thought frame_duration is good enough. current_time has been added.

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
File media/video/capture/fake_capture_video_decoder.h (right):

http://codereview.chromium.org/8887004/diff/12004/media/video/capture/fake_ca...
media/video/capture/fake_capture_video_decoder.h:47: enum DecoderState {
On 2011/12/21 20:26:45, scherkus wrote:
> you can replace all of state with a bool for stopped_
> 
> this decoder in particular doesn't need to worry about any of the other states

Done.

http://codereview.chromium.org/8887004/diff/12004/webkit/support/test_media_s...
File webkit/support/test_media_stream_client.h (right):

http://codereview.chromium.org/8887004/diff/12004/webkit/support/test_media_s...
webkit/support/test_media_stream_client.h:23: WebKit::WebUserMediaClientMock*
userMediaClientMock);
On 2011/12/21 20:26:45, scherkus wrote:
> fix to unix_hacker style here + below

Done.

http://codereview.chromium.org/8887004/diff/12004/webkit/support/test_media_s...
webkit/support/test_media_stream_client.h:24: virtual ~TestMediaStreamClient() {
}
On 2011/12/21 20:26:45, scherkus wrote:
> { } -> {}

Done.

http://codereview.chromium.org/8887004/diff/12004/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/8887004/diff/12004/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:68: WebKit::WebMediaPlayer* CreateMediaPlayer(
On 2011/12/21 20:26:45, scherkus wrote:
> is there another part to this patch that calls this method?

This method is called only from
WebKit/Tools/DumpRenderTree/chromium/WebViewHost.cpp and webkit_support can be
used there.

> 
> having non-WebKit namespace code here (webkit_support) seems suspicious and
> might be a layering violation but I'm not completely familiar with this code

[wjia(left Chromium), 2012-01-05 23:56:47]

+ @tkent for webkit/support.

[tkent, 2012-01-06 02:03:14]

LGTM for webkit/support.

[acolwell GONE FROM CHROMIUM, 2012-01-06 18:09:16]

LGTM w/ nits

http://codereview.chromium.org/8887004/diff/45002/media/filters/video_frame_g...
File media/filters/video_frame_generator.cc (right):

http://codereview.chromium.org/8887004/diff/45002/media/filters/video_frame_g...
media/filters/video_frame_generator.cc:10: #include "media/base/filter_host.h"
nit: I don't think this include is needed.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
File webkit/support/test_media_stream_client.cc (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
webkit/support/test_media_stream_client.cc:33: if (raw_media) {
nit: Reversing the condition and returning early will allow you to reduce the
indent level of this code. You also don't need the local variable.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
File webkit/support/test_media_stream_client.h (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
webkit/support/test_media_stream_client.h:13: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebURL.h"
nit: includes should be in alphabetical order.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:61: #include
"webkit/support/test_media_stream_client.h"
nit: If you change CreateMediaPlayer() to take a MediaStreamClient, I don't
think you'll need this.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:71: webkit_support::TestMediaStreamClient*
test_media_stream_client);
Is there any reason why this can't just be MediaStreamClient? It doesn't look
like this code needs to know that it is dealing with a test implementation.

[wjia(left Chromium), 2012-01-06 22:18:59]

http://codereview.chromium.org/8887004/diff/45002/media/filters/video_frame_g...
File media/filters/video_frame_generator.cc (right):

http://codereview.chromium.org/8887004/diff/45002/media/filters/video_frame_g...
media/filters/video_frame_generator.cc:10: #include "media/base/filter_host.h"
On 2012/01/06 18:09:16, acolwell wrote:
> nit: I don't think this include is needed.

Done.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
File webkit/support/test_media_stream_client.cc (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
webkit/support/test_media_stream_client.cc:33: if (raw_media) {
On 2012/01/06 18:09:16, acolwell wrote:
> nit: Reversing the condition and returning early will allow you to reduce the
> indent level of this code. You also don't need the local variable.

Done.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
File webkit/support/test_media_stream_client.h (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/test_media_s...
webkit/support/test_media_stream_client.h:13: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebURL.h"
On 2012/01/06 18:09:16, acolwell wrote:
> nit: includes should be in alphabetical order.

Done.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:61: #include
"webkit/support/test_media_stream_client.h"
On 2012/01/06 18:09:16, acolwell wrote:
> nit: If you change CreateMediaPlayer() to take a MediaStreamClient, I don't
> think you'll need this.

Done.

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/8887004/diff/45002/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:71: webkit_support::TestMediaStreamClient*
test_media_stream_client);
On 2012/01/06 18:09:16, acolwell wrote:
> Is there any reason why this can't just be MediaStreamClient? It doesn't look
> like this code needs to know that it is dealing with a test implementation.

changed to MediaStreamClient.