604682 - AudioAvailable event dispatch does unnecessary copy when there are no listeners

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

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)]

Assigning to David since he says he has a partial patch.

Comment 2

[Yury Delendik (:yury)]

Attachment: 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)]

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)]

Attachment: Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

More context

Comment 5

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

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][[e-mail]]]

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)]

Attachment: 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][[e-mail]]]

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][[e-mail]]]

And btw, it is not Avaialable, but Available

Comment 10

[Yury Delendik (:yury)]

Attachment: 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 (Not reading bugmail)]]

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 (Not reading bugmail)]]

(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 (Not reading bugmail)]]

Attachment: 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 (Not reading bugmail)]]

Comment on attachment 558126 [details] [diff] [review]
Patch v1

Requesting review from kinetik for the changes to content/media.

Comment 15

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

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 (Not reading bugmail)]]

Comment on attachment 558126 [details] [diff] [review]
Patch v1

D'oh...

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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][[e-mail]]]

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 (Not reading bugmail)]]

Landed with review comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/566d93e2500c

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/566d93e2500c

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Is there any way that QA can verify this issue?

Thank you!

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

The only way to verify this would be using a debugger. Probably easier to just trust me on this one. ;)