Remove the doppler effect from the PannerNode

RESOLVED FIXED in Firefox 63

Status

()

defect
P4
normal
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

({dev-doc-complete, site-compat})

32 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

This has always been a bit broken, and does not work anyways in Gecko because of a bug.

The WG resolution was during TPAC, Blink currently marks those members as "deprecated".

The spec bug is: https://github.com/WebAudio/web-audio-api/issues/455
This removes quite a bit of annoying code. I also believe this is not widely
used, but I don't have numbers, only anecdotal evidence (me searching github for
relevant keywords).
Attachment #8584507 - Flags: review?(ehsan)
Comment on attachment 8584507 [details] [diff] [review]
Remove the doppler effect from the PannerNode. r=

Review of attachment 8584507 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should start by deprecating this with issuing warnings in the Web Console.  Preferrably we'd uplift that to get it into all channels as fast as possible, but please check the l10n requirements on that with the release managers (I think we're OK with English only messages targeted to web devs, not completely sure though.)
Attachment #8584507 - Flags: review?(ehsan) → review-
This is built on top of bug 1148496.
Attachment #8589603 - Flags: review?(ehsan)
Comment on attachment 8589603 [details] [diff] [review]
Deprecate the doppler effect from the PannerNode. r=

Review of attachment 8589603 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice!

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +162,5 @@
>  # LOCALIZATION NOTE (WillChangeBudgetWarning): Do not translate Will-change, %1$S,%2$S,%3$S are numbers.
>  WillChangeBudgetWarning=Will-change memory consumption is too high. Surface area covers %1$S pixels, budget is the document surface area multiplied by %2$S (%3$S pixels). All occurences of will-change in the document are ignored when over budget.
>  # LOCALIZATION NOTE: Do not translate "ServiceWorker".
>  HittingMaxWorkersPerDomain=A ServiceWorker could not be started immediately because other documents in the same origin are already using the maximum number of workers. The ServiceWorker is now queued and will be started after some of the other workers have completed.
> +# LOCALIZATION: Do no translate "setVelocity", "PannerNode", "AudioListener", "speedOfSound" and "dopplerFactor"

Nit: LOCALIZATION NOTE
Attachment #8589603 - Flags: review?(ehsan) → review+
I landed with with a revised error message (including a link to a MDN page).
Keywords: leave-open
Should we set a target for eventual removal?
Flags: needinfo?(padenot)
Priority: -- → P3
I was going to ask the Chrome folks at TPAC (next week). I'll post again here with a tentative date.
Flags: needinfo?(padenot)
One thing to consider may be to first remove the implementation leaving the methods as no-ops.  It would make it hard to test for the feature, but that may be less problematic than generating exceptions in existing code.
I was talking to rtoy from Google, and he looked up the method usage in their statistics, and it was actually called 0% of the time in the last month or so [0], it's probably safe to proceed.

[0]: https://www.chromestatus.com/metrics/feature/timeline/popularity/1251
I've sent an email to the google folks, we're going to sync up on removing those.
Depends on: 1269741
Chromium people have removed the processing bits, but have left the API in place, being no-op. They see very very small but non-null calls on the API, and ultimately want to remove the methods altogether in M55.

I suggest we proceed along the same timeline, starting by removing the processing code first, and making the deprecation message stronger, indicating a release.
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
It's time! We're a bit late in fact :-).
Keywords: leave-open
Comment on attachment 8993079 [details]
Bug 1148354 - Remove the doppler effect from the PannerNode.

https://reviewboard.mozilla.org/r/257906/#review264908

::: dom/media/webaudio/AudioListener.h:48
(Diff revision 1)
>      mPosition.x = aX;
>      mPosition.y = aY;
>      mPosition.z = aZ;
> -    SendThreeDPointParameterToStream(PannerNode::LISTENER_POSITION, mPosition);

Still need to send this to the stream.

::: dom/media/webaudio/AudioListener.h:61
(Diff revision 1)
>    }
>  
>    void SetOrientation(double aX, double aY, double aZ,
>                        double aXUp, double aYUp, double aZUp);
>  
> -  const ThreeDPoint& Velocity() const
> +  const ThreeDPoint& FrontVector() {

Mozilla style is brace on new line.

::: dom/media/webaudio/AudioListener.h:65
(Diff revision 1)
> -  const ThreeDPoint& Velocity() const
> -  {
> +  const ThreeDPoint& FrontVector() {
> +    return mFrontVector;
> -    return mVelocity;
>    }
>  
> -  void SetVelocity(double aX, double aY, double aZ)
> +  const ThreeDPoint& RightVector() {

brace on new line.

::: dom/media/webaudio/AudioListener.cpp
(Diff revision 1)
> -    SendThreeDPointParameterToStream(PannerNode::LISTENER_FRONT_VECTOR, front);
>    }
>    if (!mRightVector.FuzzyEqual(right)) {
>      mRightVector = right;
> -    SendThreeDPointParameterToStream(PannerNode::LISTENER_RIGHT_VECTOR, right);

Still need something to send these to the graph thread.

::: dom/media/webaudio/PannerNode.cpp
(Diff revision 1)
> -    case PannerNode::LISTENER_POSITION: mListenerPosition = aParam; break;
> -    case PannerNode::LISTENER_FRONT_VECTOR: mListenerFrontVector = aParam; break;
> -    case PannerNode::LISTENER_RIGHT_VECTOR: mListenerRightVector = aParam; break;

May need to keep these.

::: dom/media/webaudio/PannerNode.cpp:256
(Diff revision 1)
>    RefPtr<AudioNodeStream> mDestination;
>    // This member is set on the main thread, but is not accessed on the rendering
>    // thread untile mPanningModelFunction has changed, and this happens strictly
>    // later, via a MediaStreamGraph ControlMessage.
>    nsAutoPtr<HRTFPanner> mHRTFPanner;
> +  RefPtr<AudioListener> mListener;

AudioListener is a main thread object.  It should not be accessed from the
engine.  The values required on the graph thread need to be sent via a stream.
For now, it would be possible to use the AudioDestinationNode stream and its
engine until we have an object that can be ordered appropriately, but a
simple first step would be to just leave AudioListener::RegisterPannerNode()
and SendThreeDPointParameterToStream() until AudioParams are implemented.

::: dom/webidl/AudioListener.webidl
(Diff revision 1)
> -    [Deprecated="PannerNodeDoppler"]
> -    attribute double dopplerFactor;
> -
> -    // in meters / second (default 343.3)
> -    [Deprecated="PannerNodeDoppler"]
> -    attribute double speedOfSound;

The dom folks want to review all changes to webidl files.
Attachment #8993079 - Flags: review?(karlt) → review-
I'll be very pleased to see this go, thank you!
Comment on attachment 8993080 [details]
Bug 1148354 - Remove the tests related to the doppler effect from mochitests.

https://reviewboard.mozilla.org/r/257908/#review264932
Attachment #8993080 - Flags: review?(karlt) → review+
Comment on attachment 8993081 [details]
Bug 1148354 - Add a web platform test to test for the non existence of removed methods.

https://reviewboard.mozilla.org/r/257910/#review264936
Attachment #8993081 - Flags: review?(karlt) → review+
Hrm sorry, it looks like my some chunk of 1476744 went into this one.
Attachment #8993079 - Attachment is obsolete: true
Comment on attachment 8993315 [details]
Bug 1148354 - Remove the doppler effect from the PannerNode.

https://reviewboard.mozilla.org/r/258096/#review265060

for the webidl part.
Attachment #8993315 - Flags: review?(amarchesini) → review+
Comment on attachment 8993315 [details]
Bug 1148354 - Remove the doppler effect from the PannerNode.

https://reviewboard.mozilla.org/r/258096/#review265284

::: dom/media/webaudio/AudioListener.cpp:83
(Diff revision 1)
>  
> +

No extra blank line, please.
Attachment #8993315 - Flags: review?(karlt) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/b4085dba45c5
Remove the doppler effect from the PannerNode. r=baku,karlt
https://hg.mozilla.org/integration/autoland/rev/d9174c023701
Remove the tests related to the doppler effect from mochitests.  r=karlt
https://hg.mozilla.org/integration/autoland/rev/97d89ba5f93d
Add a web platform test to test for the non existence of removed methods. r=karlt
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12095 for changes under testing/web-platform/tests
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/512f6987b0b2
Remove the doppler effect from the PannerNode. r=baku,karlt
https://hg.mozilla.org/integration/autoland/rev/8b38f23b507f
Remove the tests related to the doppler effect from mochitests.  r=karlt
https://hg.mozilla.org/integration/autoland/rev/5a481b37f45f
Add a web platform test to test for the non existence of removed methods. r=karlt
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Upstream PR merged
Clearing NI.
Flags: needinfo?(padenot)
Updated Firefox 63 for developers. Verified that the documentation for AudioListener and PannerNode are updated to mark everything deprecated as appropriate (most of this work was previously complete - yay!). Submitted PR #2567 on the BCD repo to update the compat tables.
You need to log in before you can comment on or make changes to this bug.