DirectShow backend for MP3 decoding on WinXP

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
mozilla26
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 26+)

Details

Attachments

(4 attachments)

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.

Updated

6 years ago
Blocks: 799318
(Assignee)

Updated

6 years ago
Depends on: 872866
(Assignee)

Comment 1

6 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

6 years ago
Note to self: call NotifyBytesConsumed() in DirectShowReader::DecodeAudioData().
(Assignee)

Comment 3

6 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

6 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

6 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

6 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)
Attachment #784131 - Flags: review?(rjesup) → review+
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

6 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
mfind '\$(topsrcdir' | wc -> 250 uses
mfind '\$(TOPSRCDIR' | wc -> 52 uses

and 87 in moz.build files, vs none for TOPSRCDIR
Attachment #784123 - Flags: review?(ted) → review+
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 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

6 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.
Depends on: 905592
(Assignee)

Updated

6 years ago
relnote-firefox: --- → ?
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)

Updated

6 years ago
Depends on: 908862
(Assignee)

Comment 18

6 years ago
I spun off bug 908862 to deal with this.

Updated

6 years ago
Depends on: 909111

Updated

6 years ago
Depends on: 910897
Does this needs QA validation given the automation coverage?
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.
relnote-firefox: ? → +

Updated

6 years ago
relnote-firefox: + → ?
relnote-firefox: ? → 26+

Updated

6 years ago
Depends on: 919513

Updated

5 years ago
Depends on: 986925

Updated

5 years ago
Depends on: 986947

Updated

5 years ago
Depends on: 1088848
Depends on: 1166955

Updated

3 years ago
Depends on: 1270280
You need to log in before you can comment on or make changes to this bug.