Closed Bug 838331 Opened 7 years ago Closed 6 years ago
Play Type() fails if codec is supplied
(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  and  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. : http://wiki.whatwg.org/wiki/Video_type_parameters#MPEG : http://www.rfc-editor.org/rfc/rfc3003.txt
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.
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?
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. :(
> 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>
: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?
(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.
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.
(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
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 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.
(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 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.
Thank you for the review comments! Addressed with this patch.
Thank you for the review, :doublec! What's the next step in landing this?
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b16a9c6fb6a Do we have tests for this?
Assignee: nobody → ian
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
: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.
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
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?
Hema, Please check if this is feature was ground work for 1.2
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.
(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.
Needs a branch-specific patch for uplift to b2g18.
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.
You need to clone the b2g18 repository and figure out how to adapt what landed on m-c to land there instead.
Sounds fair. Where do I find this repository? Also, what's the deadline? I won't be able to get to it until Monday.
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.
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?
I'll get it from here, thanks :)
Awesome... let me know if there's anything else you need!
I'm terribly sorry about that! This one should work better.
Attachment #806229 - Attachment is obsolete: true
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/ddd56f6a924d All done here. Thanks for sticking with it, Ian!
Awesome... thanks for walking me through it!
You need to log in before you can comment on or make changes to this bug.