1211339 - Ensure WMFDecoderModule::SupportsMimeType checks it can create decoders

Status: RESOLVED FIXED

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

Description

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

WMFDecoderModule::SupportsMimeType() should check that it can create decoders before reporting that it supports a codec mimeType.

Comment 1

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

Attachment: Patch: Ensure WMFDecoderModule::SupportsMimeType checks it can create decoders.

The MP4Decoder and WMF specific bits.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8669528 [details] [diff] [review]
Patch: Ensure WMFDecoderModule::SupportsMimeType checks it can create decoders.

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

r=me with comments:
I think this patch should be split in two.
The changes to the PDM itself (which is what causes the dependency to bug 1211335) and the changes to MP4Decoders and VideoUtils / DecoderTraits in the other.

::: dom/media/fmp4/MP4Decoder.cpp
@@ +98,5 @@
>  }
>  
>  /* static */
>  bool
>  MP4Decoder::CanHandleMediaType(const nsACString& aType,

should change the prototype here to be consistent with the header.

@@ +145,5 @@
> +        continue;
> +      }
> +  #ifndef MOZ_GONK_MEDIACODEC
> +      // Everything except B2G supports MP3 in MP4.
> +      if (codec.EqualsLiteral("mp3")) {

this is more than likely no longer true (and has it ever been true?). MP4 or not is irrelevant as far the the PDM is concerned; it only deals with codecs

::: dom/media/platforms/wmf/WMFDecoderModule.cpp
@@ +138,5 @@
> +template<const GUID& aGuid>
> +static bool
> +CanCreateWMFDecoder()
> +{
> +  static bool cachedResult = false;

would be more elegant with a Maybe<bool>

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8669528 [details] [diff] [review]
Patch: Ensure WMFDecoderModule::SupportsMimeType checks it can create decoders.

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

::: dom/media/fmp4/MP4Decoder.cpp
@@ +53,5 @@
>    return new MediaDecoderStateMachine(this, reader);
>  }
>  
>  static bool
> +IsWhitelistedAudioCodec(const nsAString& aCodec)

this is unused and gives me compilation warning (unused functions cause static analysis failure on try)

Comment 4

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

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8669528 [details] [diff] [review]
> @@ +145,5 @@
> > +        continue;
> > +      }
> > +  #ifndef MOZ_GONK_MEDIACODEC
> > +      // Everything except B2G supports MP3 in MP4.
> > +      if (codec.EqualsLiteral("mp3")) {
> 
> this is more than likely no longer true (and has it ever been true?). MP4 or
> not is irrelevant as far the the PDM is concerned; it only deals with codecs

GonkDecoderModule::SupportsMimeType() returns true for audio/mpeg. So we can probably not have the #ifdef guard here yes.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b446e382b98

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d34f694b13cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/91bff7bc67d7

Comment 7

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15177504&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3f18ef6b5ce

Comment 9

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

Attachment: Patch 3: Refuse to support video codecs inside MP4 audio mime type

We're failing web-platform tests because we're not returning negative for 'audio/mp4;codecs="avc1.4d001e"', i.e. a video codec in an audio container. This patch makes us return failure in that case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=920b4557c62b

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b374a409750b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf49f4d6c83c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe8d3254ccd

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b374a409750b
https://hg.mozilla.org/mozilla-central/rev/cf49f4d6c83c
https://hg.mozilla.org/mozilla-central/rev/dbe8d3254ccd