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

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Err, that's not right, we think DirectShow is installed because we check the DirectShow pref. MP3Decoder is only enabled on Android currently.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 3

[Jean-Yves Avenard [:jya]]

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

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f73b03732fc

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8c6a231e79

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Backed out, so don't close on merge of backout to m-c.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/107f8e63982d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/107f8e63982d