1206977 - Allow PlatformDecoderModule fallback

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Jean-Yves Avenard [:jya]]

If we are to enable ffvpx we need a mechanism to ensure that we can fallback from one PDM to the next.

The PlatformDecoderModule architecture was conceived as the name indicate for one platform at a time.

As such, on windows we will create the WMF PlatformDecoderModule and attempt to use it for everything and will ignore others.

This is too limiting, we need to be able to fallback to another PDM ; or have a more elegant way to separate PDMs that rely on the OS framework and those working with pure software that we control or 3rd party library that can be installed (like ffmpeg etc)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove unused PDM function members.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Wrap PDM creation in a new PDMFactory class.

There is no change of behaviour from the original PlatformDecoderModule.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P3. Allow PDM fallback.

We now search in all the PDM present the one that can handle the media.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P4. Add AgnosticDecoderModule object.

This removes the need for PDMFactory to know anything about decoders.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P5. Update PlatformDecoderModule documentation.

Mostly removes no longer relevant doc.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: P6. Make PlatformDecoderModule::SupportsMimeType pure virtual.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8670026 [details] [diff] [review]
P5. Update PlatformDecoderModule documentation.

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

::: dom/media/platforms/PlatformDecoderModule.h
@@ +36,1 @@
>  // It may be extended to support other codecs in future. Each platform (Windows,

"It may be extended to support other codecs in future." is an out of date comment now too!

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P7. Remove IntelWebMVideoDecoder.

That code path is no longer used and handled directly in the MediaFormatReader.

Also, partially revert commit ac6d0b0befb2 as it broke WebMReader.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P8. Have PDMFactory directly manage the EMEDecoderModule.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: P9. Ensure PDMs are only ever created through the PDMFactory.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: P10. Remove redundant code.

The same checks are performed in the PDMFactory::SupportsMimeType

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8670218 [details] [diff] [review]
P8. Have PDMFactory directly manage the EMEDecoderModule.

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

::: dom/media/platforms/PDMFactory.cpp
@@ +177,5 @@
>  
>  bool
>  PDMFactory::SupportsMimeType(const nsACString& aMimeType)
>  {
> +  if (mEMEPDM) {

This means if we have assigned a CDMProxy, we'll end up querying the EME PDM if it supports the mime type for unencrypted streams playing in the same reader. But we really want to be asking whether the PDMs in mCurrentPDMs can handle this type, as that's from where we'll create the decoder.

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #12)
> Comment on attachment 8670218 [details] [diff] [review]
> P8. Have PDMFactory directly manage the EMEDecoderModule.
> 
> Review of attachment 8670218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/PDMFactory.cpp
> @@ +177,5 @@
> >  
> >  bool
> >  PDMFactory::SupportsMimeType(const nsACString& aMimeType)
> >  {
> > +  if (mEMEPDM) {
> 
> This means if we have assigned a CDMProxy, we'll end up querying the EME PDM
> if it supports the mime type for unencrypted streams playing in the same
> reader. But we really want to be asking whether the PDMs in mCurrentPDMs can
> handle this type, as that's from where we'll create the decoder.

Yes, kind of.

It's up to EMEDecoderModule::SupportsMimeType to do the right thing if it knows it's going to pass the data back to a PDMFactory.

We have no way of telling with the current structure who (or what) is going to end up creating the decoder: the CDM or back to the PDMFactory. Hence my "TODO" comment on EMEDecoderModule::SupportsMimeType

Currently the EMEDecoderModule::SupportsMimeType will return true for h264 and aac which are the only two codecs we'll ever use through EME at this stage.

Note that the problem is already there and always has been. We've always queried the EMEDecoderModule to find out what it supported, and it always said true for H264 and AAC regardless of the stream being encrypted or not.

If the stream is encrypted, we don't call back the PDMFactory to create the decoder, we use the CDM's one.

Ideally, we should be passing a TrackInfo object to SupportsMimeType so we can answer more accurately the question.

Comment 14

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97683d8c9c6f

all green

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: P11. Don't rely on SupportsMimeType to determine if a track can be played.

The PDMFactory will run more accurate checks based on the TrackInfo object and will fail to create a decoder if the type is unsupported. So use that instead

This also fixes things like encrypted H264 track but clear MP3.

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: P12. Properly shutdown all created test decoders.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: P13. Assert that data fed to EMEDecoderModule is encrypted.

The PDMFactory ensures that the EMEDecoderModule is only used for encrypted data, we can simplify EMEDecoderModule and make strong assumptions

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: P12. Properly shutdown all created test decoders.

I had missed one

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: P14. Remove obsolete / redundant code.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8670218 [details] [diff] [review]
P8. Have PDMFactory directly manage the EMEDecoderModule.

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

The status quo marches on.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8670219 [details] [diff] [review]
P9. Ensure PDMs are only ever created through the PDMFactory.

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

::: dom/media/platforms/PDMFactory.h
@@ +17,2 @@
>  public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PDMFactory)

Note: this does the MOZ_COUNT_CTOR/DTOR for you, so you don't need them in the constructor/destructor any more (you added them in the previous patch).

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8670220 [details] [diff] [review]
P10. Remove redundant code.

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

::: dom/media/fmp4/MP4Decoder.cpp
@@ +179,5 @@
>  /* static */
>  bool
>  MP4Decoder::IsEnabled()
>  {
> +  return Preferences::GetBool("media.fragmented-mp4.enabled");

We should rename "media.fragmented-mp4.enabled" to "media.mp4.enabled" at some stage.

Comment 23

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #21)
> Comment on attachment 8670219 [details] [diff] [review]
> P9. Ensure PDMs are only ever created through the PDMFactory.
> 
> Review of attachment 8670219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/PDMFactory.h
> @@ +17,2 @@
> >  public:
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PDMFactory)
> 
> Note: this does the MOZ_COUNT_CTOR/DTOR for you, so you don't need them in
> the constructor/destructor any more (you added them in the previous patch).

Yes, I actually removed it since (I had PDMFactory not being refcounted earlier on). Not sure why you got to review the patch with this those macros in.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/041418a07ae5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d095569f6ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0de7f62202
https://hg.mozilla.org/integration/mozilla-inbound/rev/042959216393
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb10d09015e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c58afbd6c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d66be0e4547f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2d524a9b35
https://hg.mozilla.org/integration/mozilla-inbound/rev/696ecf2e2947
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4e91de99867
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f5cff5aa12
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9ddc78b394
https://hg.mozilla.org/integration/mozilla-inbound/rev/dec7c35469d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b1b076a386

Comment 25

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/005264192a61

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4190ef9edc26
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4538d79cc92
https://hg.mozilla.org/integration/mozilla-inbound/rev/27e6561dff6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/835379dc92bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6baf3e3d5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aba22296221
https://hg.mozilla.org/integration/mozilla-inbound/rev/884f650d3e75
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa313561756e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cac2b7df9ea5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae99d2aca89a
https://hg.mozilla.org/integration/mozilla-inbound/rev/158dd058323a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cee24861b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/72180de4fbb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0262c24115
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c92ba05c3ba

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: P15. Fix FFmpeg shutdown crash should decoder not be initialised.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4190ef9edc26
https://hg.mozilla.org/mozilla-central/rev/d4538d79cc92
https://hg.mozilla.org/mozilla-central/rev/27e6561dff6f
https://hg.mozilla.org/mozilla-central/rev/835379dc92bf
https://hg.mozilla.org/mozilla-central/rev/1f6baf3e3d5d
https://hg.mozilla.org/mozilla-central/rev/1aba22296221
https://hg.mozilla.org/mozilla-central/rev/884f650d3e75
https://hg.mozilla.org/mozilla-central/rev/fa313561756e
https://hg.mozilla.org/mozilla-central/rev/cac2b7df9ea5
https://hg.mozilla.org/mozilla-central/rev/ae99d2aca89a
https://hg.mozilla.org/mozilla-central/rev/158dd058323a
https://hg.mozilla.org/mozilla-central/rev/f6cee24861b3
https://hg.mozilla.org/mozilla-central/rev/72180de4fbb6
https://hg.mozilla.org/mozilla-central/rev/5d0262c24115
https://hg.mozilla.org/mozilla-central/rev/5c92ba05c3ba

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8670789 [details] [diff] [review]
P15. Fix FFmpeg shutdown crash should decoder not be initialised.

Approval Request Comment
[Feature/regressing bug #]: Making FFmpeg support more robust
[User impact if declined]: Potential crash with some version of FFmpeg
[Describe test coverage new/current, TreeHerder]: in central for a few weeks.
[Risks and why]: None; just preventing a null deref by first testing value
[String/UUID change made/needed]: None

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8670789 [details] [diff] [review]
P15. Fix FFmpeg shutdown crash should decoder not be initialised.

OK to uplift, preparation to enable ffmpeg.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

(Just uplifting patch #15 here).

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/86872269b331