Closed Bug 848994 Opened 11 years ago Closed 8 years ago

[Decoder Doctor] Detect codec problems and tell front-end to notify user to install "Windows Media Feature Pack" if required for WMF playback

Categories

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

All
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: cpearce, Assigned: mozbugz)

References

Details

Attachments

(7 files, 1 obsolete file)

58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Some versions of Windows need to have a "Media Feature Pack" installed in order for Windows Media Foundation to be present. These are usually the "N and KN" versions of Enterprise and Server variants of Windows.

We should try to detect when this is the case, and prompt the user to install the media feature pack.

See also: bug 848562, bug 847879.
We still need this after we implanted OpenH264 video codec in bug #948160 or after bug #1057646 will be finalized?

More info what Media Feature Packs for Windows N and KN versions contains:
Win 8.1 - http://support.microsoft.com/kb/2929699
Win 8 - http://support.microsoft.com/kb/2703761
Win 7 - http://support.microsoft.com/kb/968211
Flags: needinfo?(cpearce)
Yes. OpenH264 does not do H.264 Main or High profile, and it does not support the audio codec commonly streamed alongside H.264; AAC. So even with OpenH264 we can't play MP4 videos.
Flags: needinfo?(cpearce)
Blocks: 1160424
Summary: Prompt user to install "Windows Media Feature Pack" if required for WMF playback → [WMF UI] Prompt user to install "Windows Media Feature Pack" if required for WMF playback
Summary: [WMF UI] Prompt user to install "Windows Media Feature Pack" if required for WMF playback → [WMF UI] Detect codec problems and tell front-end to notify user to install "Windows Media Feature Pack" if required for WMF playback
For our MVP, we will only check for the Windows N/KN editions that are missing codecs *and* do not have Flash installed (as a fallback).
Summary: [WMF UI] Detect codec problems and tell front-end to notify user to install "Windows Media Feature Pack" if required for WMF playback → [Decoder Doctor] Detect codec problems and tell front-end to notify user to install "Windows Media Feature Pack" if required for WMF playback
Component: Audio/Video → Audio/Video: Playback
cpearce has volunteered to work on the Platform bits of the "Decoder Doctor" feature.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
OS: Windows 8 → Windows 7
Hardware: x86_64 → All
Gerald said he is working on Decoder Doctor for Windows.
Assignee: cpearce → gsquelart
This WMF-specific task depends on generic infrastructure work done in bug 1248507.
Depends on: 1248507
Comment on attachment 8740761 [details] [diff] [review]
848994-WMF-failure.patch

Boris, another quick review please (after bug 1248507), just for an added enum value in the webidl at the bottom of the patch.
Thank you.
Attachment #8740761 - Flags: review?(bzbarsky)
Attachment #8740761 - Flags: review?(cpearce) → review+
Comment on attachment 8740761 [details] [diff] [review]
848994-WMF-failure.patch

r=me
Attachment #8740761 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740761 [details] [diff] [review]
848994-WMF-failure.patch

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +107,5 @@
>  MediaLoadUnsupportedMimeType=HTTP "Content-Type" of "%1$S" is not supported. Load of media resource %2$S failed.
>  # LOCALIZATION NOTE: %S is the URL of the media resource which failed to load because of error in decoding.
>  MediaLoadDecodeError=Media resource %S could not be decoded.
> +MediaWFMNeeded=To play video, you may need to install Microsofts Media Feature Pack.
> +MediaWMFRecommended=To improve video quality, you may need to install Microsofts Media Feature Pack.

I think you have inadvertently inserted a UTF-16 0x0092 smart quote instead of UTF-8. None of the other strings in this file appear to use smart quotes. Do we have a UX style guide that has anything to say on quotes?
(In reply to Chris Peterson [:cpeterson] from comment #12)
> > +MediaWMFRecommended=To improve video quality, you may need to install Microsofts Media Feature Pack.
> I think you have inadvertently inserted a UTF-16 0x0092 smart quote instead
> of UTF-8. None of the other strings in this file appear to use smart quotes.
> Do we have a UX style guide that has anything to say on quotes?

Good catch! I copied the strings from a word-processor document, that must be why. I'll fix that of course.
Comment on attachment 8740761 [details] [diff] [review]
848994-WMF-failure.patch

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +107,5 @@
>  MediaLoadUnsupportedMimeType=HTTP "Content-Type" of "%1$S" is not supported. Load of media resource %2$S failed.
>  # LOCALIZATION NOTE: %S is the URL of the media resource which failed to load because of error in decoding.
>  MediaLoadDecodeError=Media resource %S could not be decoded.
> +MediaWFMNeeded=To play video, you may need to install Microsofts Media Feature Pack.
> +MediaWMFRecommended=To improve video quality, you may need to install Microsofts Media Feature Pack.

Since bug 1259847 we should be using curly quotes in our strings.

Also, your string name for MediaWFMNeeded is wrong. It should be MediaWMFNeeded.
(In reply to Jared Wein [:jaws] from comment #15)
Fixed & fixed. Thank you for checking.
Attachment #8740761 - Attachment is obsolete: true
Some refactoring, clean-ups, etc.

The main change is that the can-play status is passed when diagnostics are
finally recorded. This will help when introducing different types of
diagnostics in future patches (e.g., Key System issues).

Review commit: https://reviewboard.mozilla.org/r/48145/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48145/
Attachment #8743878 - Flags: review?(cpearce)
Attachment #8743879 - Flags: review?(cpearce)
Attachment #8743880 - Flags: review?(cpearce)
Attachment #8743881 - Flags: review?(cpearce)
Attachment #8743882 - Flags: review?(cpearce)
Attachment #8743883 - Flags: review?(cpearce)
Media Key System access requests are now recorded with their success/failure,
as well as accompanying issues of importance.
In this bug we focus on the Widevine-with-no-WMF case.

Review commit: https://reviewboard.mozilla.org/r/48149/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48149/
Record diagnostics information about whether the GMP CDM failed to load
(though currently impossible!), and which GMP is used in the current media
format check.

Review commit: https://reviewboard.mozilla.org/r/48151/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48151/
Analyze the diagnostics information gathered so far, and dispatch
notifications as appropriate.
The most important case in this bug is when Widevine is requested but WMF and
Silverlight are missing.
The generic case of WMF missing (when needed to play) is there too.

the following patch will add filtering to allow/prevent targeted notifications.

Review commit: https://reviewboard.mozilla.org/r/48155/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48155/
Pref "media.decoder-doctor.notifications-allowed" defines which notifications
may be forwarded to the front-end (based on the web-console message ID for
that situation). This allows fine-grained filtering.

The current default is to only notify the user about Widevine requests when WMF
and Silverlight are missing (because Widevine does not include an AAC decoder).

Review commit: https://reviewboard.mozilla.org/r/48157/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48157/
Comment on attachment 8743878 [details]
MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r=cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48145/diff/1-2/
Attachment #8743884 - Attachment description: MozReview Request: Bug 848994 - p7. Filter front-end notifications - c?cpearce → MozReview Request: Bug 848994 - p7. Filter front-end notifications - r?cpearce
Attachment #8743884 - Flags: review?(cpearce)
Comment on attachment 8743879 [details]
MozReview Request: Bug 848994 - p2. Detect when WMF is not found - r=cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48147/diff/1-2/
Comment on attachment 8743880 [details]
MozReview Request: Bug 848994 - p3. Check MediaKeySystem requests - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48149/diff/1-2/
Comment on attachment 8743881 [details]
MozReview Request: Bug 848994 - p4. Record GMP diagnostics - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48151/diff/1-2/
Comment on attachment 8743882 [details]
MozReview Request: Bug 848994 - p5. Check Silverlight presence - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48153/diff/1-2/
Comment on attachment 8743883 [details]
MozReview Request: Bug 848994 - p6. Analyze Windows issues - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48155/diff/1-2/
Comment on attachment 8743884 [details]
MozReview Request: Bug 848994 - p7. Filter front-end notifications - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48157/diff/1-2/
Attachment #8743878 - Flags: review?(cpearce) → review+
Comment on attachment 8743878 [details]
MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r=cpearce

https://reviewboard.mozilla.org/r/48145/#review44945
Comment on attachment 8743879 [details]
MozReview Request: Bug 848994 - p2. Detect when WMF is not found - r=cpearce

https://reviewboard.mozilla.org/r/48147/#review44949
Attachment #8743879 - Flags: review?(cpearce) → review+
Attachment #8743880 - Flags: review?(cpearce) → review+
Comment on attachment 8743880 [details]
MozReview Request: Bug 848994 - p3. Check MediaKeySystem requests - r?cpearce

https://reviewboard.mozilla.org/r/48149/#review44953

::: dom/media/DecoderDoctorDiagnostics.cpp:327
(Diff revision 2)
>          }
>          unplayableFormats += diag.mDecoderDoctorDiagnostics.Format();
>        }
> +    } else if (!diag.mDecoderDoctorDiagnostics.KeySystem().IsEmpty()) {
> +      if (!diag.mDecoderDoctorDiagnostics.IsKeySystemSupported()) {
> +        if (!unsupportedKeySystems.IsEmpty()) {

Optional: I feel like a helper that appends ", " to a string if the string isn't empty may make this code cleaner.

::: dom/media/DecoderDoctorDiagnostics.cpp:482
(Diff revision 2)
> +
> +  mKeySystem = aKeySystem;
> +  mIsKeySystemSupported = aIsSupported;
> +
> +  // StoreDiagnostics should only be called once, after all data is available,
> +  // so it is safe to Move() from this object.

Forgive me if I'm missing something, but is there a way you can assert or enforce that StoreDiagnostics is only called once?

::: dom/media/eme/MediaKeySystemAccessManager.cpp:108
(Diff revision 2)
>                                            aKeySystem,
>                                            MediaKeySystemStatus::Api_disabled);
>      aPromise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR,
>                            NS_LITERAL_CSTRING("EME has been preffed off"));
> +    diagnostics.StoreMediaKeySystemAccess(mWindow->GetExtantDoc(),
> +                                          aKeySystem, false, __func__);

Do we need to store diagnostics in the cases where EME is unsupported for reasons other than missing codecs?

There's already events fired and UI for handling the cases where EME is unsupported for non-codec reasons.
Comment on attachment 8743881 [details]
MozReview Request: Bug 848994 - p4. Record GMP diagnostics - r?cpearce

https://reviewboard.mozilla.org/r/48151/#review44961

::: dom/media/DecoderDoctorDiagnostics.cpp:501
(Diff revision 2)
>      }
>      if (mFFmpegFailedToLoad) {
>        s += ", Linux platform decoder failed to load";
>      }
> +    if (mGMPFailedToStartup) {
> +      s += ", GMP failed to startup";

This does *not* record whether the GMP failed to startup. This records whether the GMP *PDM* failed to startup.

So this string should reflect that. And yes, as you mentioned in the commit message, the GMP PDM can't currently fail to load.

GMP's failing to load typically manifests as a MediaKeys::Init() promise rejecting with "Call to GetGMPDecryptor() failed early", see CDMProxy::gmp_InitGetGMPDecryptor(). At least in the EME case.

::: dom/media/DecoderDoctorDiagnostics.cpp:502
(Diff revision 2)
>      if (mFFmpegFailedToLoad) {
>        s += ", Linux platform decoder failed to load";
>      }
> +    if (mGMPFailedToStartup) {
> +      s += ", GMP failed to startup";
> +    }

} else if (...) {

That is, '}' on the same line as "else".

Or at least be consistent with the rest of the file.
Attachment #8743881 - Flags: review?(cpearce) → review+
Comment on attachment 8743882 [details]
MozReview Request: Bug 848994 - p5. Check Silverlight presence - r?cpearce

https://reviewboard.mozilla.org/r/48153/#review44963

::: dom/media/DecoderDoctorDiagnostics.cpp:309
(Diff revision 2)
> +{
> +  RefPtr<nsPluginHost> pluginHost = nsPluginHost::GetInst();
> +  if (!pluginHost) {
> +    return eNoSilverlight;
> +  }
> +  nsTArray<nsCOMPtr<nsIInternalPluginTag>> plugins;

It's remarkably easier than I expected to figure out what plugins we have!  :)
Attachment #8743882 - Flags: review?(cpearce) → review+
https://reviewboard.mozilla.org/r/48153/#review44965

::: dom/media/DecoderDoctorDiagnostics.cpp:305
(Diff revision 2)
> +  eSilverlightEnabled
> +};
> +static SilverlightPresence
> +CheckSilverlight()
> +{
> +  RefPtr<nsPluginHost> pluginHost = nsPluginHost::GetInst();

At a glance nsPluginHost doesn't look like it's threadsafe. So you probably should be asserting that this function only gets called on the main thread?
Comment on attachment 8743883 [details]
MozReview Request: Bug 848994 - p6. Analyze Windows issues - r?cpearce

https://reviewboard.mozilla.org/r/48155/#review44969

::: dom/locales/en-US/chrome/dom/dom.properties:110
(Diff revision 2)
>  MediaLoadSourceMediaNotMatched=Specified "media" attribute of "%1$S" does not match the environment. Load of media resource %2$S failed.
>  # LOCALIZATION NOTE: %1$S is the MIME type HTTP header being sent by the web server, %2$S is the URL of the media resource which failed to load.
>  MediaLoadUnsupportedMimeType=HTTP "Content-Type" of "%1$S" is not supported. Load of media resource %2$S failed.
>  # LOCALIZATION NOTE: %S is the URL of the media resource which failed to load because of error in decoding.
>  MediaLoadDecodeError=Media resource %S could not be decoded.
> +MediaWidevineNoWMFNoSilverlight=Trying to play Widevine with no MWF nor Silverlight, see https://support.mozilla.org/kb/fix-video-audio-problems-firefox-windows

s/MWF/Windows Media Foundation/

Or at least, s/MWF/WMF/

;)


I think the goal here should be to get people to install the Media Feature Pack, so how about we don't mention Silverlight in the string?

We check for Silverlight, because we assume the site will fallback to Silverlight if Widevine EME doesn't work. If both Widevine EME and Silverlight don't work, the site won't work; it's a dead end. We want to drive people to getting Widevine EME working, *not* Silverlight.

::: dom/media/DecoderDoctorDiagnostics.cpp:324
(Diff revision 2)
>    }
>  
>    return eNoSilverlight;
>  }
>  
> +static void AppendToStringList(nsAString& list, const nsAString& item)

Ah, there's the helper I suggested earlier. Great minds think alike!
Attachment #8743883 - Flags: review?(cpearce) → review+
Comment on attachment 8743884 [details]
MozReview Request: Bug 848994 - p7. Filter front-end notifications - r?cpearce

https://reviewboard.mozilla.org/r/48157/#review44977

::: dom/media/DecoderDoctorDiagnostics.cpp:311
(Diff revision 2)
> +  if (filter.EqualsLiteral("-")) {
> +    // Allow nothing.
> +  } else if (filter.EqualsLiteral("*")) {
> +    allowed = true;
> +  } else for (uint32_t start = 0; start < filter.Length(); ) {
> +    int32_t comma = filter.FindChar(',', start);

You could use SplitAt() from GMPUtils.h here to make this a simpler loop. If you do that, you can move SplitAt() into VideoUtils.h I guess, as it would no longer be a GMP specific utility.

::: modules/libpref/init/all.js:357
(Diff revision 2)
>  pref("media.apple.mp4.enabled", true);
>  #endif
> +
> +// Filter what triggers user notifications.
> +// See DecoderDoctorDocumentWatcher::ReportAnalysis for details.
> +pref("media.decoder-doctor.notifications-allowed", "");

I'd stick 'MediaWidevineNoWMFNoSilverlight' here (and not in the code), so the default is obvious to the place where the behaviour is configured; the pref's value.
Attachment #8743884 - Flags: review?(cpearce) → review+
https://reviewboard.mozilla.org/r/48149/#review44953

> Optional: I feel like a helper that appends ", " to a string if the string isn't empty may make this code cleaner.

As you've noticed later, it's happening in part 6.

> Forgive me if I'm missing something, but is there a way you can assert or enforce that StoreDiagnostics is only called once?

Nothing to forgive, as I did not check -- I thought the normal usage pattern (where creation and storage happen at the same spot) would naturally prevent misuse.
However, better be safe than sorry, so I'm adding a diagnostics type that will both help with store-only-once assertions, and as a bonus will make it easier to determine what's in the diagnostics struct during analysis.

> Do we need to store diagnostics in the cases where EME is unsupported for reasons other than missing codecs?
> 
> There's already events fired and UI for handling the cases where EME is unsupported for non-codec reasons.

As discussed elsewhere, gathering this information may still be useful, and it could end up in a more-verbose web-console messages.
For sure, it will be important to avoid displaying two UX notification drop-down bars!
This won't happen in this bug (as the only MediaKeySystem-related information used is targetted at a situation where there is currently no UX), so I propose that we let that through, but make note of it in follow-up bugs that will deal with these situations with existing UX.
https://reviewboard.mozilla.org/r/48151/#review44961

> This does *not* record whether the GMP failed to startup. This records whether the GMP *PDM* failed to startup.
> 
> So this string should reflect that. And yes, as you mentioned in the commit message, the GMP PDM can't currently fail to load.
> 
> GMP's failing to load typically manifests as a MediaKeys::Init() promise rejecting with "Call to GetGMPDecryptor() failed early", see CDMProxy::gmp_InitGetGMPDecryptor(). At least in the EME case.

Changed variable names and description strings to say "GMP PDM". Yes we'll have to catch actual GMP failures elsewhere.
As discussed we don't actually need this part for this specific bug just yet, but it will be useful later on.
https://reviewboard.mozilla.org/r/48153/#review44965

> At a glance nsPluginHost doesn't look like it's threadsafe. So you probably should be asserting that this function only gets called on the main thread?

Everything in DD is on the main thread. I'll add an assert for peace of mind.
https://reviewboard.mozilla.org/r/48155/#review44969

> s/MWF/Windows Media Foundation/
> 
> Or at least, s/MWF/WMF/
> 
> ;)
> 
> 
> I think the goal here should be to get people to install the Media Feature Pack, so how about we don't mention Silverlight in the string?
> 
> We check for Silverlight, because we assume the site will fallback to Silverlight if Widevine EME doesn't work. If both Widevine EME and Silverlight don't work, the site won't work; it's a dead end. We want to drive people to getting Widevine EME working, *not* Silverlight.

I'm really having trouble with this "WMF" acronym! I'll expand it in the string.

As discussed, this string will only be shown in the web console, which will mostly be seen by more knowledgeable people, and the given SUMO link is all about WMF.
But to reduce emphasis on Silverlight, I'll change its mention to "(nor Silverlight fallback)".
Comment on attachment 8743878 [details]
MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r=cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48145/diff/2-3/
Attachment #8743878 - Attachment description: MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r?cpearce → MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r=cpearce
Attachment #8743879 - Attachment description: MozReview Request: Bug 848994 - p2. Detect when WMF is not found - r?cpearce → MozReview Request: Bug 848994 - p2. Detect when WMF is not found - r=cpearce
Comment on attachment 8743879 [details]
MozReview Request: Bug 848994 - p2. Detect when WMF is not found - r=cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48147/diff/2-3/
Comment on attachment 8743880 [details]
MozReview Request: Bug 848994 - p3. Check MediaKeySystem requests - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48149/diff/2-3/
Comment on attachment 8743881 [details]
MozReview Request: Bug 848994 - p4. Record GMP diagnostics - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48151/diff/2-3/
Comment on attachment 8743882 [details]
MozReview Request: Bug 848994 - p5. Check Silverlight presence - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48153/diff/2-3/
Comment on attachment 8743883 [details]
MozReview Request: Bug 848994 - p6. Analyze Windows issues - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48155/diff/2-3/
Comment on attachment 8743884 [details]
MozReview Request: Bug 848994 - p7. Filter front-end notifications - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48157/diff/2-3/
https://reviewboard.mozilla.org/r/48157/#review44977

> You could use SplitAt() from GMPUtils.h here to make this a simpler loop. If you do that, you can move SplitAt() into VideoUtils.h I guess, as it would no longer be a GMP specific utility.

If you don't mind, I'd like to skip this (non-issue) change right now, and come back to it in a follow-up.
I would like to use SplitAt, and in fact I would argue it should be made even more available across the project. I can't believe it's not already needed and/or reinvented in multiple places!
Depends on: 1467495
You need to log in before you can comment on or make changes to this bug.