Closed Bug 799315 Opened 12 years ago Closed 12 years ago

Windows Media Foundation backend for media playback

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
All
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: cpearce, Assigned: cpearce)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Patch 1 v1: MP4 tests
96.35 KB, patch
padenot
: review+
Details | Diff | Splinter Review
Patch 2 v1: WMFDecoder backend
85.99 KB, patch
padenot
: review+
Details | Diff | Splinter Review 
We can use Windows Media Foundation to decode MP3 and H.264/AAC on Windows Vista and later.
Blocks: 799318
Depends on: 815748
https://tbpl.mozilla.org/?tree=Try&rev=a598bc9fbe8f
You need to use LoadLibrary/GetProcAddress or delayload linker switch for MF functions. Otherwise the binary will not run on WinXP at all.
Thanks, I was wondering what was causing that.
yo chris whats up with this patch man. hurry the **** up with that feature, we're all sick from using **** flash for videos with firefox, while all other major browsers already have native support.

peace
peter
(In reply to Peter Niess from comment #3)
> yo chris whats up with this patch man. hurry the **** up with that feature,
> we're all sick from using **** flash for videos with firefox, while all
> other major browsers already have native support.
> 
> peace
> peter

Peter, this comment doesn't really meet our etiquette guidelines <https://bugzilla.mozilla.org/page.cgi?id=etiquette.html>.  Please refrain from adding similar comments in the future.  Thanks!
Depends on: 820208
Adds MP3 and M4A audio test files to media tests. We can already use gizmo.mp4 as an MP4 video test.

I also swapped in gizmo.mp4 instead of short.mp4 in the sniffer tests. The video track in short.mp4 is identified as "MPEG-4 part 2 Simple Profile" by Windows Media Foundation and since that's not H.264 I don't think we should play it. IE and Chrome don't play that file. The sniffer tests attempt to load the videos it's sniffing, and the Windows Media Foundation backend will refuse to play short.mp4 since it's not h.264, and thus the sniffer test will fail if we don't make this change.
Attachment #691734 - Flags: review?(paul)
No good deed goes unpunished around here Paul, congratulations! You've been promoted to Media code reviewer! ;)

* Adds WMFDecoder/Reader to interface with Windows Media Foundation for H.644/AAC/MP3 playback.
* Streams encoded with codecs other than H.644/AAC/MP3 are not played.
* Includes a canplayType test.
* Data from Necko is passed through to WMF by wrapping MediaResource in an IMFByteStream implementation.
* Doesn't use DXVA2 hardware accelerated decoding yet, that'll come later.
* Decodes video frames into system memory. I'll add decoding into Direct3D surfaces in another bug/patch in the not-too-distant future.
* Doesn't decode metadata, i.e. HTMLMediaElement.mozMetadata, I'll add this in another bug.
* Calls WMF functions via loading the appropriate DLL into our process and asking for the function.
* Passed mochitests (finally!)
Windows only: https://tbpl.mozilla.org/?tree=Try&rev=deb51e0689d8
try -a: https://tbpl.mozilla.org/?tree=Try&rev=34bd2047f888
* Uses simple byte/duration estimation for GetBuffered() implementation, Windows Media Foundation seems to only have an API to map byte offsets to times available on Windows 8. :(

For those interested, you can download builds with WMF support to experiment with here:

http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-deb51e0689d8/try-win32/
Attachment #691740 - Flags: review?(paul)
Comment on attachment 691740 [details] [diff] [review]
Patch 2 v1: WMFDecoder backend

Bas, could you also take a look at the this patch? There's heavy use of Windows APIs, in particular in the WMF* files. It would be good to have an experienced Windows guy go over them.
Attachment #691740 - Flags: review?(bas)
Is Mozilla gonna add H.264 support for Linux operating systems? 

Ubuntu provides H.264 support, and I am really sick and tired of not being able to use H.264 in Firefox even though Ubuntu OS supports it. I keep having to use Chrome and would love to be able to use Firefox exclusively.

Hopefully you guys haven't forgotten about Linux OSs that have licenses for H.264 among other proprietary codecs. also, linux people regularly purchase codec packs like available from Fluendo: http://www.fluendo.com/shop/product/complete-set-of-playback-plugins/
Jason, the code to support H.264 on Linux (that uses gstreamer behind the scenes) is already written (content/media/gstreamer), but behind a compile time flag (--enable-gstreamer) for various reason. If you are willing to compile your own Firefox, you can use it, I've tested it a few months ago, and it was working nicely, although there is no guarantee it still works as per now (for example, [2] says it does not work to much, but well, everything is fixable :-).

The status of this feature can be tracked in bug 794282 [1]. I can't give you a timeline, though.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=794282
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=817015
awesome ! so it will be added to aurora/beta soon ?
Will GStreamer support be added to aurora/beta? Don't know when. We need to figure out a binary compatibility strategy. Best to take up that cause in a post to mozilla.dev.media or in the bugs Paul mentioned.
Comment on attachment 691740 [details] [diff] [review]
Patch 2 v1: WMFDecoder backend

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

I need to look a little more in depth, but here's my comments so far.

::: content/media/wmf/WMFByteStream.cpp
@@ +7,5 @@
> +#include "WMF.h"
> +
> +#include "Unknwn.h"
> +#include <ole2.h>
> + 

nit: whitespace

@@ +89,5 @@
> +WMFByteStream::QueryInterface(REFIID aIId, void **aInterface)
> +{
> +  LOG("WMFByteStream::QueryInterface %s", GetGUIDName(aIId).get());
> +
> +  if (aIId == __uuidof(IMFByteStream)) {

I think this won't compile on MinGW as GCC doesn't support __uuidof, you'd need to use IID_*

@@ +114,5 @@
> +STDMETHODIMP
> +WMFByteStream::AsyncReadRequestState::QueryInterface(REFIID aIId, void **aInterface)
> +{
> +  LOG("WMFByteStream::AsyncReadRequestState::QueryInterface %s", GetGUIDName(aIId).get());
> +  

nit: whitespace

@@ +195,5 @@
> +      // Let SourceReader know the read failed.
> +      callerResult->SetStatus(E_FAIL);
> +      wmf::MFInvokeCallback(callerResult);
> +      LOG("WMFByteStream::Invoke() seek to read offset failed, aborting read");
> +      return S_OK;        

nit: whitespace

@@ +220,5 @@
> +  // Record the number of bytes read, so the caller can retrieve
> +  // it later.
> +  requestState->mBytesRead = NS_SUCCEEDED(rv) ? totalBytesRead : 0;
> +  callerResult->SetStatus(S_OK);
> +  

nit: whitespace

@@ +358,5 @@
> +    mOffset = aSeekOffset;
> +  } else {
> +    mOffset += aSeekOffset;
> +  }
> +  

nit: whitespace

::: content/media/wmf/WMFByteStream.h
@@ +67,5 @@
> +  STDMETHODIMP GetParameters(DWORD*, DWORD*);
> +  STDMETHODIMP Invoke(IMFAsyncResult* aResult);
> +
> +private:
> +  

nit: whitespace

@@ +99,5 @@
> +    // IUnknown ref counting.
> +    nsAutoRefCnt mRefCnt;
> +    NS_DECL_OWNINGTHREAD;
> +  };
> +  

nit: whitespace

::: content/media/wmf/WMFReader.cpp
@@ +256,5 @@
> +  if (FAILED(hr)) {
> +    LOG("Failed to configured video output for MFVideoFormat_YV12");
> +    return;
> +  }
> +  

nit: whitespace

@@ +335,5 @@
> +  mAudioRate = MFGetAttributeUINT32(mediaType, MF_MT_AUDIO_SAMPLES_PER_SECOND, 0);
> +  mAudioChannels = MFGetAttributeUINT32(mediaType, MF_MT_AUDIO_NUM_CHANNELS, 0);
> +  mAudioBytesPerSample = MFGetAttributeUINT32(mediaType, MF_MT_AUDIO_BITS_PER_SAMPLE, 16) / 8;
> +
> +  mInfo.mAudioChannels = mAudioChannels; 

nit: whitespace, and 2 more right below here.

@@ +421,5 @@
> +  hr = buffer->Lock(&data, &maxLength, &currentLength);
> +  NS_ENSURE_TRUE(SUCCEEDED(hr), false);
> +
> +  uint32_t numFrames = currentLength / mAudioBytesPerSample / mAudioChannels;
> +  NS_ASSERTION(numFrames * mAudioChannels * sizeof(AudioDataValue) == currentLength,

Unless again checking for some sort of overflow (I don't see how :)). I think asserting sizeof(AudioDataValue) == mAudioBytesPerSample seems to make more sense to me. It makes it more obvious what is being checked. The rest is guaranteed.

@@ +540,5 @@
> +  // to the row heights to ensure the Y'CbCr planes are referenced properly.
> +  uint32_t padding = 0;
> +  if (mVideoHeight % 16 != 0) {
> +    padding = 16 - (mVideoHeight % 16);
> +    NS_ASSERTION(((mVideoHeight + padding) % 16) == 0, "Padding calculation is wrong");

Considering this assertion is right below a statements which makes it a mathematical impossibility to fail unless the compiler is broken I'm not sure how useful I feel it is. Unless we're trying to detect from overflow situation.

@@ +554,5 @@
> +  b.mPlanes[1].mHeight = halfHeight;
> +  b.mPlanes[1].mWidth = halfPitch;
> +  b.mPlanes[1].mOffset = 0;
> +  b.mPlanes[1].mSkip = 0;
> + 

nit: whitespace

@@ +637,5 @@
> +    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +    durationUs = mDecoder->GetMediaDuration();
> +  }
> +  GetEstimatedBufferedTimeRanges(stream, durationUs, aBuffered);
> +  return NS_OK; 

nit: whitespace

::: content/media/wmf/WMFUtils.cpp
@@ +213,5 @@
> +static WMFModule sDLLs[] = {
> +  { "mfplat.dll", NULL },
> +  { "mfreadwrite.dll", NULL },
> +  { "propsys.dll", NULL },
> +  { "mf.dll", NULL },

Isn't our policy to load DLL's with their full paths? To avoid the possibility of DLL injection?

::: layout/build/nsLayoutStatics.cpp
@@ +352,2 @@
>    nsCORSListenerProxy::Shutdown();
>    

nit: whitespace
(In reply to Bas Schouten (:bas.schouten) from comment #12)
> ::: content/media/wmf/WMFUtils.cpp
> @@ +213,5 @@
> > +static WMFModule sDLLs[] = {
> > +  { "mfplat.dll", NULL },
> > +  { "mfreadwrite.dll", NULL },
> > +  { "propsys.dll", NULL },
> > +  { "mf.dll", NULL },
> 
> Isn't our policy to load DLL's with their full paths? To avoid the
> possibility of DLL injection?

We don't,
https://mxr.mozilla.org/mozilla-central/search?string=LoadLibrary
but we need to check Windows version (>=Vista here) to prevent DLL injection.
(In reply to comment #13)
> (In reply to Bas Schouten (:bas.schouten) from comment #12)
> > ::: content/media/wmf/WMFUtils.cpp
> > @@ +213,5 @@
> > > +static WMFModule sDLLs[] = {
> > > +  { "mfplat.dll", NULL },
> > > +  { "mfreadwrite.dll", NULL },
> > > +  { "propsys.dll", NULL },
> > > +  { "mf.dll", NULL },
> > 
> > Isn't our policy to load DLL's with their full paths? To avoid the
> > possibility of DLL injection?
> 
> We don't,
> https://mxr.mozilla.org/mozilla-central/search?string=LoadLibrary
> but we need to check Windows version (>=Vista here) to prevent DLL injection.

Please use the full path if that is an option.
Comment on attachment 691740 [details] [diff] [review]
Patch 2 v1: WMFDecoder backend

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

r+ with the remarks addressed and/or questions answered.

I'm concerned by the fact that Microsoft made -n and -kn versions of Windows {Vista, 7, 8} that do no have the whole Media Foundation things, mainly in Europe, for legal reasons [1] (those version lack DLLs, especially the one we need here, and other software package). Do we have plan to have some kind of user facing feature that could advise users on what to do to get the required software package, à la Flash (the required download is free [2])? Otherwise, the DLL loading will fail, and users will complain all over the internet that we shipped a feature that does not work (i.e. mp3 and h.264 playback without flash) and say they will switch to Chrome. We already have an easy mean to display a message inside a <video> tag in case of failure, we could use this.

Also, I remarked Audio().canPlayType("audio/mpeg") returns "maybe", which is okay. But Audio().canPlayType("audio/mpeg; codecs=mp3") returns "" (empty string). This should be fixed, because, as the spec say, this is not "a codec we know we can't render", and this is what the sites I used to test this patch were using to determine if the browser has mp3 support. FWIW, it makes us not pass the areweplayingyet mp3 support test.

[1]: https://en.wikipedia.org/wiki/Windows_7_editions#Special-purpose_editions
[2]: http://support.microsoft.com/kb/2703761

::: content/media/test/can_play_type_mpeg.js
@@ +22,5 @@
> +  check("video/mp4; codecs=\"avc1.64001E\"", "probably");
> +  check("video/mp4; codecs=\"avc1.64001F\"", "probably");
> +  
> +  check("audio/mp4; codecs=mp4a.40.2", "probably");
> +}

Add double quotes around the codec arguments for "audio/mp4 ..." for consistency.

Quite a few trailing spaces in this file.

Also, you may want to add a test to test mp3 support:

check("audio/mp4; codecs=\"mp3\"", "probably");

if we decide this should be supported.

::: content/media/test/test_can_play_type_mpeg.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=566245

s/566245/799315/

::: content/media/wmf/Makefile.in
@@ +30,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +LOCAL_INCLUDES = \
> +		$(MOZ_LIBVPX_CFLAGS) \
> +		$(NULL)

You don't need this, I guess.

::: content/media/wmf/WMFByteStream.cpp
@@ +39,5 @@
> +    mOffset(0),
> +    mResource(aResource)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Must be on main thread.");
> +  NS_ASSERTION(mResource, "Must have a valid media resource");  

nit, trailing space.

::: content/media/wmf/WMFDecoder.cpp
@@ +27,5 @@
> +
> +  // MP3 specifies no codecs; implicit specification of MP3.
> +  static char const *const mp3AudioCodecs[] = {
> +    nullptr
> +  };

Not sure what you mean by that. In any case, if we don't allow "audio/mpeg; codecs=mp3", we are going to be yelled at.

@@ +63,5 @@
> +  if (aType.EqualsASCII("video/mp4")) {
> +    if (aCodecList) {
> +      *aCodecList = H264Codecs;
> +    }
> +    return true;    

nit, trailing spaces;

::: content/media/wmf/WMFDecoder.h
@@ +15,5 @@
> +// codecs.
> +class WMFDecoder : public MediaDecoder
> +{
> +public:
> +  

nit: trailing space.

::: content/media/wmf/WMFReader.cpp
@@ +279,5 @@
> +                                 &aspectDenom))) {
> +    NS_WARNING("WMF video decoder failed to get pixel aspect ratio!");
> +    return;
> +  }
> +  LOG("Video apsect ratio %u x %u", aspectNum, aspectDenom);

s/apsect/aspect/

@@ +373,5 @@
> +
> +  *aInfo = mInfo;
> +  *aTags = nullptr;
> +  // aTags can be retrieved using techniques like used here:
> +  // http://blogs.msdn.com/b/mf/archive/2010/01/12/mfmediapropdump.aspx

Has a bug about this been filed? Maybe it could even be a mentored bug, it does not seem too hairy

@@ +439,5 @@
> +                                 pcmSamples.forget(),
> +                                 mAudioChannels));
> +
> +  #ifdef LOG_SAMPLE_DECODE
> +  LOG("Decoded audio sample! timestamp=%lld duration=%lld currentLength=%d",

%lld is only supported after VS2005, not sure what is currently required to build Firefox.
|currentLength| is a DWORD, and should be formatted as an unsigned.

@@ +509,5 @@
> +  }
> +
> +  // Try and use the IMF2DBuffer interface if available, otherwise fallback
> +  // to the IMFMediaBuffer interface. Apparently IMF2DBuffer is more efficient,
> +  // but only some systems (Windows 8?) support it.

[1] says Windows Vista. My understanding is that it offers methods more suited to 2d data (access to the stride, for example), and may be faster because it can avoid making a copy in certain case.

[1]: http://msdn.microsoft.com/en-us/library/windows/desktop/ms699894%28v=vs.85%29.aspx

@@ +512,5 @@
> +  // to the IMFMediaBuffer interface. Apparently IMF2DBuffer is more efficient,
> +  // but only some systems (Windows 8?) support it.
> +  BYTE* data = nullptr;
> +  LONG pitch = 0;
> +  uint32_t stride = 0;

This variable seems unused. Also, we seem to prefer the term "stride" over "pitch" in content/media/, so we might as well use "stride" here as well for consistency and to avoid confusion, because pitch as an other meaning when it comes to audio. MS says the two terms are equivalent when it comes to their API [1].

[1]: http://msdn.microsoft.com/en-us/library/windows/desktop/aa473780%28v=vs.85%29.aspx : "Stride is also called pitch"

@@ +513,5 @@
> +  // but only some systems (Windows 8?) support it.
> +  BYTE* data = nullptr;
> +  LONG pitch = 0;
> +  uint32_t stride = 0;
> +  IMF2DBufferPtr twoDBuffer = buffer;

The documentation says you should QueryInterface a buffer to a 2DBuffer. Then again, I'm certainly not the most knowledgeable person when it comes to MS COM. From what I read, they do not overload |operator=()|, so in fact you don't really know if you have this interface with the code below. Or maybe there is some COM magic I'm not aware of.

@@ +588,5 @@
> +  mVideoQueue.Push(v);
> +
> +  #ifdef LOG_SAMPLE_DECODE
> +  LOG("Decoded video sample timestamp=%lld duration=%lld 2dBuffer=%p stride=%d width=%d height=%d flags=%u",
> +      timestamp, duration, twoDBuffer.GetInterfacePtr(), pitch, mVideoWidth, mVideoHeight, flags);

Maybe it would be good to print |buffer| instead of |twoDBuffer| when we fallback on the normal buffer.

Also, mVideoWidth and mVideoHeight are unsigned, pitch is a long (or unsigned if you make the change I propose above). It probably does not matter too much considering the range those variable are expected to be in, though.

::: content/media/wmf/WMFUtils.cpp
@@ +308,5 @@
> +  ENSURE_FUNCTION_PTR(MFAllocateWorkQueue, Mfplat.dll)
> +  return (MFAllocateWorkQueuePtr)(aOutWorkQueueId);
> +}
> +
> +HRESULT 

nit, trailing space.

@@ +379,5 @@
> +                   IMFAttributes **aOutAttributes)
> +{
> +  DECL_FUNCTION_PTR(MFTGetInfo, CLSID, LPWSTR*, MFT_REGISTER_TYPE_INFO**, UINT32*, MFT_REGISTER_TYPE_INFO**, UINT32*, IMFAttributes**);
> +  ENSURE_FUNCTION_PTR(MFTGetInfo, Mfplat.dll)
> +  return (MFTGetInfoPtr)(aClsidMFT,

Indentation seems off, here.

::: layout/build/nsLayoutStatics.cpp
@@ +340,2 @@
>  #ifdef MOZ_MEDIA_PLUGINS
>    MediaPluginHost::Shutdown();  

nit: whitespace.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +409,3 @@
>    { VIDEO_MP4, "mp4" },
> +  { AUDIO_MP4, "m4a" },
> +  { AUDIO_MP4, "mp3" },

MP3 in the last one here, right? That would make us comply with RFC 3003.
Attachment #691740 - Flags: review?(paul) → review+
Comment on attachment 691734 [details] [diff] [review]
Patch 1 v1: MP4 tests

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

::: content/media/test/manifest.js
@@ +309,5 @@
>    { name:"r11025_s16_c1.wav", type:"audio/x-wav", duration:1.0 },
>    { name:"sound.ogg", type:"audio/ogg" },
> +  { name:"owl.mp3", type:"audio/mpeg", duration:3.29 },
> +  { name:"small-shot.m4a", type:"audio/mp4", duration:0.29 },
> +  

nit: whitespace.
Attachment #691734 - Flags: review?(paul) → review+
Thanks Paul!

(In reply to Paul ADENOT (:padenot) from comment #15)
> Comment on attachment 691740 [details] [diff] [review]
> Patch 2 v1: WMFDecoder backend
> 
> Review of attachment 691740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the remarks addressed and/or questions answered.
> 
> I'm concerned by the fact that Microsoft made -n and -kn versions of Windows
> {Vista, 7, 8} that do no have the whole Media Foundation things, mainly in
> Europe, for legal reasons [1] (those version lack DLLs, especially the one
> we need here, and other software package).

That's why we report that we can't play a type if LoadDlls() fails in WMFDecoder::GetSupportedCodecs(), Vista systems without the Platform Update (and I'd assume -N and -KN variants without WMF) don't have the DLLs, so canPlayType() won't report that we can play those codecs. I've tested this on Vista, but not on -N/-KN variants.


> Do we have plan to have some kind
> of user facing feature that could advise users on what to do to get the
> required software package

That's a good idea.

There's documentation on MSDN regarding how to prompt users to install the Platform Update on Vista to get the WMF DLLs, we could look at doing this. Maybe some similar documentation exists for -N and -KN.



> Also, I remarked Audio().canPlayType("audio/mpeg") returns "maybe", which is
> okay. But Audio().canPlayType("audio/mpeg; codecs=mp3") returns "" (empty
> string). This should be fixed, because, as the spec say, this is not "a
> codec we know we can't render", and this is what the sites I used to test
> this patch were using to determine if the browser has mp3 support. FWIW, it
> makes us not pass the areweplayingyet mp3 support test.

I thought this at first too (bug 803342), but it turns out there are no specified codecs for "audio/mpeg" [ http://wiki.whatwg.org/wiki/Video_type_parameters#MPEG ], so including any codecs with "audio/mpeg" technically should result in a negative canPlayType() result. 






> ::: content/media/test/can_play_type_mpeg.js
> @@ +22,5 @@
> > +  check("video/mp4; codecs=\"avc1.64001E\"", "probably");
> > +  check("video/mp4; codecs=\"avc1.64001F\"", "probably");
> > +  
> > +  check("audio/mp4; codecs=mp4a.40.2", "probably");
> > +}
> 
> Add double quotes around the codec arguments for "audio/mp4 ..." for
> consistency.

Hmm, we should support both with and without double quotes, so I'll add a doubled quoted variant.



> ::: content/media/wmf/WMFDecoder.cpp
> @@ +27,5 @@
> > +
> > +  // MP3 specifies no codecs; implicit specification of MP3.
> > +  static char const *const mp3AudioCodecs[] = {
> > +    nullptr
> > +  };
> 
> Not sure what you mean by that. In any case, if we don't allow "audio/mpeg;
> codecs=mp3", we are going to be yelled at.

I'll improve the comment here.


> @@ +373,5 @@
> > +
> > +  *aInfo = mInfo;
> > +  *aTags = nullptr;
> > +  // aTags can be retrieved using techniques like used here:
> > +  // http://blogs.msdn.com/b/mf/archive/2010/01/12/mfmediapropdump.aspx
> 
> Has a bug about this been filed? Maybe it could even be a mentored bug, it
> does not seem too hairy

I've been working on a patch in between try runs.



> 
> @@ +439,5 @@
> > +                                 pcmSamples.forget(),
> > +                                 mAudioChannels));
> > +
> > +  #ifdef LOG_SAMPLE_DECODE
> > +  LOG("Decoded audio sample! timestamp=%lld duration=%lld currentLength=%d",
> 
> %lld is only supported after VS2005, not sure what is currently required to
> build Firefox.
> |currentLength| is a DWORD, and should be formatted as an unsigned.

VS2005 is our mimimum offical supported version of VS. 
https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Windows_Prerequisites
We also use %lld all over. ;)


> @@ +509,5 @@
> > +  }
> > +
> > +  // Try and use the IMF2DBuffer interface if available, otherwise fallback
> > +  // to the IMFMediaBuffer interface. Apparently IMF2DBuffer is more efficient,
> > +  // but only some systems (Windows 8?) support it.
> 
> [1] says Windows Vista. My understanding is that it offers methods more
> suited to 2d data (access to the stride, for example), and may be faster
> because it can avoid making a copy in certain case.

In practice I only observed the QI to IMF2DBuffer succeed on my Windows 8 machine. It could be graphics driver/hardware dependent of course.

 
> @@ +513,5 @@
> > +  // but only some systems (Windows 8?) support it.
> > +  BYTE* data = nullptr;
> > +  LONG pitch = 0;
> > +  uint32_t stride = 0;
> > +  IMF2DBufferPtr twoDBuffer = buffer;
> 
> The documentation says you should QueryInterface a buffer to a 2DBuffer.
> Then again, I'm certainly not the most knowledgeable person when it comes to
> MS COM. From what I read, they do not overload |operator=()|, so in fact you
> don't really know if you have this interface with the code below. Or maybe
> there is some COM magic I'm not aware of.

MS's smart pointer class overloads operator= to QI (that's why I can null check on line 518).


> 
> @@ +588,5 @@
> > +  mVideoQueue.Push(v);
> > +
> > +  #ifdef LOG_SAMPLE_DECODE
> > +  LOG("Decoded video sample timestamp=%lld duration=%lld 2dBuffer=%p stride=%d width=%d height=%d flags=%u",
> > +      timestamp, duration, twoDBuffer.GetInterfacePtr(), pitch, mVideoWidth, mVideoHeight, flags);
> 
> Maybe it would be good to print |buffer| instead of |twoDBuffer| when we
> fallback on the normal buffer.

Right, printing out twoDBuffer was me checking if the QI of operator= worked. ;)



> ::: uriloader/exthandler/nsExternalHelperAppService.cpp
> @@ +409,3 @@
> >    { VIDEO_MP4, "mp4" },
> > +  { AUDIO_MP4, "m4a" },
> > +  { AUDIO_MP4, "mp3" },
> 
> MP3 in the last one here, right? That would make us comply with RFC 3003.

Ah yes, thanks!
(In reply to Masatoshi Kimura [:emk] from comment #13)
> (In reply to Bas Schouten (:bas.schouten) from comment #12)
> > ::: content/media/wmf/WMFUtils.cpp
> > @@ +213,5 @@
> > > +static WMFModule sDLLs[] = {
> > > +  { "mfplat.dll", NULL },
> > > +  { "mfreadwrite.dll", NULL },
> > > +  { "propsys.dll", NULL },
> > > +  { "mf.dll", NULL },
> > 
> > Isn't our policy to load DLL's with their full paths? To avoid the
> > possibility of DLL injection?
> 
> We don't,
> https://mxr.mozilla.org/mozilla-central/search?string=LoadLibrary
> but we need to check Windows version (>=Vista here) to prevent DLL injection.

Thanks for your feedback.

I will load with the full path to DLLs, and refuse to attempt to load the DLLs on Windows version < 6.1 (Windows 7).

I haven't had time to check that behaviour is correct on Vista with the Platform Update installed; my old Vista laptop overheated and bricked while installing the Platform Update, so I'll have to come back and test behaviour with and without the Platform Update installed in the new year.
I discussed this with Bas, and he said it's ok to land this pref'd off.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f71d8e13583
https://hg.mozilla.org/integration/mozilla-inbound/rev/a75e9af08e9c

Once this merges to mozilla-central, it can be used in Nightly builds by pref'ing on by setting "media.windows-media-foundation.enabled" to true in about:config.

I opted to land this pref'd off because I'm still seeing some intermittent shtudown hangs in unit tests, and we can also get a shutdown hang if we close the MediaResource before metadata has finished loading. I think we can resolve this by switching to use the SourceReader asynchronously, I'll do that in another bug in the new year.
Attachment #691740 - Flags: review?(bas)
https://hg.mozilla.org/mozilla-central/rev/a75e9af08e9c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
static WMFModule sDLLs[] = {
{ "C:\\Windows\\system32\\mfplat.dll", NULL },
{ "C:\\Windows\\system32\\mfreadwrite.dll", NULL },
{ "C:\\Windows\\system32\\propsys.dll", NULL },
{ "C:\\Windows\\system32\\mf.dll", NULL },
};

Will the patch only for x86 Win7?
x64 Win7 will be C:\\Windows\\syswow64
And if we implement --enable-gstreamer on Windows successfully, will the patch has higher priority than --enable-gstreamer?
(In reply to xunxun from comment #21)
> static WMFModule sDLLs[] = {
> { "C:\\Windows\\system32\\mfplat.dll", NULL },
> { "C:\\Windows\\system32\\mfreadwrite.dll", NULL },
> { "C:\\Windows\\system32\\propsys.dll", NULL },
> { "C:\\Windows\\system32\\mf.dll", NULL },
> };
> 
> Will the patch only for x86 Win7?
> x64 Win7 will be C:\\Windows\\syswow64

I guess the patch won't work in x64 Windows builds; but we don't officially support them anyway. :(

We could easily add a #ifdef to change the path to syswow64 for x64. I think there may be an Windows API to get the system dir too.

(In reply to xunxun from comment #22)
> And if we implement --enable-gstreamer on Windows successfully, will the
> patch has higher priority than --enable-gstreamer?

If that happens, we'll decide then.
(In reply to Chris Pearce (:cpearce, away 20 Dec until 10 Jan) from comment #23)
> (In reply to xunxun from comment #21)
> > static WMFModule sDLLs[] = {
> > { "C:\\Windows\\system32\\mfplat.dll", NULL },
> > { "C:\\Windows\\system32\\mfreadwrite.dll", NULL },
> > { "C:\\Windows\\system32\\propsys.dll", NULL },
> > { "C:\\Windows\\system32\\mf.dll", NULL },
> > };
> > 
> > Will the patch only for x86 Win7?
> > x64 Win7 will be C:\\Windows\\syswow64
> 
> I guess the patch won't work in x64 Windows builds; but we don't officially
> support them anyway. :(
> 
> We could easily add a #ifdef to change the path to syswow64 for x64. I think
> there may be an Windows API to get the system dir too.

WOW64 folder redirect would gracefully handle this.
But I think you should use GetSystemDirectory() and combine the result with DLL names. If the system was upgraded from WinXP, the system directory may not be under C: drive.
There are plenty of systems where the windows system drive is not C: - you shouldn't hard-code the paths like this! 
Think of dual-boot systems, for one. 

Windows installations when upgraded/repaired with multiple physical drives and/or partitions in it can also have an arbitrary system drive letter (which cannot be changed after the fact). This would make this implementation incompatible.
I have windows 7 64bit and this patch in latest Nightly works very well.
Thank you Chris
(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> (In reply to comment #13)
> > (In reply to Bas Schouten (:bas.schouten) from comment #12)
> > > ::: content/media/wmf/WMFUtils.cpp
> > > @@ +213,5 @@
> > > > +static WMFModule sDLLs[] = {
> > > > +  { "mfplat.dll", NULL },
> > > > +  { "mfreadwrite.dll", NULL },
> > > > +  { "propsys.dll", NULL },
> > > > +  { "mf.dll", NULL },
> > > 
> > > Isn't our policy to load DLL's with their full paths? To avoid the
> > > possibility of DLL injection?
> > 
> > We don't,
> > https://mxr.mozilla.org/mozilla-central/search?string=LoadLibrary
> > but we need to check Windows version (>=Vista here) to prevent DLL injection.
> 
> Please use the full path if that is an option.

->

> static WMFModule sDLLs[] = {
> { "C:\\Windows\\system32\\mfplat.dll", NULL },
> { "C:\\Windows\\system32\\mfreadwrite.dll", NULL },
> { "C:\\Windows\\system32\\propsys.dll", NULL },
> { "C:\\Windows\\system32\\mf.dll", NULL },


I think there must have been some serious confusion here - we should not be hardcoding c:\\windows\sustem32 paths on our code. :) This is totally not going to work on a huge number of systems.
Depends on: 823021
To add to what Jim said in comment #27, there seems to be some confusion about the SysWow64 folder. The 32-bit Windows System dlls and executables (among other 32-bit only files) are stored in the SysWow64 folder on 64-bit Windows installations. This has nothing to do with whether the Firefox build is 32-bit or 64-bit.
Additionally, GetSystemDirectory() exists for compatibility purposes only. Consider SHGetKnownFolderPath() which was introduced in Windows Vista.

http://msdn.microsoft.com/en-us/library/windows/desktop/ms724373%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/bb762188%28v=vs.85%29.aspx
Depends on: 823168
Depends on: 823265
No longer depends on: 823265
(In reply to Manoj from comment #28)
> To add to what Jim said in comment #27, there seems to be some confusion
> about the SysWow64 folder. The 32-bit Windows System dlls and executables
> (among other 32-bit only files) are stored in the SysWow64 folder on 64-bit
> Windows installations. This has nothing to do with whether the Firefox build
> is 32-bit or 64-bit.

64-bits Firefox on 64-bits Windows - 64-bits DLLs are needed that are under system32.
32-bits Firefox on 64-bits Windows - 32-bits DLLs are needed, but WOW64 will redirect system32 to SysWOW64 for 32-bits apps.
32-bits Firefox on 32-bits Windows - 32-bits DLLs are needed that are under system32.

So "system32" would just work in all cases (modulo the drive letter problem).

(In reply to Manoj from comment #29)
> Additionally, GetSystemDirectory() exists for compatibility purposes only.
> Consider SHGetKnownFolderPath() which was introduced in Windows Vista.
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms724373%28v=vs.
> 85%29.aspx
> http://msdn.microsoft.com/en-us/library/windows/desktop/bb762188%28v=vs.
> 85%29.aspx

Yes, we need GetSystemDirectory() for compatibility with Windows XP.
To use SHGetKnownFolderPath(), we need to link shell32.dll dynamically. To load shell32.dll, we need to get the path of system directory :)
(Actually it doesn't a matter because shell32.dll is one of "Known" DLLs.)
Don't we need to expose this in about:support?
(In reply to Masatoshi Kimura [:emk] from comment #30)
> So "system32" would just work in all cases (modulo the drive letter problem).
> 
It might work, but it's a hack. Since there is a clearly prescribed way of doing this, why not follow the recommended approach?

> Yes, we need GetSystemDirectory() for compatibility with Windows XP.

No we do not need compatibility with Windows XP. This feature is for Windows released post Windows Vista (see comment #1) so SHGetKnownFolderPath is the way to go.
(In reply to Manoj from comment #32)
> (In reply to Masatoshi Kimura [:emk] from comment #30)
> > So "system32" would just work in all cases (modulo the drive letter problem).
> > 
> It might work, but it's a hack. Since there is a clearly prescribed way of
> doing this, why not follow the recommended approach?

I didn't say we shouldn't use a recommended way. I just express I didn't confuse about the WOW64 situation.

> > Yes, we need GetSystemDirectory() for compatibility with Windows XP.
> 
> No we do not need compatibility with Windows XP. This feature is for Windows
> released post Windows Vista (see comment #1) so SHGetKnownFolderPath is the
> way to go.

Wrong. We need to run the binary on WinXP. If we link the SHGetKnownFolderPath statically, the binary will not even start on WinXP. It's the very reason mf functions are linked dynamically (see comment #1). If we don't have to consider WinXP, we don't have to be bothered with loading mf DLLs in the first place.
(In reply to Alexandre Folle de Menezes from comment #31)
> Don't we need to expose this in about:support?

It's already exposed if the value is set true.
(In reply to Masatoshi Kimura [:emk] from comment #33)
> Wrong. We need to run the binary on WinXP. If we link the
> SHGetKnownFolderPath statically, the binary will not even start on WinXP.
> It's the very reason mf functions are linked dynamically (see comment #1).
> If we don't have to consider WinXP, we don't have to be bothered with
> loading mf DLLs in the first place.

I see. At the very least then, please consider the use of GetSystemDirectory() or SHGetFolderPath() which will work on XP.
Can't you just use %windir%\system32\whatever.dll?
Environment variables are not automatically expanded for C++ program. It's more complex and less reliable than GetSystemDirectory().
Depends on: 823988
Depends on: 824234
Depends on: 823646
Is this supposed to be hardware accelerated? At least I don't see the code enabling CODECAPI_AVDecVideoAcceleration_H264 (see http://msdn.microsoft.com/en-us/library/windows/desktop/dd797815%28v=vs.85%29.aspx).
(In reply to JK from comment #38)
> Is this supposed to be hardware accelerated?

According to comment 6, no, that will be done later.
No longer depends on: 823021
Depends on: 823924
Depends on: 825153
Depends on: 825446
Is it possible to use Media Foundation on Windows Vista with Platform Update at the moment? I switched the line media.windows-media-foundation.enabled in the latest nightly, but still there is no h264 decoding available.
Depends on: 824877
Depends on: 830171
Depends on: 830172
Depends on: 832142
Depends on: 832143
Depends on: 832680
Blocks: 832754
No longer depends on: 832680
Depends on: 836927
Hi

I am trying live streaming with H.264 in an MPEG-TS container, but apparently MPEG-TS is not supported by this change? Error log says:

Timestamp: 01/02/2013 13:46:26
Warning: HTTP “Content-Type” of “video/mp2t” is not supported. Load of media resource http://127.0.0.1:8192/e5a12c7ad2d8fab33c699d1e198d66f79fa610c3.-1 failed.

This is on Nightly 21.0a1 (2013-01-31). What container formats should be supported now?

Thanks,
       Arno
(In reply to Arno Bakker from comment #42)
> Hi
> 
> I am trying live streaming with H.264 in an MPEG-TS container, but
> apparently MPEG-TS is not supported by this change? Error log says:
> 
> Timestamp: 01/02/2013 13:46:26
> Warning: HTTP “Content-Type” of “video/mp2t” is not supported. Load of media
> resource http://127.0.0.1:8192/e5a12c7ad2d8fab33c699d1e198d66f79fa610c3.-1
> failed.
> 
> This is on Nightly 21.0a1 (2013-01-31). What container formats should be
> supported now?
> 
> Thanks,
>        Arno

MPEG-TS 's Internet media type is video/MP2T.

At present, nightly only support video/mp4 H264
Depends on: 837842
Depends on: 837859
Depends on: 838982
Depends on: 839055
(In reply to xunxun from comment #43)
> (In reply to Arno Bakker from comment #42)
> > Hi
> > 
> > I am trying live streaming with H.264 in an MPEG-TS container, but
> > apparently MPEG-TS is not supported by this change? Error log says:
> > 
> > Timestamp: 01/02/2013 13:46:26
> > Warning: HTTP “Content-Type” of “video/mp2t” is not supported. Load of media
> > resource http://127.0.0.1:8192/e5a12c7ad2d8fab33c699d1e198d66f79fa610c3.-1
> > failed.
> > 
> > This is on Nightly 21.0a1 (2013-01-31). What container formats should be
> > supported now?
> > 
> > Thanks,
> >        Arno
> 
> MPEG-TS 's Internet media type is video/MP2T.
> 
> At present, nightly only support video/mp4 H264

Do you plan on adding/enabling MPEG-TS support? Or where should I submit the feature request? ;o)
Do other browsers (IE9/10, Chrome) support MPEG-TS? If not, we are not going to support as well.
Neither Chrome dev nor IE9 support MPEG-TS on Vista.
(In reply to Marcin Starzyk from comment #46)
> Neither Chrome dev nor IE9 support MPEG-TS on Vista.

Can I ask what the recommended solution for live H.264 streaming is, if not MPEG-TS over HTTP?

* H.264 over SRTP (via webrtc/rtcweb effort)?

* H.264 in MP4 via DASH? I think this is currently not supported in FF. The above patch suggests only WebM with DASH is supported. Please correct me if I'm wrong.

* H.264 in MP4 over HTTP (chunked encoding)? Live streaming using fragmented MP4 files appears possible, but I was unable to find a definitive reference on how this is supposed to work (in particular, tuning into a live stream, does one replay the MOOV and then send the current fragment?) And whether the Windows Media Foundation supports that.

Hint: Would be nice to have a simple HTTP solution, like there is now for Ogg live and for which H.264 in MPEG-TS is the obvious candidate ;o)
Work is soon to begin on H.264 in DASH.
(In reply to Masatoshi Kimura [:emk] from comment #45)
> Do other browsers (IE9/10, Chrome) support MPEG-TS? If not, we are not going
> to support as well.

I'm not sure it's wise reasoning behind what other browser support. MPEG-TS is quite a popular format and AFAIK it is one of the encapsulation methods best supported by DASH (the other being mp4 format).

Anyway, good job on the patches, can't wait until this feature becomes crossplatform and lands in release!
Depends on: 848994
Depends on: 849264
Is it reasonable to back this out? It’s being cited by Apple as a reason to not revisit and standardise on a codec (VP8 after the reason MPEGLA agreements) in HTML5 http://lists.w3.org/Archives/Public/public-html/2013Mar/0063.html
(In reply to John Drinkwater (:beta) from comment #50)
> Is it reasonable to back this out? It’s being cited by Apple as a reason to
> not revisit and standardise on a codec (VP8 after the reason MPEGLA
> agreements) in HTML5
> http://lists.w3.org/Archives/Public/public-html/2013Mar/0063.html

As far as I understood we're not implementing the codec ourselves here, but using the Windows foundation back-end (already present in Windows). The codec itself wouldn't be included and if there are any IP/legal issues with the codec, that would fall into Microsoft's domain, right?
Nightly [1] doesn't respect settings in Options -> Applications when media.windows-media-foundation.enabled is true.

Consequence: I'm unable to download supported media files like MP3 because following a link/redirect always brings up the integrated HTML5 player even though the file type action is set to "Always ask".

I'm aware of "Save Audio/Video As..." in the player's context menu but that's a temporary workaround at best:

1.) The player causes disturbance because it starts playing automatically. I'm forced to either turn the volume down or hit stop when all I want is to download a file.
2.) "Save Audio/Video As..." opens the operating system's "Save As" dialog instead of Firefox' "You have chosen to open" which means I can't use extensions like "Save Link in Folder" [2] anymore.

[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130313 Firefox/22.0
[2] https://addons.mozilla.org/en-US/firefox/addon/save-link-in-folder/
Depends on: 847267
Depends on: 852915
Depends on: 866561
Depends on: 872375
Depends on: 871804
Depends on: 879099
Depends on: 881165
Depends on: 881954
Depends on: 882028
Depends on: 882537
Depends on: 878449
Depends on: 862182
This is not working with Firefox 26 beta 1 (build ID: 20131028225529) on Windows Vista Ultimate 32-bit. ( media.windows-media-foundation.enabled preference is set to "true" by default)

I am not able to open a .mp3 file in the browser.
Same issue on Win 7. Mp3 extension is not found on Tools/Options/Applications
Strange, it seems to work fine on Win XP.
XP doesn't have WMF - I think it uses DirectShow. The relevant pref for that is media.directshow.enabled. IIRC there are plans to switch everything to DirectShow to reduce complexity, but I don't know where those stand.
Manuela: Can you please link to an MP3 file that doesn't play for you on Vista Ultimate 32bit.

Paul: If you have an MP3 that doesn't play in Firefox on Windows 7, please can you post a link to it here.

Emanuel is correct, we use DirectShow for MP3 on WinXP. Were were going to use it in all versions of Windows, but switched back to WMF because the duration calculation is sometimes incorrect for MP3 in DirectShow. That's still the plan going forward, but we're waiting on Edwin Flores getting back from leave next week to fix the duration calculation.
(In reply to Chris Pearce (:cpearce) from comment #57)
> Manuela: Can you please link to an MP3 file that doesn't play for you on
> Vista Ultimate 32bit.
> Paul: If you have an MP3 that doesn't play in Firefox on Windows 7, please
> can you post a link to it here.
http://www39.zippyshare.com/v/56678826/file.html
http://www.share-byte.net/fQByeK
Win Vista x86, Win 7 x64
FF 26b1, 28.0a1 (2013-10-30)
When trying to play the songs in FF, the "Open with / Save file" dialog is displayed.
media.windows-media-foundation.enabled=TRUE
media.windows-media-foundation.play-stand-alone=TRUE
media.windows-media-foundation.use-dxva=TRUE
media.directshow.enabled=TRUE
Mp3 extension is not found on Firefox/Tools/Options/Applications
(In reply to Paul Silaghi, QA [:pauly] from comment #55)
> Strange, it seems to work fine on Win XP.
Works fine on Win 8 too.
Depends on: 986926
Depends on: 1095549
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: