Closed Bug 922314 Opened 11 years ago Closed 9 years ago

Add MFT Support for WebM

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: joseph.k.olivas, Assigned: joseph.k.olivas)

References

Details

Attachments

(2 files, 15 obsolete files)

79.72 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
19.59 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
Attached patch webm-mft.patch (obsolete) — Splinter Review
Hardware acceleration for WebM formats will likely be implemented through an HMFT, so support for this is needed.

I am currently working by using the Software MFT provided through webmproject.org.
Component: General → Video/Audio
Product: Firefox → Core
I see I left in some junk (commented out check for accelerated layers, geometry workaround), but this is currently working for me.

Next step is to get a working HMFT and clean it up.
Attached patch webm-mft.patch (obsolete) — Splinter Review
Rebasing patch and some minor cleanup.
Attachment #812247 - Attachment is obsolete: true
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Updating to work-in-progress with the production MFT as part of the Intel Media SDK. This version has issues with seek and falling back to a software version if the MFT isn't in place, but otherwise works with the soon-to-be released driver/MFT.

Part of the issue with fallback is that we don't know until ReadMetadata whether we can take the HW path, and I'm not exactly sure how I should go about falling back at that point. Tried to create/destroy a bogus MediaDataDecoder to test at Init() but failed because it was on the main thread.
Attachment #8383226 - Attachment is obsolete: true
Attachment #8435368 - Flags: feedback?(cpearce)
Comment on attachment 8435368 [details] [diff] [review]
Intel_VP8.patch

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

Thanks Joe! It's gratifying that we need such few changes to the MFTDecoder to use a different MFT.

In terms of design, we'd really prefer if we could use composition here rather than inheritance. We should have a "decoder" object (Strategy pattern), which the WebMReader owns. These objects encapsulate the logic of the MediaDecoderReader::DecodeVideoFrame() function. We'll need two of these should be the existing libvpx code path, and the other can be your HW accelerated path.

We should not create this until we reach the place in WebMReader::ReadMetadata() where we know whether we are decoding a VP8 or VP9 stream. I assume your MFT can't handle VP9 right?

Also, we may need some kind of limit to how many of these hardware MFTs are created... The WMFReader just limits it to 8 instances (a number I pulled out of the air) and falls back to software decoding via WMF once more than 8 are active at once.
Attachment #8435368 - Flags: feedback?(cpearce) → feedback+
Also, we'll need Matthew Gregan (:kinetik) to review the non WMF specific parts of this. He wrote most of our WebM code.
Attachment #8435368 - Flags: feedback?(kinetik)
> In terms of design, we'd really prefer if we could use composition here
> rather than inheritance. We should have a "decoder" object (Strategy
> pattern), which the WebMReader owns. These objects encapsulate the logic of
> the MediaDecoderReader::DecodeVideoFrame() function. We'll need two of these
> should be the existing libvpx code path, and the other can be your HW
> accelerated path.
> 
> We should not create this until we reach the place in
> WebMReader::ReadMetadata() where we know whether we are decoding a VP8 or
> VP9 stream. I assume your MFT can't handle VP9 right?

Thanks for the quick feedback! I completely agree with the design recommendation. This MFT will only handle VP8, but I'd expect other codecs to be available soon, but once this deisgn is in place, the others should be trivial.

> Also, we may need some kind of limit to how many of these hardware MFTs are
> created... The WMFReader just limits it to 8 instances (a number I pulled
> out of the air) and falls back to software decoding via WMF once more than 8
> are active at once.

Since the hardware is bandwidth limited, it's hard to determine a number, so maybe we can stick with 8 for this implementation until there is a need to update it.
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Updating WIP patch to address the design concerns above. This one successfully falls back to software decoding if hardware decoding isn't available.

Hardware decoder still has issues with seeking backward; I'm still investigating.

I'd also like to better combine the decode paths since they overlap quite a bit up until the actual decoding call.
Attachment #8435368 - Attachment is obsolete: true
Attachment #8435368 - Flags: feedback?(kinetik)
Attachment #8440013 - Flags: feedback?(kinetik)
Comment on attachment 8440013 [details] [diff] [review]
Intel_VP8.patch

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

Awesome, thanks Joe!

::: content/media/webm/IntelWebMVideoDecoder.cpp
@@ +92,5 @@
> +    }
> +    mMediaDataDecoder->Flush();
> +    {
> +      mIsFlushing = false;
> +      MonitorAutoLock mon(mMonitor);

The above two lines seem to be swapped (and same above where it's set to true).
Attachment #8440013 - Flags: feedback?(kinetik) → feedback+
(In reply to Matthew Gregan [:kinetik] from comment #9)
> Comment on attachment 8440013 [details] [diff] [review]
> Intel_VP8.patch
> 
> Review of attachment 8440013 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome, thanks Joe!
> 
> ::: content/media/webm/IntelWebMVideoDecoder.cpp
> @@ +92,5 @@
> > +    }
> > +    mMediaDataDecoder->Flush();
> > +    {
> > +      mIsFlushing = false;
> > +      MonitorAutoLock mon(mMonitor);
> 
> The above two lines seem to be swapped (and same above where it's set to
> true).

I agree with your observation; I just lifted this from http://dxr.mozilla.org/mozilla-central/source/content/media/fmp4/MP4Reader.cpp#460 without really looking at it.
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Fixed the seeking issue (adding a flush before the nestegg seek), and addressed kinetik's feedback.

This version seems mostly functional, though there are some leaks to be patched, and an occasional hang when seeking quickly and often.
Attachment #8440013 - Attachment is obsolete: true
(In reply to Joe Olivas from comment #10)
> I agree with your observation; I just lifted this from
> http://dxr.mozilla.org/mozilla-central/source/content/media/fmp4/MP4Reader.
> cpp#460 without really looking at it.

Yikes, thanks for pointing that out.  I filed bug 1029251 for the MP4Reader.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Rebased, and added placeholder support for VP9. Fixed the issue preventing shutdown.
Attachment #8444824 - Attachment is obsolete: true
Attachment #8473077 - Flags: feedback?(kinetik)
Attachment #8473077 - Flags: feedback?(cpearce)
With the future addition of 1022501

There's a new method that needs to be implemented:   virtual bool PlatformDecoderModule::SupportsAudioMimeType(const char* aMimeType) MOZ_OVERRIDE;

This takes a mime-type and return true if the codec is supported.

By default, it is expected that AAC is supported. If MP3 support is to be added to WMFAudioMFTManager, SupportsAudioMimeType will have to reflect this.

thanks
Thanks for the updated patch, Joe!  I'll try to get to it next week, but may not get to it until the week after that depending on my current bug load.
Comment on attachment 8473077 [details] [diff] [review]
Intel_VP8.patch

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

Looking good. :)

::: content/media/fmp4/PlatformDecoderModule.h
@@ +102,5 @@
>                      layers::ImageContainer* aImageContainer,
>                      MediaTaskQueue* aVideoTaskQueue,
>                      MediaDataDecoderCallback* aCallback) = 0;
>  
> +  // Similar creation as H.264, but for VP8/VP9

I think we should just rename CreateH264Decoder to CreateVideoDecoder, and rely on the mp4_demuxer::VideoDecoderConfig.mime_type field to tell us which codec we should create.

We'll also need a virtual bool SupportsVideoMimeType(const char* aMimeType) function here too.

This is basically the approach we're using in bug 1022501 to add support for more audio codecs to the PDM.

@@ +115,1 @@
>    // Creates an AAC decoder with the specified properties.

We'll need a SupportsVideoMimeType() function here too, like in the audio case.

::: content/media/fmp4/wmf/WMFDecoderModule.cpp
@@ +144,5 @@
>                                                     aImageContainer,
>                                                     sDXVAEnabled),
>                              aVideoTaskQueue,
> +                            aCallback,
> +                            CLSID_CMSH264DecoderMFT,

This can be simpler; we don't need to pass in the clsids here, as the WMF*MFTManager knows the mime type (from VideoDecoderConfig.mime_type) so it can figure out the clsid required.

I in fact did this in Bug 1055974 to add MP3 support to the WMFAudioMFTManager, so you can extend that for Vorbis, and then copy the same approach for video. (Your patch here was very helpful in getting my patch for bug 1055974 to work quickly, thanks very much!).
Attachment #8473077 - Flags: feedback?(cpearce) → feedback+
Comment on attachment 8473077 [details] [diff] [review]
Intel_VP8.patch

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

This looks great, thanks Joe!  f+ with comments addressed.

Ultra-minor nit: there are a bunch of trailing spaces added, they show up as red in the review tool.  Would you mind fixing these please?

Most of the rest of my feedback is pretty minor too.

::: content/media/fmp4/wmf/WMFDecoderModule.cpp
@@ +12,5 @@
>  #include "mozilla/Preferences.h"
>  #include "mozilla/DebugOnly.h"
>  #include "WMFMediaDataDecoder.h"
>  
> +#include "nestegg/nestegg.h"

Doesn't seem to be needed in this file?

::: content/media/webm/IntelWebMVideoDecoder.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "nsError.h"
> +#include "MediaResource.h"
> +#include "IntelWebMVideoDecoder.h"

Include this first to catch any missing header deps.  Then the rest of the includes should be in alphabetical order.  

(Which the exception of SharedThreadPool.h/MediaTaskQueue.h, which are broken and need to be fixed. :-/)

@@ +21,5 @@
> +#include "vpx/vpx_decoder.h"
> +
> +using mozilla::layers::Image;
> +using mozilla::layers::LayerManager;
> +using mozilla::layers::LayersBackend;

Move these inside the namespace mozilla block and drop the mozilla::

@@ +40,5 @@
> +class VP8Sample : public MP4Sample 
> +{
> +public: 
> +  VP8Sample(
> +            int64_t aTimestamp,

Style nit:
    VP8Sample(int64_t aTimestamp,

@@ +57,5 @@
> +
> +    data =  new uint8_t[aSize];
> +    memmove(data, aData, size);
> +  }
> +  ~VP8Sample(){}

() {}

@@ +80,5 @@
> +  mEOS(false),
> +  mError(false),
> +  mIsAccepting(true),
> +  mNeedsMoreInput(true),
> +  mReader(aReader)

Style nit:
 : WebMVideoDecoder()
 , mPlatform(nullptr)
 , mMonitor("IntelWebMVideoDecoder")
 ...

Also, all of the pointer types like nsRefPtr, nsAutoPtr, etc. default initialize to null, so you can remove mPlatform(nullptr) etc.

@@ +102,5 @@
> +    }
> +    mMediaDataDecoder->Flush();
> +    {
> +      mIsFlushing = false;
> +      MonitorAutoLock mon(mMonitor);

In two places here, the lock and flag modification are supposed to be in the other order.  It was wrong in the code this came from, but it has since been fixed.

@@ +159,5 @@
> +  NS_ENSURE_TRUE(mMediaDataDecoder != nullptr, NS_ERROR_FAILURE);
> +  nsresult rv = mMediaDataDecoder->Init();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +

Nit: two empty lines.

@@ +191,5 @@
> +  mLayersBackendType = layerManager->GetCompositorBackendType();
> +}
> +
> +bool
> +IntelWebMVideoDecoder::Demux(nsAutoPtr<VP8Sample>* aSample, bool* eos)

nsAutoPtr<VP8Sample>&

eos should be aEOS, it can remain as a bool* rather than a bool& since we seem to have a mixed policy on that one.

@@ +236,5 @@
> +      }
> +      mReader->PushVideoPacket(next_holder.disown());
> +    } else {
> +      ReentrantMonitorAutoEnter decoderMon(mReader->GetDecoder()->GetReentrantMonitor());
> +      int64_t endTime = mReader->GetDecoder()->GetEndMediaTime();

We ended up removing GetEndMediaTime, there's a different approach in the WebMReader now.

This chunk, and much of the rest of this function, would be good to share code with the existing code in WebMReader as there's a fair bit of duplication.  Happy for it to be handled in a follow-up bug, though.

@@ +271,5 @@
> +
> +  return true;
> +}
> +
> +

Double empty line here, and in more places further down.

@@ +290,5 @@
> +    // The Intel VP8 MFT has very needy output, so
> +    // we keep inputting until we get MF_E_NOTACCEPTING
> +    while (prevNumFramesOutput == mNumSamplesOutput &&
> +           (mInputExhausted ||
> +           (mNumSamplesInput - mNumSamplesOutput) < mDecodeAhead) &&

This line needs more indentation since it's inside the parens starting at (mInputExhausted...

@@ +347,5 @@
> +  return rv;
> +}
> +
> +bool
> +IntelWebMVideoDecoder::SkipVideoDemuxToNextKeyFrame(int64_t aTimeThreshold, uint32_t& parsed)

aParsed

@@ +374,5 @@
> +}
> +
> +
> +bool IntelWebMVideoDecoder::DecodeVideoFrame(bool &aKeyframeSkip,
> +                                      int64_t aTimeThreshold)

bool
IntelWebMVideoDecoder::...

bool& aKeyframeSkip

and indentation of aTimeThreshold is off.

@@ +410,5 @@
> +IntelWebMVideoDecoder::PopSample()
> +{
> +  VP8Sample* sample = nullptr;
> +  if(mQueuedVideoSample) {
> +    sample = mQueuedVideoSample.forget();

Make this:
    if(mQueuedVideoSample) {
      return mQueuedVideoSample.forget();
    }

And then unindent the else.

@@ +422,5 @@
> +        return nullptr;
> +      }
> +      MOZ_ASSERT(sample);
> +      VP8Sample* s = sample.forget();
> +      mSampleQueue.push_back(s);

I'd make this
    mSampleQueue.push_back(mSampleQueue.push_back(s));

::: content/media/webm/IntelWebMVideoDecoder.h
@@ +64,5 @@
> +
> +  nsAutoPtr<PlatformDecoderModule> mPlatform;
> +  nsRefPtr<MediaDataDecoder> mMediaDataDecoder;
> +
> +  bool mDXVAEnabled;

We try to keep all of the bools together at the end of the class definition.

@@ +92,5 @@
> +
> +  bool mIsAccepting;
> +  bool mNeedsMoreInput;
> +
> +  WebMReader* mReader;

Needs documentation on ownership, and/or to be an nsRefPtr.

::: content/media/webm/SoftwareWebMVideoDecoder.cpp
@@ +6,5 @@
> +#include "nsError.h"
> +#include "MediaDecoderStateMachine.h"
> +#include "AbstractMediaDecoder.h"
> +#include "MediaResource.h"
> +#include "SoftwareWebMVideoDecoder.h"

Include this first.

@@ +123,5 @@
> +      int64_t endTime = mReader->GetDecoder()->GetEndMediaTime();
> +      if (endTime == -1) {
> +        return false;
> +      }
> +      next_tstamp = endTime * NS_PER_USEC;

Same comments apply as the other copy of this code: GetEndMediaTime thing and it'd be really great to avoid the code duplication somehow.

::: content/media/webm/SoftwareWebMVideoDecoder.h
@@ +29,5 @@
> +  virtual bool DecodeVideoFrame(bool &aKeyframeSkip,
> +                                  int64_t aTimeThreshold) MOZ_OVERRIDE;
> +
> +private:
> +  WebMReader* mReader;

Document ownership/lifecycle and/or use an nsRefPtr.

::: content/media/webm/WebMReader.cpp
@@ +147,5 @@
> +  nsAutoPtr<WebMVideoDecoder> mVideoDecoder;
> +  nsAutoPtr<WebMReader> mReader;
> +};
> +
> +class CreateSoftwareVideoDecoderEvent : public nsRunnable {

Is there a disadvantage to having a single event that tries IntelWebMVideoDecoder then SoftwareWebMVideoDecoder? That'd simplify the code in ReadMetadata a bit.

@@ +326,5 @@
>        mVideoCodec = nestegg_track_codec_id(mContext, track);
> +
> +      // Try to create video decoder that we can use
> +      nsRefPtr<CreateIntelVideoDecoderEvent> event(new CreateIntelVideoDecoderEvent(this));
> +      nsresult nv = NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);

rv rather than nv

@@ +327,5 @@
> +
> +      // Try to create video decoder that we can use
> +      nsRefPtr<CreateIntelVideoDecoderEvent> event(new CreateIntelVideoDecoderEvent(this));
> +      nsresult nv = NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);
> +      if(NS_SUCCEEDED(nv)) {

Space between if and ( here and elsewhere.

@@ +336,4 @@
>        }
> +
> +      // if we fail the HW path, backoff to SW
> +      if(NS_FAILED(nv)) {

This can just be
    } else {
      ...

@@ +880,5 @@
>                            int64_t aCurrentTime)
>  {
>    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>  
> +  nsresult nv = mVideoDecoder->Flush();

rv
Attachment #8473077 - Flags: feedback?(kinetik) → feedback+
> ::: content/media/webm/WebMReader.cpp
> @@ +147,5 @@
> > +  nsAutoPtr<WebMVideoDecoder> mVideoDecoder;
> > +  nsAutoPtr<WebMReader> mReader;
> > +};
> > +
> > +class CreateSoftwareVideoDecoderEvent : public nsRunnable {
> 
> Is there a disadvantage to having a single event that tries
> IntelWebMVideoDecoder then SoftwareWebMVideoDecoder? That'd simplify the
> code in ReadMetadata a bit.

I think the difficulty was that these need to be created on the main thread, but Init()ed on the worker thread, and we don't know if we fail until Init(), unfortunately. If there's a better approach, I'm all ears, otherwise, updated patch is coming...
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Updated patch resolving almost all of the feedback so far. Much thanks for the feedback!
Attachment #8473077 - Attachment is obsolete: true
(In reply to Joe Olivas from comment #18)
> I think the difficulty was that these need to be created on the main thread,
> but Init()ed on the worker thread, and we don't know if we fail until
> Init(), unfortunately. If there's a better approach, I'm all ears,
> otherwise, updated patch is coming...

Good point, I overlooked that!
Comment on attachment 8494162 [details] [diff] [review]
Intel_VP8.patch

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

Thanks Joe, this is great!

Just a heads up: bug 1067377 (part 3) causes your patch not to apply to WebMReader.cpp, but it's a trivial fix.  s/IMG_FMT_I420/VPX_IMG_FMT_I420/ in the old and new location.

I'll attach a trivial patch that I needed to get this to build on Linux, mostly issues with compiler differences or due to our warnings-as-errors policy.  With that applied, it builds, but playing a WebM file on Linux seems to be selecting the wrong decoder (I get a bunch of debug spam from FFmpeg's H.264 decoder!).

::: content/media/fmp4/BlankDecoderModule.cpp
@@ +215,4 @@
>                      layers::LayersBackend aLayersBackend,
>                      layers::ImageContainer* aImageContainer,
>                      MediaTaskQueue* aVideoTaskQueue,
>                      MediaDataDecoderCallback* aCallback) MOZ_OVERRIDE {

Params need extra indentation now.

::: content/media/webm/IntelWebMVideoDecoder.cpp
@@ +50,5 @@
> +    byte_offset = aByteOffset;
> +    is_sync_point = aSyncPoint;
> +    size = aSize;
> +
> +    data =  new uint8_t[aSize];

Where is this freed?

@@ +183,5 @@
> +IntelWebMVideoDecoder::Demux(nsAutoPtr<VP8Sample>& aSample, bool* aEOS)
> +{
> +  // Record number of frames decoded and parsed. Automatically update the
> +  // stats counters using the AutoNotifyDecoded stack-based class.
> +  uint32_t parsed = 0, decoded = 0;

Unused?

::: content/media/webm/IntelWebMVideoDecoder.h
@@ +88,5 @@
> +  bool mError;
> +  bool mEOS;
> +  bool mIsFlushing;
> +
> +  bool mDXVAEnabled;

Unused?

@@ +93,5 @@
> +  bool mIsAccepting;
> +  bool mNeedsMoreInput;
> +
> +  nsRefPtr<WebMReader> mReader;
> +

Move mReader above the bools (anywhere you think it makes sense), and remove the empty line between the last member and the closing brace.

::: content/media/webm/SoftwareWebMVideoDecoder.cpp
@@ +70,5 @@
> +  return NS_OK;
> +}
> +
> +bool SoftwareWebMVideoDecoder::DecodeVideoFrame(bool &aKeyframeSkip,
> +                                      int64_t aTimeThreshold)

Intendation.

::: content/media/webm/SoftwareWebMVideoDecoder.h
@@ +26,5 @@
> +  // If the Theora granulepos has not been captured, it may read several packets
> +  // until one with a granulepos has been captured, to ensure that all packets
> +  // read have valid time info.
> +  virtual bool DecodeVideoFrame(bool &aKeyframeSkip,
> +                                  int64_t aTimeThreshold) MOZ_OVERRIDE;

Intendation.

@@ +33,5 @@
> +  nsRefPtr<WebMReader> mReader;
> +
> +  // VPx decoder state
> +  vpx_codec_ctx_t mVPX;
> +

Empty line.

::: content/media/webm/WebMReader.h
@@ +125,5 @@
> +  };
> +  virtual bool DecodeVideoFrame(bool &aKeyframeSkip,
> +                                int64_t aTimeThreshold) = 0;
> +  WebMVideoDecoder() {};
> +  ~WebMVideoDecoder() {};

Needs a virtual dtor.

@@ +266,5 @@
>    bool mHasAudio;
> +
> +  // The video decoder
> +  nsAutoPtr<WebMVideoDecoder> mVideoDecoder;
> +

New line.
Attachment #8494162 - Flags: feedback+
Comment on attachment 8494162 [details] [diff] [review]
Intel_VP8.patch

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

::: content/media/webm/WebMReader.cpp
@@ +185,5 @@
>    memset(&mVorbisBlock, 0, sizeof(vorbis_block));
>    memset(&mVorbisDsp, 0, sizeof(vorbis_dsp_state));
>    memset(&mVorbisInfo, 0, sizeof(vorbis_info));
>    memset(&mVorbisComment, 0, sizeof(vorbis_comment));
>  }

Lost a new line here.

@@ +207,3 @@
>  
>    MOZ_COUNT_DTOR(WebMReader);
>  }

And here.

@@ +312,5 @@
> +      nsresult nv = NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);
> +      if (NS_SUCCEEDED(nv)) {
> +        mVideoDecoder = event->mVideoDecoder;
> +        if (mVideoDecoder) {
> +          nv = mVideoDecoder->Init(params.display_width, params.display_height);

s/nv/rv/

@@ +322,5 @@
> +        nsRefPtr<CreateSoftwareVideoDecoderEvent> event(new CreateSoftwareVideoDecoderEvent(this));
> +        NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);
> +        mVideoDecoder = event->mVideoDecoder;
> +        if (mVideoDecoder) {
> +          nv = mVideoDecoder->Init(params.display_width, params.display_height);

s/nv/rv/
What happens on Linux is: WebMReader::ReadMetadata calls IntelWebMVideoDecoder::Init, which succeeds because PlatformDecoder::Create returns a FFmpegDecoderModule, and calling mPlatform->CreateVideoDecoder succeeds, returning a FFmpegH264Decoder.  Which later fails to decode VP8 frames.

It seems like there needs to be a way to select the PDM based on the type you want to decode, or something.  cpearce will have more thoughts on this.

Anywa, simple fix is to surround CreateIntelVideoDecoderEvent code with #ifdef XP_WIN.  After that, playback with the software decoder works great.
(In reply to Matthew Gregan [:kinetik] from comment #24)
> It seems like there needs to be a way to select the PDM based on the type
> you want to decode, or something.  cpearce will have more thoughts on this.

Or perhaps FFMpegDecoderModule::CreateVideoDecoder should check aConfig->mime_type and return a nullptr if it's a type it can't decode.
(In reply to Matthew Gregan [:kinetik] from comment #25)
> (In reply to Matthew Gregan [:kinetik] from comment #24)
> > It seems like there needs to be a way to select the PDM based on the type
> > you want to decode, or something.  cpearce will have more thoughts on this.
> 
> Or perhaps FFMpegDecoderModule::CreateVideoDecoder should check
> aConfig->mime_type and return a nullptr if it's a type it can't decode.

Yes, I think if it implemented the SupportsVideoMimeType method, then things would work. Right now, it looks to default to the PlatformDecoderModule's version which now returns true for mp4 and webm, which may not be ideal.
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Updating to fix issues from feedback. Thanks again, and sorry for overlooking some obvious stuff :)
Attachment #8494162 - Attachment is obsolete: true
Ignore that last fix, I missed something... another patch coming.
Attached patch Intel_VP8.patch (obsolete) — Splinter Review
Fixing the missing SupportsVideoMimeType checks. It looks likethe other descendants of PDM need to implement this function as well.

I'm not 100% which mime types should be supported by each, so any guidance is appreciated.
Attachment #8494874 - Attachment is obsolete: true
Attached patch bug922314.patch (obsolete) — Splinter Review
Bug 1069660 caused this patch not to apply due to lots of whitespace changes (sorry!), so I've rebased it against the current head and attached it here to avoid pushing the pain on to Joe.  So: this is Joe's latest patch (attachment 8494883 [details] [diff] [review]), plus the few compile fixes still needed from my Linux build patch.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e4e6e38114d5

(note that the tests are likely to fail because of the Video MIME type issue needing to be addressed for all of the PDMs)

(...also it looks like the Try infra just fell over, so it might not even build tonight!)
Attachment #8494262 - Attachment is obsolete: true
Attachment #8494883 - Attachment is obsolete: true
Assignee: nobody → joseph.k.olivas
Status: NEW → ASSIGNED
Thanks, Matthew. Please let me know if there's anything I can do to help along the way.
Thanks Joe!

I've tracked down a couple of issues:

- init events holding a ref to WebMReader with an nsAutoRef rather than an nsRefPtr
- refcount cycle between the reader and decoder causing leaks
    it was being broken in the reader destructor, which won't be called until the cycle is broken,
    so added a Shutdown() to WebMReader and moved the decoder shutdown there

I changed the base PDM so SupportsVideoMimeType only accepts video/mp4, and then made the WMF subclass also accept video/webm.  Not sure if this is the right approach longer term, but it's easier than modifying every PDM.

Latest try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6decfc98bf56
(In reply to Matthew Gregan [:kinetik] from comment #32)
> Latest try push:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6decfc98bf56

test_encryptedMediaExtensions.html fails, then crashes in MP4Reader::Output
  - Linux opt, Linux64 opt, Linux64 debug

test_encryptedMediaExtensions.html fails
  - Linux64 asan, XP opt, XP debug

leaked (MediaDecoderReader, ReentrantMonitor, SoftwareWebMVideoDecoder, WebMBufferedState, WebMReader)
  - XP debug

test_cycle_5.html crashes (not a media test, but after media tests have run):
  - Linux debug (no call stack)

test_dataChannel_basicAudioVideo.html times out
  - Linux debug

test_autoplay_contentEditable.html, test_buffered.html, test_bug448534.html, test_bug465498.html times out
  - OSX 10.6 opt, OSX 10.6 debug, OSX 10.8 opt, OSX 10.8 debug, Win7 opt, Win7 debug, Win8 opt, Win8 debug

test_FrameSelection.html | videoWidth has correct final value - got 320, expected 160
  - Win7 opt, Win7 debug, Win8 opt, Win8 debug

Also some issues on Android and B2G emulator builds, but it's unclear what's going on there, so I'll focus on the stuff above first and revisit those later.

I'm not seeing any of these failures locally when running the media tests with:
  ./mach mochitest-plain content/media
Making progress, two issues remaining, both of which I can reproduce now:

(In reply to Matthew Gregan [:kinetik] from comment #33)
> leaked (MediaDecoderReader, ReentrantMonitor, SoftwareWebMVideoDecoder,
> WebMBufferedState, WebMReader)

Seeing this on multiple platforms now.  Unsure which test(s) are leaking so far.  Narrowing that down will probably make it easier to find the root cause.

> test_FrameSelection.html | videoWidth has correct final value - got 320,
> expected 160
>   - Win7 opt, Win7 debug, Win8 opt, Win8 debug

Seems to be timing related, I need to run many cycles of the test to reproduce this locally.
Thanks for the time, Matthew! I would like to enable the VP9 part of this patch so others here can easily get up and running on bug 1079533, but right now, the WMFVideoMFTManager constructor only has access to a mime type (through mp4_demuxer::VideoDecoderConfig), which isn't enough information to determine if the codec is VP8 or VP9.

Any suggestions on the best approach for this? In an earlier patch, I was passing the Nestegg codec constant around, but that didn't seem ideal, so right now it defaults to VP8.
Looks like the memory leak is fixed, so just test_FrameSelection on Windows left to track down.  Still working on that one.  I'll post the updated patch shortly.

(In reply to Joe Olivas from comment #35)
> Thanks for the time, Matthew! I would like to enable the VP9 part of this
> patch so others here can easily get up and running on bug 1079533, but right
> now, the WMFVideoMFTManager constructor only has access to a mime type
> (through mp4_demuxer::VideoDecoderConfig), which isn't enough information to
> determine if the codec is VP8 or VP9.
> 
> Any suggestions on the best approach for this? In an earlier patch, I was
> passing the Nestegg codec constant around, but that didn't seem ideal, so
> right now it defaults to VP8.

My immediate suggestion (so not necessarily the best approach!) is to extend the MIME string to contain the codec (https://wiki.whatwg.org/wiki/Video_type_parameters).  That'd make it "video/webm; codecs=vp8" for VP8 and "video/webm; codecs=vp9" for VP9.  There's existing code for parsing these strings in nsContentTypeParser.h (example in HTMLMediaElement.cpp GetCanPlay to fetch the codecs string).
(In reply to Matthew Gregan [:kinetik] from comment #36)
> My immediate suggestion (so not necessarily the best approach!) is to extend
> the MIME string to contain the codec
> (https://wiki.whatwg.org/wiki/Video_type_parameters).  That'd make it
> "video/webm; codecs=vp8" for VP8 and "video/webm; codecs=vp9" for VP9. 
> There's existing code for parsing these strings in nsContentTypeParser.h
> (example in HTMLMediaElement.cpp GetCanPlay to fetch the codecs string).

I've pushed a simple patch to the try server that addresses this: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=978a4a196797
Attached patch bug922314_v2.patch (obsolete) — Splinter Review
Rebased to latest mozilla-inbound.

Remaining test failures:
- audio_has_no_subtitles on Wr (looks like I missed this one earlier)
- test_FrameSelection on M2

This includes my attempt at fleshing out the MIME type with the codec details for VP8/VP9, but I haven't tested it as I don't have the appropriate hardware handy/configured yet.
Attachment #8498663 - Attachment is obsolete: true
(In reply to Matthew Gregan [:kinetik] from comment #38)
> Remaining test failures:
> - audio_has_no_subtitles on Wr (looks like I missed this one earlier)

This is caused by using a video-only WebM in an <audio> element, which used to succeed but now fails and reports a DecodeError(), i.e. the same issue as test_video_in_audio_element.html:

< kinetik> cpearce: the test you added for bug 1060896 seems to assume the test video also has an audio track, but that's only true for gizmo.mp4 and the other media just happen to pass because we don't disable the video track
< kinetik> cpearce: with the intel patch (and the same image container crash fixed), the webm tests hang
< cpearce> so what should we do in an audio element if there's no audio stream?
< cpearce> should we actually play?
< cpearce> should we be duration 0?
< kinetik> fire an error?
< cpearce> yeah, I guess fire an error, since there's no way we can meaningfully play anything

Note that this is the same behaviour as our MP4 decoder, so the test would still fail if it only tested MP4, and now our behaviour matches.  I think the best way forward is to xfail that test and file a follow up bug to work out the correct decoder behaviour/test expectations.

> - test_FrameSelection on M2

If I disable the attempt to initialize the IntelWebMVideoDecoder, this test stops failing, so that narrows down the code to investigate substantially.  I'm not sure I'll be able to spend much more time on this, so I'm going to propose landing this patch with IntelWebMVideoDecoder behind a pref for now (so we can avoid rebasing the patch even more), and file a follow up bug to track down the cause of this test failure.
With pref and audio_has_no_subtitles web-platform reftest xfailed:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d8da59f84119
Depends on: 1096742
Depends on: 1096748
Depends on: 1096750
Attached patch bug922314_v3.patch (obsolete) — Splinter Review
Beyond the patch that :cpearce and I f+ed a while back are the following changes made by Joe and I:

- rebased to latest mozilla-inbound
- hid IntelWebMVideoDecoder stuff behind MOZ_FMP4 since it relies on that infrastructure to build
- adds SupportsVideoMimeType (alongside existing SupportsAudioMimeType) to PDM interface
- adds IsSupportedVideoMimeType to MP4Reader and IntelWebMVideoDecoder
- adds codec=vp{8,9} to WebM MIME types to allow IntelWebMVideoDecoder to initialize correct decoder
- switch to nsRefPtr vs nsAutoPtr where necessary
- fix WebMReader cycle/leak
- slightly simplified the CreateIntelVideoDecoderEvent/CreateSoftwareVideoDecoderEvent initialization
- minor formatting changes

Already pushed to try as https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d8da59f84119, but now the test changes have been split out to bug 1096748 and bug 1096750.
Attachment #8517946 - Attachment is obsolete: true
Attachment #8520369 - Flags: review?(cpearce)
Comment on attachment 8520369 [details] [diff] [review]
bug922314_v3.patch

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

Overall looking promising.

Fix these issues, and then we'd better run mochitests on the reference Ultrabook Joe sent me...

::: dom/media/fmp4/wmf/WMFDecoderModule.cpp
@@ +96,5 @@
> +{
> +  return !strcmp(aMimeType, "video/mp4") ||
> +         !strcmp(aMimeType, "video/avc") ||
> +         !strcmp(aMimeType, "video/webm; codecs=vp8") ||
> +         !strcmp(aMimeType, "video/webm; codecs=vp9");

If we have a flag that stores whether the MFT's are available, we can check that here and be more accurate about whether we support VPx here.

::: dom/media/webm/IntelWebMVideoDecoder.cpp
@@ +35,5 @@
> +using layers::Image;
> +using layers::LayerManager;
> +using layers::LayersBackend;
> +
> +class VP8Sample : public MP4Sample

We should (in future) remove stagefright::MediaBuffer; it doesn't really serve any purpose for us, and it looks like we only need to subclass MP4Sample here to avoid storing the compressed sample data in a MediaBuffer.

@@ +71,5 @@
> +  , mLayersBackendType(layers::LayersBackend::LAYERS_NONE)
> +  , mMonitor("IntelWebMVideoDecoder")
> +  , mNumSamplesInput(0)
> +  , mNumSamplesOutput(0)
> +  , mDecodeAhead(32)

You should be able to use a much smaller decode ahead... Like 2 for example. If this number is too big, you'll probably use more memory than you need to. The MediaDataDecoder should call InputExhausted if it needs more input to produce more output and if we need more output we will then send more input. So we shouldn't need decode-ahead to be so large.

@@ +91,5 @@
> +void
> +IntelWebMVideoDecoder::Shutdown()
> +{
> +  if (mMediaDataDecoder) {
> +    {

If you're going to go to the trouble of locking and setting mIsFlushing, then I think you may as well call the full blown IntelWebMVideoDecoder::Flush() here, so that you don't miss any important steps, or duplicate code.

@@ +208,5 @@
> +
> +bool
> +IntelWebMVideoDecoder::Demux(nsAutoPtr<VP8Sample>& aSample, bool* aEOS)
> +{
> +  nsAutoRef<NesteggPacketHolder> holder(mReader->NextPacket(WebMReader::VIDEO));

This demuxing code is shared with the Software path as well. You should factor it out into a common place... Like in a WebMDemuxer class or somesuch that you can pass to the WebMDecoders?

@@ +278,5 @@
> +  return true;
> +}
> +
> +bool
> +IntelWebMVideoDecoder::Decode()

One day, we'll refactor to pull our demuxer layer out and we won't have to duplicate MP4Reader's logic here...

@@ +343,5 @@
> +    }
> +
> +  }
> +  mMonitor.AssertCurrentThreadOwns();
> +  bool rv = !(mEOS || mError);

MP4Reader's equivalent condition here is:

bool rv = !(data.mDrainComplete || data.mError);

I think that's because (IIRC) EOS is set to true when the demux hits EOS, but the decoder may not have finishing draining and outputting all samples yet. I suspect that Joe copied the MP4Reader code before we found that issue with the MP4Reader.

So you should make this equivalent (unless you can convince yourself that that can't happen for WebM).

::: dom/media/webm/WebMReader.cpp
@@ +359,3 @@
>        mVideoCodec = nestegg_track_codec_id(mContext, track);
> +
> +#if defined(MOZ_FMP4)

I think this should be also #ifdef XP_WIN (or HW_VPX_DECODER perhaps?), as we don't need to to this on non-Windows platforms.

@@ +359,5 @@
>        mVideoCodec = nestegg_track_codec_id(mContext, track);
> +
> +#if defined(MOZ_FMP4)
> +      // Try to create a hardware accelerated Intel video decoder.
> +      nsRefPtr<CreateIntelVideoDecoderEvent> ev(new CreateIntelVideoDecoderEvent(this));

You should not need to dispatch to the main thread anymore to create the PDM or MediaDataDecoders. We removed the create-on-main-thread requirement recently to delay creation of the PDM incase MP4Reader needed to have a PDM backed by a CDM - and we don't know whether we're decoding encrypted video until we've reached ReadMetadata() on the decode thread.

So you should be able to remove this thread dispatch here completely and create the PDM/Intel decoder here on this thread immediately. Even if you can't in the HW path, you should be able to in the SW path.

I think you'll need to call PlatformDecoderModule::Init() in WebMReader::Init() for this to work. If you're doing that I think it would be a good idea to try instantiating the MFTs and setting a flag if that fails, and checking that flag here. You could do that in WebMReader::Init() or in WMFDecoderModule::Init(), which PlatformDecoderModule::Init() calls, and only if you've not already instantiated the MFTs.

Making the flag a static (rather than storing the result) means we'll still try again after restart, so if the user has updated drivers for example to enable the codec, it will work.
Attachment #8520369 - Flags: review?(cpearce) → review-
Blocks: 1101885
(In reply to Chris Pearce (:cpearce) from comment #42)
> Fix these issues, and then we'd better run mochitests on the reference
> Ultrabook Joe sent me...

Thanks for the review.  Let's spin off a separate bug for testing and enabling the Intel backend.  There's already bug 1096742 for a test failure with it enabled but not used (which may be fixed by removing the main thread dispatches, I need to verify).  I don't have a lot more time to spend on this, so we need to land the bulk of the patch to reduce the surface of rebasing/testing required to make progress.  Filed bug 1101885 for this.

> We should (in future) remove stagefright::MediaBuffer; it doesn't really
> serve any purpose for us, and it looks like we only need to subclass
> MP4Sample here to avoid storing the compressed sample data in a MediaBuffer.

Filed bug 1101876.

> One day, we'll refactor to pull our demuxer layer out and we won't have to
> duplicate MP4Reader's logic here...

I made the same observation in my local TODO list.  Filed bug 1101878.
Awesome; thanks guys. I'm doing some testing on my side, and will start on the non-future changes for this patch.
Sorry, I've gone ahead and made some already, let me upload the patches and hand them back to you!
Attached patch bug922314_v4_p1.patch (obsolete) — Splinter Review
This is the patch that Chris reviewed, plus small changes to rebase against the refcounted MediaData and shared decoder patches that just landed on mozilla-inbound.
Attachment #8520369 - Attachment is obsolete: true
Attached patch bug922314_v4_p2.patch (obsolete) — Splinter Review
Review comments addressed in this patch:

- mConfig removed from WMFVideoMFTManager base class, use mStreamType instead
  (This should've been part of p1 rebasing, but leaked here)
- Shrink mDecodeAhead to 2
- Flush() in Shutdown()
- Remove main-thead dispatch and Create*Decoder runnables
- Move PlatformDecoderModule::Init, task queue setup, and layer queries to WebMReader::Init (as these need to run on the main thread)
- Introduce MOZ_PDM_VPX to enable the PDM-based VPx decoder
  (currently depends on MOZ_FMP4 and XP_WIN)

Still todo:
- Add flag for MFT availability and test in WMFDecoderModule::SupportedVideoMimeType
- Address EOS/error comment in IntelWebMVideoDecoder::Decode
- Factor shared code from SoftwareWebMVideoDecoder::DecodeVideoFrame and IntelWebMVideoDecoder::Demux to a single location
All yours Joe, thanks!  Let me know here (or on irc.mozilla.org in #media) if I can help out in any way.  I'm keen to get this landed ASAP. :-)
Chris mentioned on IRC that VP8 was accelerated but VP9 wasn't with bug922314_v3.patch, so that's something else that may need chasing up.
(In reply to Matthew Gregan [:kinetik] from comment #47)
> Still todo:
> - Add flag for MFT availability and test in
> WMFDecoderModule::SupportedVideoMimeType
> - Address EOS/error comment in IntelWebMVideoDecoder::Decode
> - Factor shared code from SoftwareWebMVideoDecoder::DecodeVideoFrame and
> IntelWebMVideoDecoder::Demux to a single location

Discussed with Anthony yesterday; the refactoring can be spun off into a separate patch, and since the other two issues are in the code I'm going to land prefed-off initially, they can be fixed as part of the pref-on patch (bug 1101885).

So ideally, to break this down into smaller pieces and avoid more rebasing work, we'd like to land the patch as-is (if it passes review, minus the items above), then follow up in spun off bugs.
Need to chase up a new issue created by the MediaTaskQueue move in p2 first, then patches will be back up for review.
This is the patch Chris reviewed +/- any changes required for rebasing.
Attachment #8525639 - Attachment is obsolete: true
Attachment #8526535 - Flags: review?(cpearce)
Address most of the review comments; others (specific to the new decoder) deferred to bug 1101885.

Pref on:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09494ecc9bbe

Pref off:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f4563f4525f1
Attachment #8525640 - Attachment is obsolete: true
Attachment #8526536 - Flags: review?(cpearce)
Thanks, Matthew. Got hung up on something else and didn't get to fixing it yet... I'll track/fix the new bugs.
Attachment #8526535 - Flags: review?(cpearce) → review+
Comment on attachment 8526536 [details] [diff] [review]
bug922314_v5_p2.patch

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

::: dom/media/webm/WebMReader.cpp
@@ +160,5 @@
>    packet.packetno = aPacketNo;
>    return packet;
>  }
>  
> +static bool sIsIntelDecoderEnabled = false;

#if defined(MOZ_PDM_VPX) around this?

@@ +198,5 @@
>    memset(&mVorbisDsp, 0, sizeof(vorbis_dsp_state));
>    memset(&mVorbisInfo, 0, sizeof(vorbis_info));
>    memset(&mVorbisComment, 0, sizeof(vorbis_comment));
> +
> +  sIsIntelDecoderEnabled = Preferences::GetBool("media.webm.intel_decoder.enabled", false);

#if defined(MOZ_PDM_VPX) around this?
Attachment #8526536 - Flags: review?(cpearce) → review+
Added those two ifdefs, rolled patch up and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9f88d61684
https://hg.mozilla.org/mozilla-central/rev/7e9f88d61684
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
No longer depends on: 1096742
Depends on: 1104410
Depends on: 1105867
You need to log in before you can comment on or make changes to this bug.