1148354 - Remove the doppler effect from the PannerNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P4)

Description

[Paul Adenot (:padenot)]

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

Comment 1

[Paul Adenot (:padenot)]

Attachment: Remove the doppler effect from the PannerNode. r=

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

Comment 2

[(no longer active)]

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

Comment 3

[Paul Adenot (:padenot)]

Attachment: Deprecate the doppler effect from the PannerNode. r=

This is built on top of bug 1148496.

Comment 4

[(no longer active)]

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

Comment 5

[Paul Adenot (:padenot)]

I landed with with a revised error message (including a link to a MDN page).

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e75189490f3c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e75189490f3c

Comment 8

[Maire Reavy [:mreavy]]

Should we set a target for eventual removal?

Comment 9

[Paul Adenot (:padenot)]

I was going to ask the Chrome folks at TPAC (next week). I'll post again here with a tentative date.

Comment 10

[Karl Tomlinson (:karlt)]

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.

Comment 11

[Paul Adenot (:padenot)]

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

Comment 12

[Paul Adenot (:padenot)]

I've sent an email to the google folks, we're going to sync up on removing those.

Comment 13

[Paul Adenot (:padenot)]

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.

Comment 14

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1148354 - Remove the doppler effect from the PannerNode.

Review commit: https://reviewboard.mozilla.org/r/257906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257906/

Comment 16

[Paul Adenot (:padenot)]

Attachment: Bug 1148354 - Remove the tests related to the doppler effect from mochitests.

Review commit: https://reviewboard.mozilla.org/r/257908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257908/

Comment 17

[Paul Adenot (:padenot)]

Attachment: Bug 1148354 - Add a web platform test to test for the non existence of removed methods.

Review commit: https://reviewboard.mozilla.org/r/257910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257910/

Comment 18

[Paul Adenot (:padenot)]

It's time! We're a bit late in fact :-).

Comment 19

[Karl Tomlinson (:karlt)]

https://bugs.chromium.org/p/chromium/issues/detail?id=439644
https://chromium.googlesource.com/chromium/src/+/9848564877f0057f4dcbc98bce061dfa17a9202c%5E%21/

Comment 20

[Karl Tomlinson (:karlt)]

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.

Comment 21

[Karl Tomlinson (:karlt)]

I'll be very pleased to see this go, thank you!

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8993080 [details]
Bug 1148354 - Remove the tests related to the doppler effect from mochitests.

https://reviewboard.mozilla.org/r/257908/#review264932

Comment 23

[Karl Tomlinson (:karlt)]

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

Comment 24

[Paul Adenot (:padenot)]

Hrm sorry, it looks like my some chunk of 1476744 went into this one.

Comment 25

[Paul Adenot (:padenot)]

Attachment: Bug 1148354 - Remove the doppler effect from the PannerNode.

Review commit: https://reviewboard.mozilla.org/r/258096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258096/

Comment 26

[Paul Adenot (:padenot)]

Comment on attachment 8993080 [details]
Bug 1148354 - Remove the tests related to the doppler effect from mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257908/diff/1-2/

Comment 27

[Paul Adenot (:padenot)]

Comment on attachment 8993081 [details]
Bug 1148354 - Add a web platform test to test for the non existence of removed methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257910/diff/1-2/

Comment 28

[Andrea Marchesini [:baku]]

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.

Comment 29

[Karl Tomlinson (:karlt)]

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.

Comment 30

[Paul Adenot (:padenot)]

Comment on attachment 8993315 [details]
Bug 1148354 - Remove the doppler effect from the PannerNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258096/diff/1-2/

Comment 31

[Paul Adenot (:padenot)]

Comment on attachment 8993080 [details]
Bug 1148354 - Remove the tests related to the doppler effect from mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257908/diff/2-3/

Comment 32

[Paul Adenot (:padenot)]

Comment on attachment 8993081 [details]
Bug 1148354 - Add a web platform test to test for the non existence of removed methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257910/diff/2-3/

Comment 33

[Pulsebot]

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

Comment 34

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12095 for changes under testing/web-platform/tests

Comment 35

[Andreea Pavel [:apavel]]

Backed out for failing  android at tests/dom/media/test/crashtests/doppler-1.html 

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=97d89ba5f93dad118fe54358ff70c28a3a33cde5

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=189187217&repo=autoland&lineNumber=2406 
https://treeherder.mozilla.org/logviewer.html#?job_id=189191407&repo=autoland&lineNumber=4732

Backout: https://hg.mozilla.org/integration/autoland/rev/060df6ed6995bdba888551568560087805b902e4

Comment 36

[Paul Adenot (:padenot)]

Comment on attachment 8993315 [details]
Bug 1148354 - Remove the doppler effect from the PannerNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258096/diff/2-3/

Comment 37

[Paul Adenot (:padenot)]

Comment on attachment 8993080 [details]
Bug 1148354 - Remove the tests related to the doppler effect from mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257908/diff/3-4/

Comment 38

[Paul Adenot (:padenot)]

Comment on attachment 8993081 [details]
Bug 1148354 - Add a web platform test to test for the non existence of removed methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257910/diff/3-4/

Comment 39

[Pulsebot]

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

Comment 40

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 41

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/512f6987b0b2
https://hg.mozilla.org/mozilla-central/rev/8b38f23b507f
https://hg.mozilla.org/mozilla-central/rev/5a481b37f45f

Comment 42

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 43

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 44

[Paul Adenot (:padenot)]

Clearing NI.

Comment 45

[Eric Shepherd [:sheppy]]

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.

Comment 46

[Kohei Yoshino]

Posted a site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/doppler-effect-support-has-been-removed-from-web-audio-api/