Closed
Bug 838331
Opened 11 years ago
Closed 11 years ago
Audio's canPlayType() fails if codec is supplied
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: iangilman, Assigned: iangilman)
References
Details
Attachments
(2 files, 2 obsolete files)
1.62 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
Details | Diff | Splinter Review |
(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.
Updated•11 years ago
|
Component: General → Video/Audio
Product: Firefox for Android → Core
Version: Firefox 18 → Trunk
Comment 1•11 years ago
|
||
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•11 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
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Any thoughts on who might be the best person to do the Android version of this fix?
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
Cool, thanks! How about Firefox OS? Is that the same code branch?
Comment 8•11 years ago
|
||
Unfortunately no, we'll need a separate patch for each platform to get behaviour to align on every platform we support. :(
Comment 9•11 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•11 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•11 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•11 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.
Comment 14•11 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•11 years ago
|
||
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 16•11 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]: ----------------------------------------------------------------- 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•11 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•11 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•11 years ago
|
||
Thank you for the review comments! Addressed with this patch.
Attachment #755561 -
Attachment is obsolete: true
Attachment #756105 -
Flags: review?(chris.double)
Updated•11 years ago
|
Attachment #756105 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 20•11 years ago
|
||
Thank you for the review, :doublec! What's the next step in landing this?
Assignee | ||
Comment 21•11 years ago
|
||
Can anyone provide some guidance on how to land this patch now that it's R+?
Comment 22•11 years ago
|
||
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
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b16a9c6fb6a Do we have tests for this?
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3b16a9c6fb6a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee | ||
Comment 25•11 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?
Comment 26•11 years ago
|
||
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•11 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•11 years ago
|
||
Filed bug 887512 for Mac, bug 887514 for Linux, and bug 887517 for Android.
Comment 29•11 years ago
|
||
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?
Comment 30•11 years ago
|
||
I am concerned about uplifting anything given how late it is in the cycle. Can this wait for 1.2 instead?
Updated•11 years ago
|
Flags: needinfo?(hkoka)
Flags: needinfo?(cbeasley)
Comment 31•11 years ago
|
||
Hema, Please check if this is feature was ground work for 1.2
Comment 32•11 years ago
|
||
Adding a ni? for Sri as well.
Comment 33•11 years ago
|
||
(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•11 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...).
Comment 35•11 years ago
|
||
(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.
Updated•11 years ago
|
blocking-b2g: leo? → leo+
Updated•11 years ago
|
Flags: needinfo?(cbeasley)
Comment 36•11 years ago
|
||
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•11 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)
Comment 38•11 years ago
|
||
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•11 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)
Comment 40•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 42•11 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)
Assignee | ||
Comment 44•11 years ago
|
||
Awesome... let me know if there's anything else you need!
Comment 45•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fbd799b73e71
Keywords: branch-patch-needed
Comment 46•11 years ago
|
||
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
Updated•11 years ago
|
Flags: needinfo?(hkoka)
Assignee | ||
Comment 47•11 years ago
|
||
I'm terribly sorry about that! This one should work better.
Attachment #806229 -
Attachment is obsolete: true
Flags: needinfo?(ian)
Comment 48•11 years ago
|
||
Thanks :) https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddd56f6a924d
Keywords: branch-patch-needed
Comment 49•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/ddd56f6a924d All done here. Thanks for sticking with it, Ian!
Assignee | ||
Comment 50•11 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.
Description
•