Support creation of Audio objects outside of the document

VERIFIED FIXED

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: cajbir, Assigned: kinetik)

Tracking

({verified1.9.1})

Trunk
x86
All
verified1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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?
(Reporter)

Comment 2

10 years ago
Created attachment 339015 [details] [diff] [review]
Implements Audio constructor as per spec
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.
(Reporter)

Comment 5

10 years ago
Created attachment 339187 [details] [diff] [review]
Small Ogg sound file
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+
(Reporter)

Comment 6

10 years ago
Created attachment 339193 [details] [diff] [review]
Address comment 3
Attachment #339015 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.9.1?
Resolution: --- → FIXED
(Reporter)

Comment 8

10 years ago
Backed out due to leak report in mochitest from tinderbox
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 9

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

Comment 10

10 years ago
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.
(Assignee)

Comment 12

10 years ago
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.
(Reporter)

Comment 13

10 years ago
Created attachment 344404 [details] [diff] [review]
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.
Attachment #339193 - Attachment is obsolete: true
(Assignee)

Comment 14

10 years ago
Created attachment 364025 [details] [diff] [review]
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...
Attachment #339187 - Attachment is obsolete: true
Attachment #344404 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #364025 - Attachment is patch: true
Attachment #364025 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

10 years ago
Depends on: 480477
(Assignee)

Comment 15

10 years ago
(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+
(Assignee)

Comment 17

10 years ago
Created attachment 378148 [details] [diff] [review]
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.
Assignee: chris.double → kinetik
Attachment #364025 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Are you sure you're not just seeing bug 472773?
(Assignee)

Comment 19

10 years ago
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.
(Assignee)

Comment 20

10 years ago
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.
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]

Comment 23

10 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.