Video capture implementation using the Media Foundation API.

Video capture implementation using the Media Foundation API.
This replaces the DirectShow based implementation for Vista,Win7 and Win8.
DirectShow isn't supported in metro mode so once we've landed this,
we can revert the workaround we have in place for Win8.

The CapabilityList class is mostly moved code from the DS implementation.
The difference is that I'm not using a std::map<> since that wasn't
really necessary and instead adding one member (via inheritance) to
the capability struct on Windows that holds the stream id that is needed.

BUG=140545
TEST=All video capture tests in media_unittests now test this new implementation (except on XP).

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

[tommi (sloooow) - chröme, 2012-11-28 10:45:50]

mflodman: Main review.
darin: owner (and please review if you have time)

[darin (slow to review), 2012-11-29 00:28:05]

The change to add more DLLs seems OK to me.

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/ca...
File media/video/capture/win/capability_list_win.cc (right):

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/ca...
media/video/capture/win/capability_list_win.cc:14: struct ResolutionDiff {
looks like this struct and the Compare* functions should be in an anonymous
namespace (within the media namespace).

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:21: using
media::CapabilityList;
nit: i recommend putting the anonymous namespace within the media namespace so
you won't need these using directives.

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:198:
observer_->OnIncomingCapturedFrame(data, length, stamp);
is it possible for the observer_ to destroy |this| within
OnIncomingCapturedFrame?

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_mf_win.h (right):

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.h:12: #include <mfapi.h>
nit: can you avoid including these headers here, but instead just forward
declare the types you need?  or would that be too messy?  it seems nice to
reduce includes in header files.

[tommi (sloooow) - chröme, 2012-11-29 11:02:58]

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/ca...
File media/video/capture/win/capability_list_win.cc (right):

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/ca...
media/video/capture/win/capability_list_win.cc:14: struct ResolutionDiff {
On 2012/11/29 00:28:05, darin wrote:
> looks like this struct and the Compare* functions should be in an anonymous
> namespace (within the media namespace).

Done.

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:21: using
media::CapabilityList;
On 2012/11/29 00:28:05, darin wrote:
> nit: i recommend putting the anonymous namespace within the media namespace so
> you won't need these using directives.

Done.

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:198:
observer_->OnIncomingCapturedFrame(data, length, stamp);
On 2012/11/29 00:28:05, darin wrote:
> is it possible for the observer_ to destroy |this| within
> OnIncomingCapturedFrame?

This particular OnIncomingCapturedFrame is basically an extension of another
OnIncomingCapturedFrame.  In this particular case, the implementation is private
to this object file and |this| cannot be deleted during the call because a
reference is held by the caller of OnReadSample.

However, extending the question to the other OnIncomingCapturedFrame which
belongs to the cross platform VideoCapture interface (see implementation at the
bottom of this file), I guess that's a question that mflodman can provide better
background to. But from what I can see, that is never done, but the API itself
doesn't prevent it.

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_mf_win.h (right):

https://codereview.chromium.org/11419200/diff/4001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.h:12: #include <mfapi.h>
On 2012/11/29 00:28:05, darin wrote:
> nit: can you avoid including these headers here, but instead just forward
> declare the types you need?  or would that be too messy?  it seems nice to
> reduce includes in header files.

Agreed. I could get rid of the mfapi.h header, but the other two include
declspec magic to associate IMFActivate and IMFSourceReader with their IIDs, so
I left them in there.

[mflodman_chromium_OOO, 2012-11-29 20:00:02]

LG. I have a couple of questions regarding MF details, but that might be easier
to discuss offline rather than in the review?

https://codereview.chromium.org/11419200/diff/4003/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/11419200/diff/4003/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:105: *frame_rate =
numerator / denominator;
Protect agains denominator == 0? For DirectShow, that could be reported by some
drivers.

https://codereview.chromium.org/11419200/diff/4003/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:185:
sample->GetBufferCount(&count);
When testing, have you ever seen multiple buffers in the same sample or will
that only happen in practice for oter cases, like media file read and decoding?

[tommi (sloooow) - chröme, 2012-11-30 07:04:06]

https://codereview.chromium.org/11419200/diff/4003/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/11419200/diff/4003/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:105: *frame_rate =
numerator / denominator;
On 2012/11/29 20:00:02, mflodman wrote:
> Protect agains denominator == 0? For DirectShow, that could be reported by
some
> drivers.

Gaa thanks!

https://codereview.chromium.org/11419200/diff/4003/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_mf_win.cc:185:
sample->GetBufferCount(&count);
On 2012/11/29 20:00:02, mflodman wrote:
> When testing, have you ever seen multiple buffers in the same sample or will
> that only happen in practice for oter cases, like media file read and
decoding?

I've never seen that.  From what I've read it's unlikely to happen for webcams
but since the interface is the same for file based streams we've got to loop.

[mflodman_chromium_OOO, 2012-11-30 14:31:10]

LGTM

[darin (slow to review), 2012-12-03 18:15:43]

LGTM

https://codereview.chromium.org/11419200/diff/6006/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/11419200/diff/6006/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:161: VideoCaptureDevice*
VideoCaptureDevice::Create(const Name& device_name) {
nit: might be nice to return scoped_ptr<VideoCaptureDevice> here, but not
necessary for this CL.

[tommi (sloooow) - chröme, 2012-12-03 23:23:05]

On 2012/12/03 18:15:43, darin wrote:
> LGTM
> 
>
https://codereview.chromium.org/11419200/diff/6006/media/video/capture/win/vi...
> File media/video/capture/win/video_capture_device_win.cc (right):
> 
>
https://codereview.chromium.org/11419200/diff/6006/media/video/capture/win/vi...
> media/video/capture/win/video_capture_device_win.cc:161: VideoCaptureDevice*
> VideoCaptureDevice::Create(const Name& device_name) {
> nit: might be nice to return scoped_ptr<VideoCaptureDevice> here, but not
> necessary for this CL.

Thanks. Agreed.  Will do on another cl which will include changes for the other
platforms as well.

[commit-bot: I haz the power, 2012-12-03 23:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/11419200/6006

[commit-bot: I haz the power, 2012-12-03 23:24:12]

Presubmit check for 11419200-6006 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling a cross-platform API that
accepts a wstring, fix the API.
    media/video/capture/win/video_capture_device_mf_win.cc:240

Presubmit checks took 1.9s to calculate.