Make codecs/mimetypes supported by MP4Decoder/Reader more accurate

RESOLVED FIXED in mozilla34

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 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)

(Assignee)

Description

3 years ago
MP4Decoder/DecoderTraits doesn't accurately report the H.264/AAC codecs string that the MP4Reader can play accurately.
(Assignee)

Comment 1

3 years ago
Created attachment 8480186 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5b56644f1d10
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/25d79d686cc8
https://hg.mozilla.org/mozilla-central/rev/25d79d686cc8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.