Re-implement DirectShow base classes for video capture

RESOLVED FIXED in mozilla16

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jesup, Unassigned)

Tracking

Trunk
mozilla16
All
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
To avoid using the DirectShow sample base classes, we should use our own implementations of them.  Luckily, cpearce has already done most of this work as part of bug 435339.  We wouldn't need the integration with <video>, etc; just the base classes, and then recode video_capture_windows.cc and sink_filter_windows.cc to use those base classes (probably as an "external" video capture implementation to avoid touching mozilla includes from deep within webrtc).

Note that we'll want to avoid the "samplegrabber" function as it's deprecated and unsupported (and may not exist in Windows 8), and instead probably do like sink_filter_windows does an implement a sink filter.
jesup: is there scope to collaborate with the WebRTC library team to use our code to replace their use of the DirectShow sample base classes (i.e. upstream our changes), or would that add significantly to the cost and scope of the project?

Gerv
(Reporter)

Comment 2

6 years ago
I think it's possible to do so; in fact it's possible that the Chromium people might want to use it as well - Justin Uberti spoke to them being unhappy with the same issue; he wasn't sure if Chromium had replaced those base classes yet or not.  They did (or do) apparently have them within Chromium itself, separate from their use in webrtc.org code.

We could submit them in a webrtc.org bug as a patch once we've proven they work and reimplemented the video capture code using them.
(Reporter)

Comment 3

6 years ago
Created attachment 602338 [details] [diff] [review]
BasePin and BaseFilter base classes reimplemented in mozilla (from cpearce in bug 435399)
(Reporter)

Comment 4

6 years ago
Note: untested so far, just extracted and inserted into the currently appropriate portion of the tree.

Note that the API is not identical to the Microsoft examples (CBaseInputPin and CBaseFilter), so the video capture filters will need to be rewritten for the nsBase* APIs.
(Reporter)

Comment 5

6 years ago
The filter is in 
media/webrtc/trunk/src/modules/video_capture/main/source/Windows/sink_filter_windows.h
and 
media/webrtc/trunk/src/modules/video_capture/main/source/Windows/sink_filter_windows.cc
(on the alder branch:   http://hg.mozilla.org/projects/alder)

It's possible that other files in that directory will need changes, but I don't think so.

Comment 6

6 years ago
Randell, do we know what else is needed here to make this go yet?
(Reporter)

Comment 7

6 years ago
Per discussion with cpearce: we'd need to implement a BaseInputPin class and debug it, and then port over sink_filter_windows.cc to use the nsBlah base classes (if possible in a way that can survive updates of the base file without hand-merging).
(Reporter)

Updated

5 years ago
Attachment #602338 - Attachment is obsolete: true
Created attachment 605926 [details] [diff] [review]
Remove DShow BaseClass usage

This patch does the trick for me, it relies on no mozilla code and on none of the dshow base classes.

It does do one thing tricky, which is from BaseInputPin it doesn't return an IMemAllocator from GetAllocator, this works fine on my machine since the filter on the other side will simply pick an allocator, I'm not sure this is true everywhere, if it isn't IMemAllocator and IMediaSample will need to be implemented as well, I've got this mostly done though in a separate patch, but we should see if this works as it's a little simpler.
Attachment #605926 - Flags: feedback?(rjesup)
(Reporter)

Comment 9

5 years ago
Comment on attachment 605926 [details] [diff] [review]
Remove DShow BaseClass usage

Review of attachment 605926 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, one minor nit (double -> 32-bit int).  With that dealt with, feedback+ (but I'm not really appropriate to review the newly-added files).

::: media/webrtc/trunk/src/modules/video_capture/main/source/Windows/sink_filter_windows.h
@@ +14,4 @@
>  #include "video_capture_defines.h"
> +#include "BaseInputPin.h"
> +#include "BaseFilter.h"
> +#include "MediaType.h"

Might want to add a comment that these replace the DirectShow sample base classes

::: netwerk/protocol/device/GipsCaptureProvider.cpp
@@ +146,5 @@
>          mCap.rawType == webrtc::kVideoRGB24,
>          mCap.rawType == webrtc::kVideoI420,
>          mCap.rawType == webrtc::kVideoARGB,
>          mCap.rawType);
> +    if (abs(double(mCap.width * mCap.height - mWidth * mHeight)) < sizeDelta) {

Integer type please, not double.  PRInt32 should be more than sufficient until we have 2 gigapixel displays; I think all 4 vars here are unsigned.
Attachment #605926 - Flags: review?(cpearce)
Attachment #605926 - Flags: feedback?(rjesup)
Attachment #605926 - Flags: feedback+
Comment on attachment 605926 [details] [diff] [review]
Remove DShow BaseClass usage

Review of attachment 605926 [details] [diff] [review]:
-----------------------------------------------------------------

All the license boilerplate for new files should be the new MPL2 boilerplate from: http://www.mozilla.org/MPL/headers/

We may in the not-too-distant future want to use these base classes and friends in content/media/ in order to write an MP3 decoder. Unless anyone has some bright idea now, we can delay figuring out how to expose them until the decision is made to support MP3.
Attachment #605926 - Flags: review?(cpearce) → review+
Is MOZ_COUNT_CTOR/DTOR able to be used in these classes to help detect/guard against leaks?
(In reply to Chris Pearce (:cpearce) from comment #11)
> Is MOZ_COUNT_CTOR/DTOR able to be used in these classes to help detect/guard
> against leaks?

Nope, they're implemented in XPCOM which we can't link against.

(In reply to Chris Pearce (:cpearce) from comment #10)
> Comment on attachment 605926 [details] [diff] [review]
> Remove DShow BaseClass usage
> 
> Review of attachment 605926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All the license boilerplate for new files should be the new MPL2 boilerplate
> from: http://www.mozilla.org/MPL/headers/
> 
> We may in the not-too-distant future want to use these base classes and
> friends in content/media/ in order to write an MP3 decoder. Unless anyone
> has some bright idea now, we can delay figuring out how to expose them until
> the decision is made to support MP3.

I tried exposing them in the other direction (from content/media to WebRTC) - that didn't work for me.
I pushed this to alder.

https://hg.mozilla.org/projects/alder/rev/4de595e8ec8b

Sadly I messed up the comment and some of my alterations to make windows builds work slipped in as well.
(Reporter)

Comment 14

5 years ago
Created attachment 612278 [details] [diff] [review]
need to include <assert.h> in non-debug builds (unproven)
(Reporter)

Comment 15

5 years ago
Comment on attachment 612278 [details] [diff] [review]
need to include <assert.h> in non-debug builds (unproven)

Pretty well proven now... :-)  Just need review
Attachment #612278 - Flags: review?(cpearce)
Attachment #612278 - Flags: review?(cpearce) → review+
(Reporter)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1e066a4b6176
(Reporter)

Comment 17

5 years ago
Relanding:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2ad9ba0e0ccb
https://hg.mozilla.org/mozilla-central/rev/b7c47028aa52
https://hg.mozilla.org/mozilla-central/rev/8fe9bb1baa2d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.