Paul Adenot (:padenot) Assignee
	Description • 9 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

	Paul Adenot (:padenot) Assignee
	Comment 1 • 9 years ago

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

	(no longer active)
	Comment 2 • 9 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.)

	Paul Adenot (:padenot) Assignee
	Comment 3 • 9 years ago

This is built on top of bug 1148496.

	(no longer active)
	Comment 4 • 9 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

	Paul Adenot (:padenot) Assignee
	Comment 5 • 9 years ago

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

	Pulsebot
	Comment 6 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 9 years ago

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

	Maire Reavy [:mreavy]
	Comment 8 • 9 years ago

Should we set a target for eventual removal?

	Paul Adenot (:padenot) Assignee
	Comment 9 • 9 years ago

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

	Karl Tomlinson (:karlt)
	Comment 10 • 9 years ago

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.

	Paul Adenot (:padenot) Assignee
	Comment 11 • 8 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

	Paul Adenot (:padenot) Assignee
	Comment 12 • 8 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 13 • 8 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.

	Bulk Bug Changes for mreavy's org
	Comment 14 • 7 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 18 • 6 years ago

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

	Karl Tomlinson (:karlt)
	Comment 19 • 6 years ago

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

	Karl Tomlinson (:karlt)
	Comment 20 • 6 years 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.

	Karl Tomlinson (:karlt)
	Comment 21 • 6 years ago

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

	Karl Tomlinson (:karlt)
	Comment 22 • 6 years 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

	Karl Tomlinson (:karlt)
	Comment 23 • 6 years 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

	Paul Adenot (:padenot) Assignee
	Comment 24 • 6 years ago

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

	Andrea Marchesini [:baku]
	Comment 28 • 6 years 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.

	Karl Tomlinson (:karlt)
	Comment 29 • 6 years 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.

	Pulsebot
	Comment 33 • 6 years 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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 34 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 35 • 6 years ago

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

	Pulsebot
	Comment 39 • 6 years 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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 40 • 6 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 41 • 6 years 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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 42 • 6 years ago

Upstream PR was closed without merging

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 43 • 6 years ago

Upstream PR merged

	Paul Adenot (:padenot) Assignee
	Comment 44 • 6 years ago

Clearing NI.

	Eric Shepherd [:sheppy]
	Comment 45 • 6 years ago

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.

	Kohei Yoshino [:kohei]
	Comment 46 • 6 years ago

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