Audio's canPlayType() fails if codec is supplied

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: iangilman, Assigned: iangilman)

Tracking

Trunk
mozilla24
All
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
(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.
Component: General → Video/Audio
Product: Firefox for Android → Core
Version: Firefox 18 → Trunk
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
(Assignee)

Comment 2

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

Comment 5

4 years ago
Any thoughts on who might be the best person to do the Android version of this fix?
(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?
(Assignee)

Comment 7

4 years ago
Cool, thanks!

How about Firefox OS? Is that the same code branch?
Unfortunately no, we'll need a separate patch for each platform to get behaviour to align on every platform we support. :(

Comment 9

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

Comment 10

4 years ago
: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

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

Comment 12

4 years ago
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.

Updated

4 years ago
Duplicate of this bug: 876992

Comment 14

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

Comment 15

4 years ago
Created attachment 755561 [details] [diff] [review]
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?
Attachment #755561 - Flags: review?(mchen)
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

4 years ago
(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

4 years ago
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.
Attachment #755561 - Flags: review?(mchen)
(Assignee)

Comment 19

4 years ago
Created attachment 756105 [details] [diff] [review]
Firefox OS patch 2; separate H264 from Mpeg Audio

Thank you for the review comments! Addressed with this patch.
Attachment #755561 - Attachment is obsolete: true
Attachment #756105 - Flags: review?(chris.double)

Updated

4 years ago
Attachment #756105 - Flags: review?(chris.double) → review+
(Assignee)

Comment 20

4 years ago
Thank you for the review, :doublec!

What's the next step in landing this?
(Assignee)

Comment 21

4 years ago
Can anyone provide some guidance on how to land this patch now that it's R+?
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
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b16a9c6fb6a

Do we have tests for this?
Assignee: nobody → ian
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b16a9c6fb6a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Comment 25

4 years ago
: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?
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.
(Assignee)

Comment 27

4 years ago
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.
OS: Android → Gonk (Firefox OS)
Hardware: ARM → All
(Assignee)

Comment 28

4 years ago
Filed bug 887512 for Mac, bug 887514 for Linux, and bug 887517 for Android.
Blocks: 899748
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."
blocking-b2g: --- → leo?
I am concerned about uplifting anything given how late it is in the cycle. Can this wait for 1.2 instead?
Flags: needinfo?(hkoka)
Flags: needinfo?(cbeasley)
Hema,

Please check if this is feature was ground work for 1.2

Comment 32

4 years ago
Adding a ni? for Sri as well.
(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

4 years ago
(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...).
(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.
blocking-b2g: leo? → leo+
Flags: needinfo?(cbeasley)
Needs a branch-specific patch for uplift to b2g18.
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
Flags: needinfo?(ian)
Flags: in-testsuite?
Flags: in-testsuite-
Keywords: branch-patch-needed
(Assignee)

Comment 37

4 years ago
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.
Flags: needinfo?(ian)
You need to clone the b2g18 repository and figure out how to adapt what landed on m-c to land there instead.
Flags: needinfo?(ian)
(Assignee)

Comment 39

4 years ago
Sounds fair. Where do I find this repository?

Also, what's the deadline? I won't be able to get to it until Monday.
Flags: needinfo?(ian)
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.
(Assignee)

Comment 41

4 years ago
Created attachment 806229 [details] [diff] [review]
Same patch, now ready for bg218 branch
(Assignee)

Comment 42

4 years ago
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?
Flags: needinfo?(ryanvm)
I'll get it from here, thanks :)
Flags: needinfo?(ryanvm)
(Assignee)

Comment 44

4 years ago
Awesome... let me know if there's anything else you need!
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fbd799b73e71
status-b2g18: affected → fixed
Keywords: branch-patch-needed
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
status-b2g18: fixed → affected
Flags: needinfo?(ian)
Keywords: branch-patch-needed

Updated

4 years ago
Flags: needinfo?(hkoka)
(Assignee)

Comment 47

4 years ago
Created attachment 807417 [details] [diff] [review]
Ready for b2g18, now with less bustage

I'm terribly sorry about that! This one should work better.
Attachment #806229 - Attachment is obsolete: true
Flags: needinfo?(ian)
Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddd56f6a924d
status-b2g18: affected → fixed
Keywords: branch-patch-needed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/ddd56f6a924d

All done here. Thanks for sticking with it, Ian!
status-b2g-v1.1hd: affected → fixed
(Assignee)

Comment 50

4 years ago
Awesome... thanks for walking me through it!
You need to log in before you can comment on or make changes to this bug.