Ensure WMFDecoderModule::SupportsMimeType checks it can create decoders

RESOLVED FIXED in Firefox 44

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments)

WMFDecoderModule::SupportsMimeType() should check that it can create decoders before reporting that it supports a codec mimeType.
The MP4Decoder and WMF specific bits.
Attachment #8669528 - Flags: review?(jyavenard)
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>
Attachment #8669528 - Flags: review?(jyavenard) → review+
Blocks: 1208995
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)
(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.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15177504&repo=mozilla-inbound
Flags: needinfo?(cpearce)
Priority: -- → P2
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
Flags: needinfo?(cpearce)
Attachment #8670067 - Flags: review?(jyavenard)
Attachment #8670067 - Flags: review?(jyavenard) → review+
Depends on: 1212412
Duplicate of this bug: 1234157
You need to log in before you can comment on or make changes to this bug.