All users were logged out of Bugzilla on October 13th, 2018

canPlayType("audio/mpeg") returns positive on Windows N

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Windows N doesn't ship with WMF or a DirectShow MP3 decoder filter, so it's not gonna work. Yet we report it does because DecoderTraits just checks whether MP3Decoder::IsEnabled(), and that just checks media.mp3.enabled.
(Assignee)

Comment 1

3 years ago
Err, that's not right, we think DirectShow is installed because we check the DirectShow pref. MP3Decoder is only enabled on Android currently.
(Assignee)

Comment 2

3 years ago
Created attachment 8666532 [details] [diff] [review]
Patch: Check whether DirectShow can decode MP3 before assuming it will work.

This means Windows N variants no longer report that they support MP3 when actually they don't.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #8666532 - Flags: review?(jyavenard)
Comment on attachment 8666532 [details] [diff] [review]
Patch: Check whether DirectShow can decode MP3 before assuming it will work.

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

Not knowing this code ; but assuming it's all been verified :)

We should do the same for plain MP3 for when the media.mp3.enabled is true (and we should make this a priority so we don't even need DirectShow anymore) ; and similar to the AAC test in MP4Decoder

::: dom/media/directshow/DirectShowUtils.cpp
@@ +281,5 @@
> +                              nullptr,
> +                              CLSCTX_INPROC_SERVER,
> +                              IID_IBaseFilter,
> +                              getter_AddRefs(filter)))) {
> +    return false;                           

whitespace

@@ +283,5 @@
> +                              IID_IBaseFilter,
> +                              getter_AddRefs(filter)))) {
> +    return false;                           
> +  }
> +  

whitespace

@@ +293,5 @@
> +                              getter_AddRefs(filter))) &&
> +      FAILED(CreateMP3DMOWrapperFilter(getter_AddRefs(filter)))) {
> +    return false;
> +  }
> +  

whitespace
Attachment #8666532 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 6

3 years ago
Backed out, so don't close on merge of backout to m-c.
Keywords: leave-open
(Assignee)

Comment 7

3 years ago
Comment on attachment 8666532 [details] [diff] [review]
Patch: Check whether DirectShow can decode MP3 before assuming it will work.

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

::: dom/media/directshow/DirectShowUtils.cpp
@@ +241,5 @@
>      // expected error.
>      WARN("dmoWrapper Init failed, hr=%x", hr);
>      return hr;
>    }
>  

I needed to add here:

  filter.forget(aOutFilter);
(Assignee)

Updated

3 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/107f8e63982d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1209410
Blocks: 1210667
You need to log in before you can comment on or make changes to this bug.