Last Comment Bug 838331 - Audio's canPlayType() fails if codec is supplied
: Audio's canPlayType() fails if codec is supplied
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla24
Assigned To: Ian Gilman [:iangilman]
:
Mentors:
: 876992 (view as bug list)
Depends on:
Blocks: 899748
  Show dependency treegraph
 
Reported: 2013-02-05 14:11 PST by Ian Gilman [:iangilman]
Modified: 2013-09-23 09:20 PDT (History)
13 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
leo+
wontfix
wontfix
fixed
fixed
wontfix
wontfix
fixed


Attachments
Fix for Firefox OS; adds MP3 to valid codecs (1.66 KB, patch)
2013-05-29 13:01 PDT, Ian Gilman [:iangilman]
no flags Details | Diff | Review
Firefox OS patch 2; separate H264 from Mpeg Audio (1.62 KB, patch)
2013-05-30 11:49 PDT, Ian Gilman [:iangilman]
cajbir.bugzilla: review+
Details | Diff | Review
Same patch, now ready for bg218 branch (1.49 KB, patch)
2013-09-17 13:49 PDT, Ian Gilman [:iangilman]
no flags Details | Diff | Review
Ready for b2g18, now with less bustage (2.22 KB, patch)
2013-09-19 14:00 PDT, Ian Gilman [:iangilman]
no flags Details | Diff | Review

Description Ian Gilman [:iangilman] 2013-02-05 14:11:08 PST
(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) 2013-02-06 06:32:29 PST
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] 2013-02-06 13:58:39 PST
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) 2013-02-07 14:26:03 PST
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) 2013-02-25 18:34:49 PST
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] 2013-05-01 14:07:46 PDT
Any thoughts on who might be the best person to do the Android version of this fix?
Comment 6 Chris Peterson [:cpeterson] 2013-05-01 14:38:35 PDT
(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] 2013-05-07 13:54:52 PDT
Cool, thanks!

How about Firefox OS? Is that the same code branch?
Comment 8 Chris Pearce (:cpearce) 2013-05-07 16:54:23 PDT
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) 2013-05-07 17:11:47 PDT
> 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] 2013-05-08 09:06:21 PDT
: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) 2013-05-08 10:13:59 PDT
(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] 2013-05-13 11:33:37 PDT
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 13 Marco Chen [:mchen] 2013-05-28 21:24:08 PDT
*** Bug 876992 has been marked as a duplicate of this bug. ***
Comment 14 Marco Chen [:mchen] 2013-05-29 03:35:35 PDT
(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] 2013-05-29 13:01:37 PDT
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?
Comment 16 Chris Pearce (:cpearce) 2013-05-29 15:44:31 PDT
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) 2013-05-29 16:36:50 PDT
(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] 2013-05-30 00:48:28 PDT
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] 2013-05-30 11:49:52 PDT
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.
Comment 20 Ian Gilman [:iangilman] 2013-06-04 09:01:47 PDT
Thank you for the review, :doublec!

What's the next step in landing this?
Comment 21 Ian Gilman [:iangilman] 2013-06-20 11:10:37 PDT
Can anyone provide some guidance on how to land this patch now that it's R+?
Comment 22 Kevin Brosnan [:kbrosnan] 2013-06-20 11:26:46 PDT
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] 2013-06-20 13:51:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b16a9c6fb6a

Do we have tests for this?
Comment 24 Ed Morley [:emorley] 2013-06-21 07:58:15 PDT
https://hg.mozilla.org/mozilla-central/rev/3b16a9c6fb6a
Comment 25 Ian Gilman [:iangilman] 2013-06-21 12:52:52 PDT
: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) needinfo me 2013-06-21 13:35:51 PDT
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] 2013-06-26 14:53:51 PDT
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] 2013-06-26 15:07:53 PDT
Filed bug 887512 for Mac, bug 887514 for Linux, and bug 887517 for Android.
Comment 29 Lawrence Mandel [:lmandel] (use needinfo) 2013-09-03 17:33:25 PDT
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) 2013-09-05 16:22:24 PDT
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) 2013-09-09 08:06:38 PDT
Hema,

Please check if this is feature was ground work for 1.2
Comment 32 Stephany Wilkes 2013-09-09 08:07:31 PDT
Adding a ni? for Sri as well.
Comment 33 Lawrence Mandel [:lmandel] (use needinfo) 2013-09-09 08:24:30 PDT
(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) 2013-09-09 17:28:09 PDT
(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) 2013-09-10 10:08:40 PDT
(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] 2013-09-12 06:59:59 PDT
Needs a branch-specific patch for uplift to b2g18.
Comment 37 Ian Gilman [:iangilman] 2013-09-12 10:58:16 PDT
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] 2013-09-13 06:06:59 PDT
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] 2013-09-13 09:36:24 PDT
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] 2013-09-13 09:56:56 PDT
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] 2013-09-17 13:49:15 PDT
Created attachment 806229 [details] [diff] [review]
Same patch, now ready for bg218 branch
Comment 42 Ian Gilman [:iangilman] 2013-09-17 13:51:06 PDT
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] 2013-09-17 14:01:30 PDT
I'll get it from here, thanks :)
Comment 44 Ian Gilman [:iangilman] 2013-09-18 08:51:17 PDT
Awesome... let me know if there's anything else you need!
Comment 45 Ryan VanderMeulen [:RyanVM] 2013-09-19 06:28:06 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fbd799b73e71
Comment 47 Ian Gilman [:iangilman] 2013-09-19 14:00:17 PDT
Created attachment 807417 [details] [diff] [review]
Ready for b2g18, now with less bustage

I'm terribly sorry about that! This one should work better.
Comment 48 Ryan VanderMeulen [:RyanVM] 2013-09-20 07:39:08 PDT
Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddd56f6a924d
Comment 49 Ryan VanderMeulen [:RyanVM] 2013-09-20 15:57:44 PDT
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] 2013-09-23 09:20:35 PDT
Awesome... thanks for walking me through it!

Note You need to log in before you can comment on or make changes to this bug.