Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Description • 11 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 1 • 11 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

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 2 • 11 years ago

Note to self: call NotifyBytesConsumed() in DirectShowReader::DecodeAudioData().

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 3 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 4 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 5 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 6 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 7 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=9fdad02c2e80

	Gregory Szorc [:gps]
	Comment 8 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 9 • 11 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

	Randell Jesup [:jesup] (needinfo me)
	Comment 10 • 11 years ago

mfind '\$(topsrcdir' | wc -> 250 uses
mfind '\$(TOPSRCDIR' | wc -> 52 uses

and 87 in moz.build files, vs none for TOPSRCDIR

	Gregory Szorc [:gps]
	Comment 11 • 11 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|.

	Paul Adenot (:padenot)
	Comment 12 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 13 • 11 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.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 14 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=6834604aaf0a

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 15 • 11 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

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 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

	Trevor Rowbotham [:rowbot]
	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

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 18 • 11 years ago

I spun off bug 908862 to deal with this.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 19 • 11 years ago

Does this needs QA validation given the automation coverage?

	Asa Dotzler [:asa]
	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.