1476744 - Pull values from the AudioListener when computing PannerNode values

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement, P2)

Description

[Paul Adenot (:padenot)]

For now we're pushing them to all PannerNodes when calling the methods on the AudioListener, and pulling them from PannerNodes is more in line with how I'm going to do things in bug 1283029.

I'm going with more or less the same design as our AudioNode/AudioNodeEngine here, it's really the same.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8993075 [details]
Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

https://reviewboard.mozilla.org/r/257892/#review264820


Code analysis found 5 defects in this patch:
 - 5 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webaudio/AudioListener.h:79
(Diff revision 1)
>  
>  private:
> +  void SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter aParameter,
> +                               const ThreeDPoint& aValue);
> +
>    ~AudioListener() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~AudioListener() {}
  ^                ~~
                   = default;

::: dom/media/webaudio/AudioListener.h:79
(Diff revision 1)
>  
>  private:
> +  void SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter aParameter,
> +                               const ThreeDPoint& aValue);
> +
>    ~AudioListener() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~AudioListener() {}
  ^                ~~
                   = default;

::: dom/media/webaudio/AudioListener.h:79
(Diff revision 1)
>  
>  private:
> +  void SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter aParameter,
> +                               const ThreeDPoint& aValue);
> +
>    ~AudioListener() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~AudioListener() {}
  ^                ~~
                   = default;

::: dom/media/webaudio/AudioListener.h:79
(Diff revision 1)
>  
>  private:
> +  void SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter aParameter,
> +                               const ThreeDPoint& aValue);
> +
>    ~AudioListener() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~AudioListener() {}
  ^                ~~
                   = default;

::: dom/media/webaudio/AudioListener.h:79
(Diff revision 1)
>  
>  private:
> +  void SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter aParameter,
> +                               const ThreeDPoint& aValue);
> +
>    ~AudioListener() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~AudioListener() {}
  ^                ~~
                   = default;

Comment 3

[Paul Adenot (:padenot)]

Comment on attachment 8993075 [details]
Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

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

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8993075 [details]
Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

https://reviewboard.mozilla.org/r/257892/#review264916

This looks good, thanks, but I think this will be easier to review with AudioListener::RegisterPannerNode() and SendThreeDPointParameterToStream() removed in this patch.

::: dom/media/webaudio/AudioListener.cpp:126
(Diff revision 2)
> +  mPosition.y = aY;
> +  mPosition.z = aZ;
> +  SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter::POSITION, mPosition);
> +}
> +
> +void AudioListener::SendListenerEngineEvent(AudioListenerEngine::AudioListenerParameter aParameter, const ThreeDPoint& aValue)

80

::: dom/media/webaudio/AudioListener.cpp:148
(Diff revision 2)
> +    AudioListenerEngine* mEngine;
> +    AudioListenerEngine::AudioListenerParameter mParameter;
> +    ThreeDPoint mValue;
> +  };
> +
> +  mContext->DestinationStream()->GraphImpl()->AppendMessage(MakeUnique<Message>(mEngine.get(), aParameter, aValue));

80 columns.

::: dom/media/webaudio/PannerNode.cpp:45
(Diff revision 2)
>  NS_IMPL_RELEASE_INHERITED(PannerNode, AudioNode)
>  
>  class PannerNodeEngine final : public AudioNodeEngine
>  {
>  public:
> -  explicit PannerNodeEngine(AudioNode* aNode, AudioDestinationNode* aDestination, AudioListener* aListener)
> +  explicit PannerNodeEngine(AudioNode* aNode, AudioDestinationNode* aDestination, const AudioListenerEngine* aListenerEngine)

80 columns

::: dom/media/webaudio/PannerNode.cpp:256
(Diff revision 2)
>    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;
> +  const AudioListenerEngine* mListenerEngine;

Please document what keeps this alive long enough.

::: dom/media/webaudio/PannerNode.cpp:298
(Diff revision 2)
>    , mConeInnerAngle(360.)
>    , mConeOuterAngle(360.)
>    , mConeOuterGain(0.)
>  {
>    mStream = AudioNodeStream::Create(aContext,
> -                                    new PannerNodeEngine(this, aContext->Destination(), aContext->Listener()),
> +                                    new PannerNodeEngine(this, aContext->Destination(), aContext->Listener()->Engine()),

80 columns

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8993075 [details]
Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

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

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8993075 [details]
Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

https://reviewboard.mozilla.org/r/257892/#review265288

::: dom/media/webaudio/AudioListener.h:35
(Diff revision 3)
> +    FRONT,
> +    RIGHT
> +  };
> +  AudioListenerEngine();
> +  void RecvListenerEngineEvent(
> +    AudioListenerEngine::AudioListenerParameter aParameter,

I don't think AudioListenerEngine:: is required here.  At least it is not with clang 5.0.2.

::: dom/media/webaudio/AudioListener.cpp:29
(Diff revision 3)
> +{
> +}
> +
> +void
> +AudioListenerEngine::RecvListenerEngineEvent(
> +  AudioListenerEngine::AudioListenerParameter aParameter,

AudioListenerEngine:: not required.

::: dom/media/webaudio/PannerNode.h
(Diff revision 3)
> -    LISTENER_FRONT_VECTOR, // unit length
> -    LISTENER_RIGHT_VECTOR, // unit length, orthogonal to LISTENER_FRONT_VECTOR

Can you copy the comments to the new parameter definitions, please?

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8993075 [details]
Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae34db4c34f
Pull values from the AudioListener when computing PannerNode values. r=karlt

Comment 9

[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 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11b4729e92ec
Pull values from the AudioListener when computing PannerNode values. r=karlt

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/11b4729e92ec

Comment 12

[Paul Adenot (:padenot)]

Clearing NI.

Comment 13

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

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

Comment 14

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

Upstream PR was closed without merging

Comment 15

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

Upstream PR merged