454971 - Support creation of Audio objects outside of the document

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

An audio object should be able to be created and played with:

 var sound = new Audio("foo.ogg");
 sound.onload = function() { sound.play() };

or

  var sound = new Audio("foo.ogg");
  sound.oncanplaythrough = function() { sound.play() };

From the spec:

"Two constructors are provided for creating HTMLAudioElement objects (in addition to the factory methods from DOM Core such as createElement()): Audio() and Audio(url). When invoked as constructors, these must return a new HTMLAudioElement object (a new audio element). If the src argument is present, the object created must have its src content attribute set to the provided value, and the user agent must invoke the load() method on the object before returning."

Comment 1

[Dave Townsend [:mossop]]

This would be very useful for extension and app developers who want to provide audio feedback without having to stick things into the DOM, maybe only a wanted but would be good to get it on the radar either way.

Comment 2

[cajbir (:cajbir)]

Attachment: Implements Audio constructor as per spec

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 339015 [details] [diff] [review]
Implements Audio constructor as per spec

+  nsIContent* inst = NS_NewHTMLAudioElement(nsnull);
+  nsresult rv = NS_ERROR_OUT_OF_MEMORY;
+  if (inst) {
+    NS_ADDREF(inst);
+    rv = inst->QueryInterface(aIID, aResult);
+    NS_RELEASE(inst);

make inst an nsCOMPtr<nsIContent> to avoid manual addref/release.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> This would be very useful for extension and app developers who want to provide
> audio feedback without having to stick things into the DOM, maybe only a wanted
> but would be good to get it on the radar either way.

Yes, but the JS caller still needs to have a global object that's a window, so we can get a document from it for the audio element to be owned by. So this would work from most extensions and in chrome, but it wouldn't work in a JS component, I guess.

Comment 5

[cajbir (:cajbir)]

Attachment: Small Ogg sound file

Comment 6

[cajbir (:cajbir)]

Attachment: Address comment 3

Comment 7

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/file/477309013214
http://hg.mozilla.org/mozilla-central/file/8433a061d711

Comment 8

[cajbir (:cajbir)]

Backed out due to leak report in mochitest from tinderbox

Comment 9

[cajbir (:cajbir)]

nsOggDecoder leaks the nsChannelToPipeListener. The fix was applied to bug 449159 (which is when I found it).

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

If Audio is created and playback started, what cancels playback if the page
goes to bfcache?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good point --- nothing. I guess we need a mechanism to track the media elements owned by a given document.

Comment 12

[Matthew Gregan [:kinetik]]

NS_NewHTMLAudioElement should pass aFromParser to the nsHTMLAudioElement constructor, otherwise it's possible to end up in nsHTMLMediaElement::PickMediaElement (via BindToTree, since mIsDoneAddingChildren ends up being false) before any child <source> elements have been attached.

Comment 13

[cajbir (:cajbir)]

Attachment: Update to trunk

Updated to trunk and fixed issue raised in comment 12. Still leaks after running tests so bug 449159 didn't resolve the issue. Looking into it.

Comment 14

[Matthew Gregan [:kinetik]]

Attachment: rebased to trunk, tests fixed up

Tests were still using setTimeout (which we've sinced removed from the rest of the media tests), so removed that.  Call load() explicitly as required by the spec.  

Note that I changed the tests to use a Wave file because the tests never finish using an Ogg file as "loaded" never fires (unless you play() first)!  This is a recent regression, I'll file a separate bug for it.

I don't see any leaks when running all of the tests in content/media/video/test using a Wave file.  If I run the same set of tests and call play() after load() (using sound.ogg) in the two new tests, I consistently see an nsBaseAppShell and nsRunnable leaked...

Comment 15

[Matthew Gregan [:kinetik]]

(In reply to comment #14)
> Note that I changed the tests to use a Wave file because the tests never finish
> using an Ogg file as "loaded" never fires (unless you play() first)!  This is a
> recent regression, I'll file a separate bug for it.

Bug 480477.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 480819 is fixed so comment #11 is now resolved. Matthew, can we land this on trunk and get it on 1.9.1?

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: rebased again

Rebased against current trunk.  I've also reverted the tests back to using the Ogg Vorbis sound file.

All of the tests pass, but I'm still seeing the nsRunnable and nsBaseAppShell leak sometimes (both with the Wave and Ogg backend now, unlike I claimed earlier).  I can reproduce it randomly every third run of test_audio1.html and/or test_audio2.html.  If I wait long enough before quitting after running those tests, I never seem to see the leak.  It seems like a timing issue, but the strange thing is that it seems to consistently leak after running all of the tests in content/media/video/test, and since test_audio[12].html are near the start they should've had plenty of time to clean up...

I've got some logging from the XPCOM leak debugging stuff, but I'm having trouble making sense of it.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are you sure you're not just seeing bug 472773?

Comment 19

[Matthew Gregan [:kinetik]]

Ah, thanks.  I had seen that bug before, but had forgotten about it.  No, I'm not sure.  It sounds exactly like that bug, but I don't think I've ever noticed that leak happen locally without this patch applied.

Comment 20

[Matthew Gregan [:kinetik]]

Looking at the leak logs (not that I understand them very well), and based on bug 472773 comment 10, we should classify the leak I'm seeing as the random leak documented in bug 472773 and not something new caused by the patch in this bug.  I'll attach an archive of the leak logs to the other bug just in case further analysis is needed.

Based on that, I think this patch is ready to land.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/df4a4ef0e00e

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/282bccc6e8b3

Comment 23

[Tony Chung [:tchung]]

I see tests.  verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre Ubiquity/0.1.5 and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090526 Minefield/3.6a1pre