Allow PlatformDecoderModule fallback

RESOLVED FIXED in Firefox 43

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 2 bugs)

43 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed, firefox44 fixed)

Details

Attachments

(15 attachments, 1 obsolete attachment)

1.76 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
29.04 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
9.16 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
11.76 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
7.60 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
4.64 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
29.20 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
15.47 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
10.99 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.03 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.61 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.74 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.98 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.08 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
898 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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)
(Assignee)

Updated

3 years ago
Depends on: 1206979
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard
Priority: -- → P1
Blocks: 1210231
(Assignee)

Updated

3 years ago
Depends on: 1211335
(Assignee)

Comment 1

3 years ago
Created attachment 8670022 [details] [diff] [review]
P1. Remove unused PDM function members.
Attachment #8670022 - Flags: review?(cpearce)
(Assignee)

Comment 2

3 years ago
Created attachment 8670023 [details] [diff] [review]
P2. Wrap PDM creation in a new PDMFactory class.

There is no change of behaviour from the original PlatformDecoderModule.
Attachment #8670023 - Flags: review?(cpearce)
(Assignee)

Comment 3

3 years ago
Created attachment 8670024 [details] [diff] [review]
P3. Allow PDM fallback.

We now search in all the PDM present the one that can handle the media.
Attachment #8670024 - Flags: review?(cpearce)
(Assignee)

Comment 4

3 years ago
Created attachment 8670025 [details] [diff] [review]
P4. Add AgnosticDecoderModule object.

This removes the need for PDMFactory to know anything about decoders.
Attachment #8670025 - Flags: review?(cpearce)
(Assignee)

Comment 5

3 years ago
Created attachment 8670026 [details] [diff] [review]
P5. Update PlatformDecoderModule documentation.

Mostly removes no longer relevant doc.
Attachment #8670026 - Flags: review?(cpearce)
(Assignee)

Comment 6

3 years ago
Created attachment 8670027 [details] [diff] [review]
P6. Make PlatformDecoderModule::SupportsMimeType pure virtual.
Attachment #8670027 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Blocks: 1211731
Attachment #8670022 - Flags: review?(cpearce) → review+
Attachment #8670023 - Flags: review?(cpearce) → review+
Attachment #8670024 - Flags: review?(cpearce) → review+
Attachment #8670025 - Flags: review?(cpearce) → review+
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!
Attachment #8670026 - Flags: review?(cpearce) → review+
Attachment #8670027 - Flags: review?(cpearce) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8670092 [details] [diff] [review]
[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.
Attachment #8670092 - Flags: review?(kinetik)
Attachment #8670092 - Flags: review?(kinetik) → review+
Blocks: 1101885
(Assignee)

Comment 9

3 years ago
Created attachment 8670218 [details] [diff] [review]
P8. Have PDMFactory directly manage the EMEDecoderModule.
Attachment #8670218 - Flags: review?(cpearce)
(Assignee)

Comment 10

3 years ago
Created attachment 8670219 [details] [diff] [review]
P9. Ensure PDMs are only ever created through the PDMFactory.
Attachment #8670219 - Flags: review?(cpearce)
(Assignee)

Comment 11

3 years ago
Created attachment 8670220 [details] [diff] [review]
P10. Remove redundant code.

The same checks are performed in the PDMFactory::SupportsMimeType
Attachment #8670220 - Flags: review?(cpearce)
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.
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Comment 15

3 years ago
Created attachment 8670548 [details] [diff] [review]
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.
(Assignee)

Comment 16

3 years ago
Created attachment 8670549 [details] [diff] [review]
P12. Properly shutdown all created test decoders.
Attachment #8670549 - Flags: review?(cpearce)
(Assignee)

Comment 17

3 years ago
Created attachment 8670550 [details] [diff] [review]
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
Attachment #8670550 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8670548 - Flags: review?(cpearce)
(Assignee)

Comment 18

3 years ago
Created attachment 8670551 [details] [diff] [review]
P12. Properly shutdown all created test decoders.

I had missed one
Attachment #8670551 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8670549 - Attachment is obsolete: true
Attachment #8670549 - Flags: review?(cpearce)
(Assignee)

Comment 19

3 years ago
Created attachment 8670555 [details] [diff] [review]
P14. Remove obsolete / redundant code.
Attachment #8670555 - Flags: review?(cpearce)
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.
Attachment #8670218 - Flags: review?(cpearce) → review+
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).
Attachment #8670219 - Flags: review?(cpearce) → review+
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.
Attachment #8670220 - Flags: review?(cpearce) → review+
(Assignee)

Comment 23

3 years ago
(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.
Attachment #8670548 - Flags: review?(cpearce) → review+
Attachment #8670550 - Flags: review?(cpearce) → review+
Attachment #8670551 - Flags: review?(cpearce) → review+
Attachment #8670555 - Flags: review?(cpearce) → review+
(Assignee)

Comment 27

3 years ago
Created attachment 8670789 [details] [diff] [review]
P15. Fix FFmpeg shutdown crash should decoder not be initialised.
(Assignee)

Updated

3 years ago
Blocks: 1214943
(Assignee)

Comment 29

3 years ago
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
Attachment #8670789 - Flags: approval-mozilla-aurora?
status-firefox43: --- → affected
Comment on attachment 8670789 [details] [diff] [review]
P15. Fix FFmpeg shutdown crash should decoder not be initialised.

OK to uplift, preparation to enable ffmpeg.
Attachment #8670789 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Just uplifting patch #15 here).
You need to log in before you can comment on or make changes to this bug.