Closed
Bug 861693
Opened 12 years ago
Closed 12 years ago
DirectShow backend for MP3 decoding on WinXP
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 26+ |
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(4 files)
2.71 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
3.64 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
90.46 KB,
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
Windows Media Foundation isn't available on WinXP, so we need an MP3 decoding solution. So we'll use DirectShow on WinXP.
Previous DirectShow backend: bug 435339.
WebRTC used some of that code for video capture:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/windows
My previous DirectShow backend is fatally bitrotted, it's based on the old architecture where we subclassed nsMediaDecoder and reimplemented everything for every backend, rather than only subclassing MediaReader. But I can still use some of the code.
The basic plan is to use the nsSourceFilter I wrote for bug 435339, and write an audio sink filter that feeds data into our audio pipeline. There's a video capture sink filter in the WebRTC code that I can use as a guide. I should probably build the graph manually so that other filter's can't be invoked.
Assignee | ||
Comment 1•12 years ago
|
||
Status update: I got this green on try two weeks ago, but I'm having trouble finding time to work on it. Am hoping to get back to it next week or later on this week.
https://tbpl.mozilla.org/?tree=Try&rev=ff5fd7c9668a
Assignee | ||
Comment 2•12 years ago
|
||
Note to self: call NotifyBytesConsumed() in DirectShowReader::DecodeAudioData().
Assignee | ||
Comment 3•12 years ago
|
||
* Add MOZ_DIRECTSHOW define when building on Windows.
* Add --disable-directshow configure arg to disable it.
* Add media.directshow.enabled pref. On by default.
Attachment #784123 -
Flags: review?(ted)
Assignee | ||
Comment 4•12 years ago
|
||
Some fixes to our DirectShow Base Classes replacement:
* Make BaseFilter::~BaseFilter virtual. I leaked without this. Destructors should be virtual.
* Move some #defines around so that I can use these headers in content/media without build errors.
Attachment #784131 -
Flags: review?(rjesup)
Assignee | ||
Comment 5•12 years ago
|
||
* Use DirectShow for MP3 playback on all Windows platforms.
* I construct a filter graph explicitly, so filters I don't know about won't get a chance to touch the bitstream.
* I feed data into the filter graph using a SourceFilter that wraps MediaResource.
* I extract decoded samples using a AudioSinkFilter.
* See comment in DirectShowReader.h for a more detaild explanation of the architecture.
Attachment #784134 -
Flags: review?(paul)
Assignee | ||
Comment 6•12 years ago
|
||
* Makefile/moz.build changes for files added in previous patch.
* Note we still need to link dshow libs if webrtc is disabled (as it is in comm-central) else this won't build.
* Note we still need to build some of the files in webrtc/trunk/webrtc/modules/video_capture/windows/ if webrtc is disabled (as it is in comm-central) else this won't build.
Attachment #784138 -
Flags: review?(gps)
Updated•12 years ago
|
Attachment #784131 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 7•12 years ago
|
||
Comment 8•12 years ago
|
||
Comment on attachment 784138 [details] [diff] [review]
Patch 4 v1: Makefile/moz.build changes for DirectShow MP3 decoder
Review of attachment 784138 [details] [diff] [review]:
-----------------------------------------------------------------
I trust you will address things before landing.
::: content/media/Makefile.in
@@ +19,3 @@
> include $(topsrcdir)/ipc/chromium/chromium-config.mk
>
> INCLUDES += \
While you are here, please change this to LOCAL_INCLUDES.
@@ +26,5 @@
>
> +ifdef MOZ_DIRECTSHOW
> +LOCAL_INCLUDES += -I$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/
> +endif
> +
Nit: trailing whitespace
::: content/media/directshow/Makefile.in
@@ +5,5 @@
> +DEPTH = @DEPTH@
> +topsrcdir = @top_srcdir@
> +srcdir = @srcdir@
> +VPATH = @srcdir@
> +FAIL_ON_WARNINGS := 1
Nit: Put FAIL_ON_WARNINGS after autoconf.mk include. The rule is pretty much nothing should go before autoconf.mk.
::: content/media/directshow/moz.build
@@ +20,5 @@
> + 'DirectShowDecoder.cpp',
> + 'DirectShowReader.cpp',
> + 'DirectShowUtils.cpp',
> + 'SampleSink.cpp',
> + 'SourceFilter.cpp',
Nit: 4 spaces, not tabs.
@@ +25,5 @@
> +]
> +
> +# If WebRTC isn't being built, we need to compile the DirectShow base classes so that
> +# they're available at link time.
> +if CONFIG['MOZ_WEBRTC_IN_LIBXUL'] != "1":
You shouldn't need the |!= "1"| bit. I believe MOZ_WEBRTC_IN_LIBXUL will just not be defined.
@@ +30,5 @@
> + CPP_SOURCES += [
> + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/BaseFilter.cpp',
> + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/BaseInputPin.cpp',
> + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/BasePin.cpp',
> + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/MediaType.cpp',
Use TOPSRCDIR instead of $(topsrcdir). As-is works because this string is being added to a make file. This won't always work. If you copied this from somewhere, please point me at it so it can be fixed.
Attachment #784138 -
Flags: review?(gps) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Thanks for the quick review.
(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 784138 [details] [diff] [review]
> Patch 4 v1: Makefile/moz.build changes for DirectShow MP3 decoder
>
> Review of attachment 784138 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +30,5 @@
> > + CPP_SOURCES += [
> > + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/BaseFilter.cpp',
> > + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/BaseInputPin.cpp',
> > + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/BasePin.cpp',
> > + '$(topsrcdir)/media/webrtc/trunk/webrtc/modules/video_capture/windows/MediaType.cpp',
>
> Use TOPSRCDIR instead of $(topsrcdir). As-is works because this string is
> being added to a make file. This won't always work. If you copied this from
> somewhere, please point me at it so it can be fixed.
So should that be:
CPP_SOURCES += [
'TOPSRCDIR/media/webrtc/trunk/webrtc/modules/video_capture/windows/BaseFilter.cpp',
...
?
I don't actually see TOPSRCDIR being used in any moz.build files, whereas $(topsrcdir) is used all over the place:
http://mxr.mozilla.org/mozilla-central/search?string=topsrcdir&find=moz.build&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 10•12 years ago
|
||
mfind '\$(topsrcdir' | wc -> 250 uses
mfind '\$(TOPSRCDIR' | wc -> 52 uses
and 87 in moz.build files, vs none for TOPSRCDIR
Updated•12 years ago
|
Attachment #784123 -
Flags: review?(ted) → review+
Comment 11•12 years ago
|
||
TOPSRCDIR is exposed as a Python variable inside moz.build files. e.g.
CPP_SOURCES += [TOPSRCDIR + '/media/webrtc/foo.cpp']
$(topsrcdir) and other make variable references exist in moz.build files so that conversion could be performed easier. There should be open bugs to remove all of them because their occurrence is wrong.
To see what all is exposed to the moz.build Python environment, run |mach mozbuild-reference|.
Comment 12•12 years ago
|
||
Comment on attachment 784134 [details] [diff] [review]
Patch 3 v1: DirectShow MP3 backend
Review of attachment 784134 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/directshow/DirectShowDecoder.cpp
@@ +8,5 @@
> +#include "DirectShowReader.h"
> +#include "MediaDecoderStateMachine.h"
> +#include "mozilla/Preferences.h"
> +
> +using namespace mozilla::widget;
You don't need this.
::: content/media/directshow/DirectShowReader.cpp
@@ +343,5 @@
> + {
> + ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> + durationUs = mDecoder->GetMediaDuration();
> + }
> + GetEstimatedBufferedTimeRanges(stream, durationUs, aBuffered);
Conveniently, a big machinery just got finished to be able to have real buffered ranges and duration for mp3.
This is in bug 831224.
::: content/media/directshow/DirectShowReader.h
@@ +28,5 @@
> +// Decoder backend for decoding MP3 using DirectShow. DirectShow operates as
> +// a filter graph. The basic design of the DirectShowReader is that we have
> +// a SourceFilter that wraps the MediaResource that connects to the
> +// MP3 decoder filter. The MP3 decoder filter "pulls" data as it requires it
> +// downstream on its own thread. When the MP3 decoder as produced a block of
s/as/has/
::: content/media/directshow/SourceFilter.cpp
@@ +37,5 @@
> +class ReadRequest {
> +public:
> +
> + ReadRequest(IMediaSample* aSample,
> + DWORD_PTR dwUser,
aArgument syntax.
@@ +204,5 @@
> +
> +HRESULT
> +OutputPin::BreakConnect()
> +{
> + mQueriedForAsyncReader = false;
Maybe you want to decrement the pin count here.
@@ +274,5 @@
> + return VFW_S_NO_MORE_ITEMS;
> +}
> +
> +static inline bool
> +IsPowerOf2(PRInt32 x) {
s/PRInt32/int32_t/
@@ +350,5 @@
> + return hr;
> +}
> +
> +STDMETHODIMP
> +OutputPin::Request(IMediaSample* aSample, DWORD_PTR dwUser)
aArgument syntax.
@@ +392,5 @@
> +
> +STDMETHODIMP
> +OutputPin::WaitForNext(DWORD dwTimeout,
> + IMediaSample** ppSample,
> + DWORD_PTR* pdwUser)
Use the aArgument syntax here as well.
@@ +458,5 @@
> + return VFW_E_SAMPLE_TIME_NOT_SET;
> + }
> +
> + // Convert reference time to bytes.
> + PRInt32 start = (PRInt32)(lStart / 10000000);
s/PRInt32/int32_t/, and below.
@@ +464,5 @@
> +
> + PRInt32 numBytes = end - start;
> +
> + // If the range extends off the end of stream, truncate to the end of stream
> + // as per IAsynReader specificiation.
s/IAsynReader/IAsyncReader/
::: content/media/wmf/WMFDecoder.cpp
@@ +35,5 @@
> + // DirectShowDecoder is enabled, we use that in preference to the WMF
> + // backend.
> + return false;
> + }
> +#endif
trailing space.
Attachment #784134 -
Flags: review?(paul) → review+
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #12)
> Comment on attachment 784134 [details] [diff] [review]
> Conveniently, a big machinery just got finished to be able to have real
> buffered ranges and duration for mp3.
Cool, we can handle that in a follow up bug.
> @@ +204,5 @@
> > +
> > +HRESULT
> > +OutputPin::BreakConnect()
> > +{
> > + mQueriedForAsyncReader = false;
>
> Maybe you want to decrement the pin count here.
Thanks for pointing this out. It turns out that SourceFilter::Connect() isn't actually called, and that's the only user of mPinUsedCount, so I'll remove SourceFilter::Connect() and mPinUsedCount. I must have cargo-culted this from the video capture filter.
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Comment 15•12 years ago
|
||
Landed in Nightly for Firefox 26:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c323132512
https://hg.mozilla.org/integration/mozilla-inbound/rev/533d68380bbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b920cb3e84
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7fef018f421
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4c323132512
https://hg.mozilla.org/mozilla-central/rev/533d68380bbd
https://hg.mozilla.org/mozilla-central/rev/b2b920cb3e84
https://hg.mozilla.org/mozilla-central/rev/e7fef018f421
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee | ||
Updated•12 years ago
|
relnote-firefox:
--- → ?
Comment 17•11 years ago
|
||
After landing this bug, it would appear that Windows 7/Vista machines are now using DirectShow to decode mp3 files. If you attempt to play this file [1] in the latest nightly on a Win 7/Vista machine, it will tell you that the "Video cannot be played because the file is corrupt". The browser console gives the following message: Media resource http://nxt-level.com/music/02.%20everlast%20-%20black%20jesus.mp3 could not be decoded. However, setting media.directshow.enabled = false causes the file to play without any problems.
Last good nightly: 2013-08-13
First bad nightly: 2013-08-14
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c146d402a55f&tochan
ge=3c61cc01a3b1
[1] http://nxt-level.com/music/02.%20everlast%20-%20black%20jesus.mp3
Assignee | ||
Comment 18•11 years ago
|
||
I spun off bug 908862 to deal with this.
Comment 19•11 years ago
|
||
Does this needs QA validation given the automation coverage?
Comment 20•11 years ago
|
||
Release note should be something like "Firefox now supports MP3 decoding on Windows XP, completing MP3 support across Windows OS versions." Feel free to suggest something better if you've got it. Thanks.
Updated•11 years ago
|
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•