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

RESOLVED FIXED in Firefox 48

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: cpearce, Assigned: gerald)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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)
(Reporter)

Comment 3

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

Updated

3 years ago
See Also: → bug 1180108
Blocks: 1180108
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
Priority: -- → P2
Gerald said he is working on Decoder Doctor for Windows.
Assignee: cpearce → gsquelart
(Assignee)

Comment 8

2 years ago
This WMF-specific task depends on generic infrastructure work done in bug 1248507.
Depends on: 1248507
(Assignee)

Comment 9

2 years ago
Created attachment 8740761 [details] [diff] [review]
848994-WMF-failure.patch

Detect and report when WMF is not found.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6f3c0cdd1eb989cf3316ceb0783719b2a74d2b9
Attachment #8740761 - Flags: review?(cpearce)
(Assignee)

Comment 10

2 years ago
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)
(Reporter)

Updated

2 years ago
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 Microsoft’s Media Feature Pack.
> +MediaWMFRecommended=To improve video quality, you may need to install Microsoft’s 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?
(Assignee)

Comment 13

2 years ago
(In reply to Chris Peterson [:cpeterson] from comment #12)
> > +MediaWMFRecommended=To improve video quality, you may need to install Microsoft’s 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 Microsoft’s Media Feature Pack.
> +MediaWMFRecommended=To improve video quality, you may need to install Microsoft’s 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.
(Assignee)

Comment 16

2 years ago
(In reply to Jared Wein [:jaws] from comment #15)
Fixed & fixed. Thank you for checking.
(Assignee)

Updated

2 years ago
Attachment #8740761 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8743878 [details]
MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r=cpearce

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)
(Assignee)

Comment 18

2 years ago
Created attachment 8743879 [details]
MozReview Request: Bug 848994 - p2. Detect when WMF is not found - r=cpearce

Review commit: https://reviewboard.mozilla.org/r/48147/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48147/
(Assignee)

Comment 19

2 years ago
Created attachment 8743880 [details]
MozReview Request: Bug 848994 - p3. Check MediaKeySystem requests - r?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/
(Assignee)

Comment 20

2 years ago
Created attachment 8743881 [details]
MozReview Request: Bug 848994 - p4. Record GMP diagnostics - r?cpearce

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/
(Assignee)

Comment 21

2 years ago
Created attachment 8743882 [details]
MozReview Request: Bug 848994 - p5. Check Silverlight presence - r?cpearce

Review commit: https://reviewboard.mozilla.org/r/48153/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48153/
(Assignee)

Comment 22

2 years ago
Created attachment 8743883 [details]
MozReview Request: Bug 848994 - p6. Analyze Windows issues - r?cpearce

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/
(Assignee)

Comment 23

2 years ago
Created attachment 8743884 [details]
MozReview Request: Bug 848994 - p7. Filter front-end notifications - r?cpearce

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/
(Assignee)

Comment 24

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

Comment 25

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

Comment 26

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

Comment 27

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

Comment 28

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

Comment 29

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

Comment 30

2 years ago
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/
(Reporter)

Updated

2 years ago
Attachment #8743878 - Flags: review?(cpearce) → review+
(Reporter)

Comment 31

2 years ago
Comment on attachment 8743878 [details]
MozReview Request: Bug 848994 - p1. Refactor Decoder Doctor - r=cpearce

https://reviewboard.mozilla.org/r/48145/#review44945
(Reporter)

Comment 32

2 years ago
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+
(Reporter)

Updated

2 years ago
Attachment #8743880 - Flags: review?(cpearce) → review+
(Reporter)

Comment 33

2 years ago
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.
(Reporter)

Comment 34

2 years ago
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+
(Reporter)

Comment 35

2 years ago
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+
(Reporter)

Comment 36

2 years ago
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?
(Reporter)

Comment 37

2 years ago
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+
(Reporter)

Comment 38

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

Comment 39

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

Comment 40

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

Comment 41

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

Comment 42

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

Comment 43

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

Comment 44

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

Comment 45

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

Comment 46

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

Comment 47

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

Comment 48

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

Comment 49

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

Comment 50

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

Updated

2 years ago
Depends on: 1288585

Updated

2 months ago
Depends on: 1467495
You need to log in before you can comment on or make changes to this bug.