Remove the doppler effect from the PannerNode

RESOLVED FIXED in Firefox 63

Status

()

P4
normal
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

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

32 Branch
mozilla63
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 8584507 [details] [diff] [review]
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).
Attachment #8584507 - Flags: review?(ehsan)

Comment 2

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

Comment 3

4 years ago
Created attachment 8589603 [details] [diff] [review]
Deprecate the doppler effect from the PannerNode. r=

This is built on top of bug 1148496.
Attachment #8589603 - Flags: review?(ehsan)

Comment 4

4 years ago
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+
(Assignee)

Comment 5

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

Comment 9

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

Comment 11

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

Comment 12

3 years ago
I've sent an email to the google folks, we're going to sync up on removing those.
(Assignee)

Updated

3 years ago
Depends on: 1269741
(Assignee)

Comment 13

3 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

4 months ago
It's time! We're a bit late in fact :-).
(Assignee)

Updated

4 months ago
Keywords: leave-open

Comment 20

4 months ago
mozreview-review
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 22

4 months ago
mozreview-review
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 23

4 months ago
mozreview-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+
(Assignee)

Comment 24

4 months ago
Hrm sorry, it looks like my some chunk of 1476744 went into this one.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8993079 - Attachment is obsolete: true

Comment 28

4 months ago
mozreview-review
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 29

4 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

4 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

4 months ago
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.

Comment 41

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/512f6987b0b2
https://hg.mozilla.org/mozilla-central/rev/8b38f23b507f
https://hg.mozilla.org/mozilla-central/rev/5a481b37f45f
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR was closed without merging
Upstream PR merged
(Assignee)

Comment 44

4 months ago
Clearing NI.
Flags: needinfo?(padenot)
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
Posted a site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/doppler-effect-support-has-been-removed-from-web-audio-api/
Keywords: site-compat
OS: Linux → All
Hardware: x86_64 → All
You need to log in before you can comment on or make changes to this bug.