Make codecs/mimetypes supported by MP4Decoder/Reader more accurate

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks 2 bugs)

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

MP4Decoder/DecoderTraits doesn't accurately report the H.264/AAC codecs string that the MP4Reader can play accurately.
Posted patch PatchSplinter Review
Split out the WMF H.264 codecs string parsing code into a helper, and use it in MP4Decoder as well. We don't do anything clever to ensure it's super accurate, we rely on the WMF documentation to tell us what profiles/levels the WMF decoders can play.

We just assume that every other backend can play what the WMF backend can play for now since we don't actually have any documentation about what any other platform can play... 

Ideally we'd ask the PDMs in a static function, but the ffmpeg's template heavy approach makes that hard, so I just hardcoded the supported codecs for now...
Attachment #8480186 - Flags: review?(kinetik)
Comment on attachment 8480186 [details] [diff] [review]
Patch

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

::: content/media/VideoUtils.h
@@ +219,5 @@
> +  H264_PROFILE_UNKNOWN                     = 0,
> +  H264_PROFILE_BASE                        = 66,
> +  H264_PROFILE_MAIN                        = 77,
> +  H264_PROFILE_EXTENDED                    = 88,
> +  H264_PROFILE_HIGH                        = 100,

Since these and (the levels) appear in the MIME types in base 16, how do you feel about declaring them in base 16 here?  It makes it slightly easier to eyeball when looking at the MIME strings.

@@ +224,5 @@
> +};
> +
> +enum H264_LEVEL {
> +    H264_LEVEL_1         = 10,
> +    H264_LEVEL_1_b       = 11,    

Trailing space.

::: content/media/test/can_play_type_mpeg.js
@@ +50,5 @@
> +  check("audio/mp4; codecs=\"mp4a.40.5\"", "probably");
> +  check("audio/mp4; codecs=mp4a.40.5", "probably");
> +  check("audio/x-m4a; codecs=\"mp4a.40.5\"", "probably");
> +  check("audio/x-m4a; codecs=mp4a.40.5", "probably");
> +  

Space.
Attachment #8480186 - Flags: review?(kinetik) → review+
Comment on attachment 8480186 [details] [diff] [review]
Patch

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

::: content/media/DecoderTraits.cpp
@@ +380,5 @@
>  DecoderTraits::CanHandleMediaType(const char* aMIMEType,
>                                    bool aHaveRequestedCodecs,
>                                    const nsAString& aRequestedCodecs)
>  {
> +  MOZ_ASSERT(aHaveRequestedCodecs ? !aRequestedCodecs.IsEmpty()

This assertion fails, as someone can specify codecs="". Some of the other backends fail if we assume aHaveRequestedCodecs = !aRequestedCodecs.IsEmtpy().

So I'll have to remove this assertion.

https://tbpl.mozilla.org/?tree=Try&rev=a86161a1e8ef
https://hg.mozilla.org/mozilla-central/rev/25d79d686cc8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.