Last Comment Bug 731407 - Re-implement DirectShow base classes for video capture
: Re-implement DirectShow base classes for video capture
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: WebRTC: Audio/Video (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 688178
  Show dependency treegraph
 
Reported: 2012-02-28 14:21 PST by [:jesup] on pto until 2016/8/1 Randell Jesup
Modified: 2012-07-27 10:32 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
BasePin and BaseFilter base classes reimplemented in mozilla (from cpearce in bug 435399) (36.46 KB, patch)
2012-03-02 06:28 PST, [:jesup] on pto until 2016/8/1 Randell Jesup
no flags Details | Diff | Splinter Review
Remove DShow BaseClass usage (88.46 KB, patch)
2012-03-14 13:31 PDT, Bas Schouten (:bas.schouten)
cpearce: review+
rjesup: feedback+
Details | Diff | Splinter Review
need to include <assert.h> in non-debug builds (unproven) (3.94 KB, patch)
2012-04-04 11:50 PDT, [:jesup] on pto until 2016/8/1 Randell Jesup
cpearce: review+
Details | Diff | Splinter Review

Description [:jesup] on pto until 2016/8/1 Randell Jesup 2012-02-28 14:21:13 PST
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.
Comment 1 Gervase Markham [:gerv] 2012-03-01 04:14:19 PST
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
Comment 2 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-03-01 05:29:49 PST
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.
Comment 3 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-03-02 06:28:18 PST
Created attachment 602338 [details] [diff] [review]
BasePin and BaseFilter base classes reimplemented in mozilla (from cpearce in bug 435399)
Comment 4 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-03-02 06:43:00 PST
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.
Comment 5 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-03-02 06:46:52 PST
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 JP Rosevear [:jpr] 2012-03-08 16:15:58 PST
Randell, do we know what else is needed here to make this go yet?
Comment 7 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-03-08 20:31:59 PST
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).
Comment 8 Bas Schouten (:bas.schouten) 2012-03-14 13:31:25 PDT
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.
Comment 9 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-03-14 14:17:40 PDT
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.
Comment 10 Chris Pearce (:cpearce) 2012-03-14 19:41:27 PDT
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.
Comment 11 Chris Pearce (:cpearce) 2012-03-14 19:56:52 PDT
Is MOZ_COUNT_CTOR/DTOR able to be used in these classes to help detect/guard against leaks?
Comment 12 Bas Schouten (:bas.schouten) 2012-03-15 11:39:27 PDT
(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.
Comment 13 Bas Schouten (:bas.schouten) 2012-03-15 16:14:04 PDT
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.
Comment 14 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-04-04 11:50:44 PDT
Created attachment 612278 [details] [diff] [review]
need to include <assert.h> in non-debug builds (unproven)
Comment 15 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-05-22 16:16:52 PDT
Comment on attachment 612278 [details] [diff] [review]
need to include <assert.h> in non-debug builds (unproven)

Pretty well proven now... :-)  Just need review
Comment 16 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-06-20 05:26:46 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1e066a4b6176
Comment 17 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-06-21 06:53:29 PDT
Relanding:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2ad9ba0e0ccb

Note You need to log in before you can comment on or make changes to this bug.