The default bug view has changed. See this FAQ.

AudioAvailable event dispatch does unnecessary copy when there are no listeners

RESOLVED FIXED in mozilla11

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: derf, Assigned: cpearce)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

7 years ago
Assigning to David since he says he has a partial patch.
Assignee: nobody → david.humphrey
Status: NEW → ASSIGNED
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 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.
Attachment #485973 - Flags: review?(Olli.Pettay)
Assignee: david.humphrey → async.processingjs
Created attachment 486055 [details] [diff] [review]
Suppresses queuing of the MozAudioAvailable event if there were no listeners registered

More context
Attachment #485973 - Attachment is obsolete: true
Attachment #486055 - Flags: review?(Olli.Pettay)
Attachment #485973 - Flags: review?(Olli.Pettay)
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...
Attachment #486055 - Flags: review?(Olli.Pettay) → review-
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.
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.
Attachment #486055 - Attachment is obsolete: true
Attachment #486628 - Flags: review?(Olli.Pettay)
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.
Attachment #486628 - Flags: review?(Olli.Pettay) → review-
And btw, it is not Avaialable, but Available
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
Attachment #486628 - Attachment is obsolete: true
Attachment #492075 - Flags: feedback?(david.humphrey)

Updated

6 years ago
Assignee: async.processingjs → nobody
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

6 years ago
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.
Assignee: nobody → chris
Attachment #492075 - Attachment is obsolete: true
Attachment #558126 - Flags: review?(Olli.Pettay)
Attachment #492075 - Flags: feedback?(david.humphrey)
(Assignee)

Comment 14

6 years ago
Comment on attachment 558126 [details] [diff] [review]
Patch v1

Requesting review from kinetik for the changes to content/media.
Attachment #558126 - Flags: review?(kinetik)
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;
}
(Assignee)

Comment 16

6 years ago
Comment on attachment 558126 [details] [diff] [review]
Patch v1

D'oh...
Attachment #558126 - Attachment is obsolete: true
Attachment #558126 - Flags: review?(kinetik)
Attachment #558126 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 17

5 years ago
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.
Attachment #575972 - Flags: review?(kinetik)
Attachment #575972 - Flags: review?(bugs)
Attachment #575972 - Flags: review?(kinetik) → review+
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.
Attachment #575972 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

5 years ago
Landed with review comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/566d93e2500c
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/566d93e2500c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Is there any way that QA can verify this issue?

Thank you!
(Assignee)

Comment 22

5 years ago
The only way to verify this would be using a debugger. Probably easier to just trust me on this one. ;)
You need to log in before you can comment on or make changes to this bug.