Closed Bug 454971 Opened 12 years ago Closed 12 years ago

Support creation of Audio objects outside of the document

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: cajbir, Assigned: kinetik)

References

()

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 5 obsolete files)

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."
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.
Flags: blocking1.9.1?
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attachment #339015 - Flags: superreview?(roc)
Attachment #339015 - Flags: review?(roc)
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.
Attachment #339015 - Flags: superreview?(roc)
Attachment #339015 - Flags: superreview+
Attachment #339015 - Flags: review?(roc)
Attachment #339015 - Flags: review+
(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.
Attached patch Small Ogg sound file (obsolete) — Splinter Review
Attachment #339187 - Flags: superreview?(roc)
Attachment #339187 - Flags: review?(roc)
Attachment #339187 - Flags: superreview?(roc)
Attachment #339187 - Flags: superreview+
Attachment #339187 - Flags: review?(roc)
Attachment #339187 - Flags: review+
Attached patch Address comment 3 (obsolete) — Splinter Review
Attachment #339015 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: blocking1.9.1?
Resolution: --- → FIXED
Backed out due to leak report in mochitest from tinderbox
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
nsOggDecoder leaks the nsChannelToPipeListener. The fix was applied to bug 449159 (which is when I found it).
If Audio is created and playback started, what cancels playback if the page
goes to bfcache?
Good point --- nothing. I guess we need a mechanism to track the media elements owned by a given document.
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.
Attached patch Update to trunk (obsolete) — Splinter Review
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.
Attachment #339193 - Attachment is obsolete: true
Attached patch rebased to trunk, tests fixed up (obsolete) — Splinter Review
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...
Attachment #339187 - Attachment is obsolete: true
Attachment #344404 - Attachment is obsolete: true
Attachment #364025 - Attachment is patch: true
Attachment #364025 - Attachment mime type: application/octet-stream → text/plain
Depends on: 480477
(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.
Bug 480819 is fixed so comment #11 is now resolved. Matthew, can we land this on trunk and get it on 1.9.1?
Flags: wanted1.9.1+
Attached patch rebased againSplinter Review
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.
Assignee: chris.double → kinetik
Attachment #364025 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Are you sure you're not just seeing bug 472773?
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.
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.
Keywords: checkin-needed
Whiteboard: [needs landing]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.