Last Comment Bug 604682 - AudioAvailable event dispatch does unnecessary copy when there are no listeners
: AudioAvailable event dispatch does unnecessary copy when there are no listeners
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-15 10:03 PDT by Timothy B. Terriberry (:derf)
Modified: 2012-02-01 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered (14.95 KB, patch)
2010-10-25 21:04 PDT, Yury Delendik (:yury)
no flags Details | Diff | Review
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered (24.06 KB, patch)
2010-10-26 07:34 PDT, Yury Delendik (:yury)
bugs: review-
Details | Diff | Review
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered (25.06 KB, patch)
2010-10-28 08:46 PDT, Yury Delendik (:yury)
bugs: review-
Details | Diff | Review
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered (27.23 KB, patch)
2010-11-20 10:58 PST, Yury Delendik (:yury)
no flags Details | Diff | Review
Patch v1 (26.21 KB, patch)
2011-09-04 00:04 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch v2 (25.50 KB, patch)
2011-11-21 14:10 PST, Chris Pearce (:cpearce)
bugs: review+
kinetik: review+
Details | Diff | Review

Description Timothy B. Terriberry (:derf) 2010-10-15 10:03:38 PDT
nsAudioAvailableEventManager::QueueWrittenAudioData makes a copy of the audio data (including a memory allocation) for the purposes of event dispatch. However, there is no check to see if there are any listeners for these events until it gets to nsBuiltinDecoder::AudioAvailable(). This will be exacerbated on Fennec because of the patch in bug 511348#c20, which also does an int->float conversion during the copy. I spoke with David Humphrey, and he has a plan to get rid of the copy:

21:09:01 < humph> I think what needs to happen is that we need to change nsPIDOMWindow so that it reports when listeners change for audio events
21:11:40 < humph> I have a patch that is 70% done on the nsPIDOMWindow if you want to try and go that route
Comment 1 Timothy B. Terriberry (:derf) 2010-10-15 10:04:57 PDT
Assigning to David since he says he has a partial patch.
Comment 2 Yury Delendik (:yury) 2010-10-25 21:04:09 PDT
Created attachment 485973 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

Introduces MozAudioAvailableMonitored event. The internal listener for this event is registered when the nsBuiltinDecoder is created.
Comment 3 David Humphrey (:humph) 2010-10-26 06:59:20 PDT
Comment on attachment 485973 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

I've read and built this patch, and it works.  Assigning review.
Comment 4 Yury Delendik (:yury) 2010-10-26 07:34:15 PDT
Created attachment 486055 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

More context
Comment 5 Olli Pettay [:smaug] 2010-10-26 07:54:54 PDT
Comment on attachment 486055 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

>diff -r 39de140a8bdd content/base/src/nsContentUtils.cpp
    >--- a/content/base/src/nsContentUtils.cpp	Fri Sep 24 12:48:30 2010 +1200
    >+++ b/content/base/src/nsContentUtils.cpp	Mon Oct 25 23:02:30 2010 -0400
    >@@ -670,6 +670,7 @@
    >     { nsGkAtoms::ondurationchange,              NS_DURATIONCHANGE, EventNameType_HTML, NS_EVENT_NULL },
    >     { nsGkAtoms::onvolumechange,                NS_VOLUMECHANGE, EventNameType_HTML, NS_EVENT_NULL },
    >     { nsGkAtoms::onMozAudioAvailable,           NS_MOZAUDIOAVAILABLE, EventNameType_None, NS_EVENT_NULL },
    >+    { nsGkAtoms::onMozAudioAvailableMonitored,  NS_MOZAUDIOAVAILABLEMONITORED, EventNameType_None, NS_EVENT_NULL },
    This would not be need, because MozAudioAvailableMonitored is just a simple
    event and onfoo listener
    wouldn't be needed in an object. addEventListener is enough.

    But in general, I don't want to expose new events to content unless they are
    really useful.
    In this case the event is just for gecko internal usage, so I think something
    else should be done.
    (but still random comments about the patch.)


    >+  mAudioAvailableMonitoredListener(nsnull),
    nsCOMPtr variables are automatically initialized to null.

    >+  if (mAudioAvailableMonitoredListener) {
    >+    mAudioAvailableMonitoredListener.forget();
    >+  }
    So here you leak mAudioAvailableMonitoredListener object.


    >+DecoderAudioAvailableMonitoredListener::~DecoderAudioAvailableMonitoredListener()
    >+{
    >+  Detach();
    dtor won't be ever called if window's event listener manager holds still a
    reference to this object. So Detach() should be useless here.


    But anyway, this is not the right approach.
    Still thinking what could be...
Comment 6 Olli Pettay [:smaug] 2010-10-26 08:00:28 PDT
So seems that document keeps already a list of media elements ( mFreezableElements is practice).
So when an AudioAvailable event is added, it could go through
all the media element in the current document
and mark that there is a listener.
Comment 7 Yury Delendik (:yury) 2010-10-28 08:46:26 PDT
Created attachment 486628 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

Removing internal event, maintain the list of subscribers/decoders in the document.
Comment 8 Olli Pettay [:smaug] 2010-11-09 08:37:34 PST
Comment on attachment 486628 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

>diff -r 39de140a8bdd content/base/public/nsIDocument.h
>--- a/content/base/public/nsIDocument.h	Fri Sep 24 12:48:30 2010 +1200
>+++ b/content/base/public/nsIDocument.h	Thu Oct 28 11:39:40 2010 -0400
>@@ -102,16 +102,18 @@ template<class E> class nsCOMArray;
> class nsIDocumentObserver;
> class nsBindingManager;
> class nsIDOMNodeList;
> class mozAutoSubtreeModified;
> struct JSObject;
> class nsFrameLoader;
> class nsIBoxObject;
> class imgIRequest;
>+class mozAudioAvailableMonitoredSubscriber;
>+class mozAudioAvailableMonitoredSubscription;
> 
> namespace mozilla {
> namespace css {
> class Loader;
> } // namespace css
> 
> namespace dom {
> class Link;
>@@ -1263,16 +1265,22 @@ public:
>   PRBool IsActive() { return mDocumentContainer && !mRemovedFromDocShell; }
> 
>   void RegisterFreezableElement(nsIContent* aContent);
>   PRBool UnregisterFreezableElement(nsIContent* aContent);
>   typedef void (* FreezableElementEnumerator)(nsIContent*, void*);
>   void EnumerateFreezableElements(FreezableElementEnumerator aEnumerator,
>                                   void* aData);
> 
>+#ifdef MOZ_MEDIA
>+  virtual mozAudioAvailableMonitoredSubscription* SubscribeToAudioAvailableMonitored(
>+    mozAudioAvailableMonitoredSubscriber* aSubscriber) = 0;
>+  virtual void SetAudioAvailableMonitored() = 0;
>+#endif
You're changing an interface, so you should update IID.
If you want to get this to FF4, that may not be possible anymore (because of interface freeze).
In that case the options are to add new interface which document implements or
cast document to nsDocument when needed and add new methods to nsDocument (not to nsIDocument).
The latter is IMO better option in this case.


>+class mozAudioAvailableMonitoredSubscription
>+{
>+public:
>+  virtual ~mozAudioAvailableMonitoredSubscription() { }
>+};
>+
>+class mozAudioAvailableMonitoredSubscriber
>+{
>+public:
>+  virtual void AudioAvailableMonitored() = 0;
>+};

The naming of the classes is rather strange.
It is a bit hard to understand what they are for.
Could you please think some better names and add some comments.



>+nsAudioAvaialableMonitoredDocumentSubscription::nsAudioAvaialableMonitoredDocumentSubscription(
>+  nsDocument* aDocument, mozAudioAvailableMonitoredSubscriber *aSubscriber) 
>+  : mDocument(aDocument), mSubscriber(aSubscriber) 
>+{ 
>+  aDocument->mAudioAvailableMonitoredSubscribers.AppendElement(aSubscriber);
>+}
>+
>+nsAudioAvaialableMonitoredDocumentSubscription::~nsAudioAvaialableMonitoredDocumentSubscription()
>+{
>+  mDocument->mAudioAvailableMonitoredSubscribers.RemoveElement(mSubscriber);
Is it guaranteed that mDocument points to a valid object here?

>+  // Determine if window has an audio avialable event listener and,
>+  // also, listen for new listeners.  Note: we allow for the case of
>+  // |var a = new Audio()| with no parent document.
>+  nsIDocument *document = mElement->GetDocument();
You want OwnerDoc()


I don't understand how this works when an audio element is adopted from one document to another.
The mozAudioAvailableMonitoredSubscriber object is, as far as I see, still in the old document.
Comment 9 Olli Pettay [:smaug] 2010-11-09 08:46:27 PST
And btw, it is not Avaialable, but Available
Comment 10 Yury Delendik (:yury) 2010-11-20 10:58:45 PST
Created attachment 492075 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

Adds/moves methods (SubscribeToAudioAvailableMonitored, SetAudioAvailableMonitored, etc.) to nsDocument (instead of nsIDocument and nsPIDOMWindow); comments
Comment 11 Chris Pearce (:cpearce) 2011-08-15 15:57:09 PDT
Olli: Would it be acceptable to traverse the subtree of nsEventListenerManager::mTarget in nsEventListenerManager::AddEventListener() when a MozAudioAvailable listener is added, and mark all media elements in the subtree as having a MozAudioAvailable event listener? That would mean that we're not (effectively) marking all media elements in a document as having a MozAudioAvailable event listener, as we are with the existing patch here.
Comment 12 Chris Pearce (:cpearce) 2011-08-15 16:07:26 PDT
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #11)
> Olli: Would it be acceptable to traverse the subtree of
> nsEventListenerManager::mTarget in
> nsEventListenerManager::AddEventListener() when a MozAudioAvailable listener
> is added, and mark all media elements in the subtree as having a
> MozAudioAvailable event listener?

Wait, scratch that, it won't work if we add a listener for a bubbling event and then add a media element descendant after that. In that case we'd also need to mark the document, so we may as well just stay with the approach we had going here already, which covers the common case of no MozAudioAvailable listeners.
Comment 13 Chris Pearce (:cpearce) 2011-09-04 00:04:49 PDT
Created attachment 558126 [details] [diff] [review]
Patch v1

Notify a document when a mozaudioavailable listener is added to one of its nodes. This then sets a flag in the document, and notifies all the document's media elements that they need to start dispatching mozaudioavailable events. When we bind a media element to a document, it checks that flag. We also check the flag when we create a decoder (since the decoder may not exist when we bind to the doc tree).

Requesting review from Olli for the dom/content changes, and will request review from kinetik for the changes to content/media specifically.
Comment 14 Chris Pearce (:cpearce) 2011-09-04 00:06:00 PDT
Comment on attachment 558126 [details] [diff] [review]
Patch v1

Requesting review from kinetik for the changes to content/media.
Comment 15 Olli Pettay [:smaug] 2011-09-04 04:49:07 PDT
Comment on attachment 558126 [details] [diff] [review]
Patch v1

> #ifdef MOZ_MEDIA
>   } else if (aType == NS_MOZAUDIOAVAILABLE) {
>-    mMayHaveAudioAvailableEventListener = PR_TRUE;
>-    nsPIDOMWindow* window = GetInnerWindowForTarget();
>-    if (window) {
>-      window->SetHasAudioAvailableEventListeners();
>+    nsCOMPtr<nsINode> node = do_QueryInterface(mTarget);
>+    if (node) {
>+      nsIDocument* document = node->GetOwnerDoc();    
>+      if (document) {
>+        document->NotifyAudioAvailableListener();
>+      }
>     }
So what if the event listener is added to window?

Btw, please use the coding style
if (expr) {
  stmt;
}
Comment 16 Chris Pearce (:cpearce) 2011-09-04 13:02:51 PDT
Comment on attachment 558126 [details] [diff] [review]
Patch v1

D'oh...
Comment 17 Chris Pearce (:cpearce) 2011-11-21 14:10:08 PST
Created attachment 575972 [details] [diff] [review]
Patch v2

When a "MozAudioAvailable" listener is added, notify the document and its media elements, who then know that they need to dispatch "MozAudioAvailable" events. If a media element's document has not been notified, it won't dispatch "MozAudioAvailable" event.

Requesting review from Matthew for changes to content/media and nsHTMLMediaElment, requesting review from Olli for everything else.
Comment 18 Olli Pettay [:smaug] 2011-11-21 15:14:00 PST
Comment on attachment 575972 [details] [diff] [review]
Patch v2


>+  virtual bool HasAudioAvailableListeners() {
{ should be on the next line.
And why is this method virtual?


>+nsGlobalWindow::SetHasAudioAvailableEventListeners()
>+{
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));
>+  doc->NotifyAudioAvailableListener();
nsGlobalWindow has mDoc;
But just to be safe, null check it.
Comment 19 Chris Pearce (:cpearce) 2011-11-21 16:38:52 PST
Landed with review comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/566d93e2500c
Comment 20 Ed Morley [:emorley] 2011-11-22 09:09:12 PST
https://hg.mozilla.org/mozilla-central/rev/566d93e2500c
Comment 21 Mihaela Velimiroviciu (:mihaelav) 2011-11-30 02:25:31 PST
Is there any way that QA can verify this issue?

Thank you!
Comment 22 Chris Pearce (:cpearce) 2011-11-30 18:13:00 PST
The only way to verify this would be using a debugger. Probably easier to just trust me on this one. ;)

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