Allow PlatformDecoderModule fallback

RESOLVED FIXED in Firefox 43

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 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

4 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

4 years ago
Depends on: 1206979
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
(Assignee)

Updated

4 years ago
Depends on: 1211335
(Assignee)

Comment 1

4 years ago
Attachment #8670022 - Flags: review?(cpearce)
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Updated

4 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

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

Comment 11

4 years ago
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

4 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

4 years ago
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

4 years ago
Attachment #8670549 - Flags: review?(cpearce)
(Assignee)

Comment 17

4 years ago
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

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

Comment 18

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

Updated

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

Comment 19

4 years ago
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

4 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)

Updated

4 years ago
Blocks: 1214943
(Assignee)

Comment 29

4 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?
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.