Closed Bug 1476744 Opened 6 years ago Closed 6 years ago

Pull values from the AudioListener when computing PannerNode values

Categories

(Core :: Web Audio, enhancement, P2)

Product:
Core
Component:
Web Audio
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(1 file)

Bug 1476744 - Pull values from the AudioListener when computing PannerNode values.
59 bytes, text/x-review-board-request
karlt
: review+
Details 
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.
Summary: Pull values from the AudioListener when computing PannerNode Values → Pull values from the AudioListener when computing PannerNode values
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;
Assignee: nobody → padenot
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
Attachment #8993075 - Flags: review?(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?
Attachment #8993075 - Flags: review?(karlt) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/0ae34db4c34f
Pull values from the AudioListener when computing PannerNode values. r=karlt
Rank: 19
Priority: -- → P2
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/11b4729e92ec
Pull values from the AudioListener when computing PannerNode values. r=karlt
https://hg.mozilla.org/mozilla-central/rev/11b4729e92ec
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Clearing NI.
Flags: needinfo?(padenot)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12278 for changes under testing/web-platform/tests
Upstream PR was closed without merging
Depends on: 1477704
Blocks: 1518352
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: