838331 - Audio's canPlayType() fails if codec is supplied

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ian Gilman [:iangilman]]

(new Audio()).canPlayType('audio/mpeg; codecs="mp3"') on Android Firefox 18 returns "".

(new Audio()).canPlayType('audio/mpeg') on the same browser returns "maybe".

Playing an MP3 works.

Expected: The codecs part of the string should be supported.

Comment 1

[Paul Adenot (:padenot)]

See [1] and [2] for the rationale behind this behaviour, which is correct per spec.

The HTML spec says: 

> Generally, a user agent should never return "probably" for a type that allows the codecs 
> parameter if that parameter is not present.

It does not say whether we should return "" or something else for a type that does not allow codec parameters and when a codec parameter is present. Maybe we should fix the spec.

[1]: http://wiki.whatwg.org/wiki/Video_type_parameters#MPEG
[2]: http://www.rfc-editor.org/rfc/rfc3003.txt

Comment 2

[Ian Gilman [:iangilman]]

Yes, seems like a hole in the spec. Insomuch as the spec doesn't specify what to return when the codec is present, it seems returning "" is misleading; we do after all support MP3.

For what it's worth, Chrome, Safari, and IE all return in the affirmative when you give the codec for MP3 (Chrome and Safari give "maybe" and IE9 gives "probably"). I built this jsbin to test: http://jsbin.com/enaheq/1

Comment 3

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

I think we should change our canPlayType implementation to report affirmatively when 'codecs="mp3"' is present for MP3, and get the spec changed. Other implementations are doing this, and people keep filing bugs because they expect this behaviour.

Comment 4

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

I have a patch in bug 841239 to change our canPlayType behaviour to on Windows 7 and later to support mp3 as discussed here. A number of sites were being caught out by this.

This change needs to be made on a per platform basis.

Comment 5

[Ian Gilman [:iangilman]]

Any thoughts on who might be the best person to do the Android version of this fix?

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Ian Gilman [:iangilman] from comment #5)
> Any thoughts on who might be the best person to do the Android version of
> this fix?

Probably Chris Double?

Comment 7

[Ian Gilman [:iangilman]]

Cool, thanks!

How about Firefox OS? Is that the same code branch?

Comment 8

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

Unfortunately no, we'll need a separate patch for each platform to get behaviour to align on every platform we support. :(

Comment 9

[cajbir (:cajbir)]

> Generally, a user agent should never return "probably" for a type that allows the codecs 
> parameter if that parameter is not present.

Technically audio/mpeg is not a type that allows the codecs parameter so this statement doesn't actually apply.

<http://wiki.whatwg.org/wiki/Video_type_parameters>

Comment 10

[Ian Gilman [:iangilman]]

:doublec, it's a hole in the spec, it's misleading, and it's out of step with all the other browsers. See comments #3 and #4. While fixing this we should fix the spec as well.

:cpearce, who might it be for Firefox OS?

Comment 11

[cajbir (:cajbir)]

(In reply to Ian Gilman [:iangilman] from comment #10)
> 
> :cpearce, who might it be for Firefox OS?

I would suggest Chris Pearce since he's done it already for a platform and knows what's involved.  Bug 841239 probably should have been a cross platform fix in hindsite.

Comment 12

[Ian Gilman [:iangilman]]

I imagine the trouble is knowing where in the code to make the change for each platform, and then knowing how to build and test that platform. I'd be happy to propose a patch based on Bug 841239 if someone could point me to the right spot in the code for the Android and Firefox OS platforms.

Comment 14

[Marco Chen [:mchen]]

(In reply to Ian Gilman [:iangilman] from comment #12)
> right spot in the code for the Android and Firefox OS platforms.

Hi, 
For Firefox OS, you can refer to the link as below. Currently the codec list is set for h.264 directly.

https://hg.mozilla.org/mozilla-central/file/e58336e81395/content/media/DecoderTraits.cpp#l339

Comment 15

[Ian Gilman [:iangilman]]

Attachment: Fix for Firefox OS; adds MP3 to valid codecs

Marco, thanks for the pointer. This patch seems like it would do the trick; what do you think?

And this file is just for Firefox OS? Or is it used on other platforms?

Comment 16

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

Comment on attachment 755561 [details] [diff] [review]
Fix for Firefox OS; adds MP3 to valid codecs

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

This will do it on B2G, but with this patch we report that we can play H.264+MP3, which I'm not sure we want to support or whether we can actually play that combination of codecs.

This patch doesn't affect behaviour on Fennec/Android. I'm not sure how to make this change on Fennec/Android since we ask the platform somehow in Fennec. doublec may know.

Comment 17

[cajbir (:cajbir)]

(In reply to Chris Pearce (:cpearce) from comment #16)
> This patch doesn't affect behaviour on Fennec/Android. I'm not sure how to
> make this change on Fennec/Android since we ask the platform somehow in
> Fennec. doublec may know.

We don't ask the platform we use hardcoded strings.

Comment 18

[Marco Chen [:mchen]]

Comment on attachment 755561 [details] [diff] [review]
Fix for Firefox OS; adds MP3 to valid codecs

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

Thanks for your patch here.

And just as :cpearce said that maybe we can split the codec list into H.264 and mp3.
Then we can assign one of them into codecList and depend on what mime type there.

By the way, please raise the review to Chris Double who has permission on review this area.
Thanks.

Comment 19

[Ian Gilman [:iangilman]]

Attachment: Firefox OS patch 2; separate H264 from Mpeg Audio

Thank you for the review comments! Addressed with this patch.

Comment 20

[Ian Gilman [:iangilman]]

Thank you for the review, :doublec!

What's the next step in landing this?

Comment 21

[Ian Gilman [:iangilman]]

Can anyone provide some guidance on how to land this patch now that it's R+?

Comment 22

[Kevin Brosnan [Ex-Mozilla]]

Assuming you don't have tree level 3 access. The keyword checkin-needed is used. Which I set above. For more details see https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b16a9c6fb6a

Do we have tests for this?

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3b16a9c6fb6a

Comment 25

[Ian Gilman [:iangilman]]

:kbrosnan, thanks for the info!

:RyanVM, this patch doesn't have a test, and I don't know if this area of the code has tests, or where they are. If someone points me in the right direction, I could possibly write a followup.

:edmorley, thanks for landing!

So, the fix that just landed is for Firefox OS, and I believe bug 841239 fixes the issue for Windows 7. I believe that leaves Android, Mac, and Linux (unless I'm missing a version?). I know this is an issue on Android... how about the Mac and Linux versions of Firefox? Do either of them support MP3 via HTML5 audio?

Perhaps rather than trying to do all of those versions in the same bug (:cpearce has suggested each platform needs its own patch), should I open a fresh bug for each?

Comment 26

[Ralph Giles (:rillian)]

There are tests for canPlayType in content/media/test, but perhaps that's not what you're asking for since it already checks codecs=mp3.

https://hg.mozilla.org/mozilla-central/file/tip/content/media/test/can_play_type_mpeg.js

There are platform-specific switches in test_can_play_type_mpeg.html in the same directory.

Comment 27

[Ian Gilman [:iangilman]]

Sure enough, looks like there are tests. I'm confused about how those tests passed before this patch landed, actually!

I'll file bugs for the other platforms.

Comment 28

[Ian Gilman [:iangilman]]

Filed bug 887512 for Mac, bug 887514 for Linux, and bug 887517 for Android.

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

nomming for Leo as this bug impacts mobile Web compatibility, specific the ability to play MP3 files. (See bug 899748.) Chris Double had the following to say about the patch in bug 899748 comment 15 "Yes, it's a safe change and could be uplifted. The patch should apply with minimal changes, if any. Nom away I think."

Comment 30

[Preeti Raghunath(:Preeti)]

I am concerned about uplifting anything given how late it is in the cycle. Can this wait for 1.2 instead?

Comment 31

[Preeti Raghunath(:Preeti)]

Hema,

Please check if this is feature was ground work for 1.2

Comment 32

[Stephany Wilkes]

Adding a ni? for Sri as well.

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Preeti Raghunath(:Preeti) from comment #30)
> I am concerned about uplifting anything given how late it is in the cycle.
> Can this wait for 1.2 instead?

Waiting for 1.2 means that the 1.1 release will not support audio/video on some number of sites. (I don't know the specific number but there is one example in the dependent bug.) I think that if this uplift is considered safe, it's highly preferable for content and worth the risk to uplift.

Comment 34

[cajbir (:cajbir)]

(In reply to Lawrence Mandel [:lmandel] from comment #33)
> 
> Waiting for 1.2 means that the 1.1 release will not support audio/video on
> some number of sites. (I don't know the specific number but there is one
> example in the dependent bug.) I think that if this uplift is considered
> safe, it's highly preferable for content and worth the risk to uplift.

Correct me if I'm wrong but it means that it will not support MP3 audio playback on some number of sites. Other audio/video will continue to work. I do think that it is safe and worthwhile to uplift however (assuming someone has actually tested it works...).

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Chris Double (:doublec) from comment #34)
> Correct me if I'm wrong but it means that it will not support MP3 audio
> playback on some number of sites. Other audio/video will continue to work.

Correct. I'm advocating for an uplift of what I understand to be a safe patch in order to fix some amount of audio/video content.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift to b2g18.

Comment 37

[Ian Gilman [:iangilman]]

While I wrote the original patch (after being pointed in the right direction), I don't know what's involved in making a branch-specific patch. If someone else would like to volunteer for that, please do! Otherwise, if someone wants to walk me through it, I can give it a try.

Either way, I agree it would be good to get this uplifted.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

You need to clone the b2g18 repository and figure out how to adapt what landed on m-c to land there instead.

Comment 39

[Ian Gilman [:iangilman]]

Sounds fair. Where do I find this repository?

Also, what's the deadline? I won't be able to get to it until Monday.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/releases/mozilla-b2g18

Can't speak to a deadline, sorry. I'm just the guy who shuffles the patches around :). I would say as soon as you can get to it.

Comment 41

[Ian Gilman [:iangilman]]

Attachment: Same patch, now ready for bg218 branch

Comment 42

[Ian Gilman [:iangilman]]

Ok, the b2g18 branch-specific patch is in. What's the next step? Does it need to be reviewed again? Flagged for landing on the b2g18 branch?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

I'll get it from here, thanks :)

Comment 44

[Ian Gilman [:iangilman]]

Awesome... let me know if there's anything else you need!

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/fbd799b73e71

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/67e0acd20896

https://tbpl.mozilla.org/php/getParsedLog.php?id=28093744&tree=Mozilla-B2g18

Comment 47

[Ian Gilman [:iangilman]]

Attachment: Ready for b2g18, now with less bustage

I'm terribly sorry about that! This one should work better.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddd56f6a924d

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/ddd56f6a924d

All done here. Thanks for sticking with it, Ian!

Comment 50

[Ian Gilman [:iangilman]]

Awesome... thanks for walking me through it!