Closed Bug 1241499 Opened 6 years ago Closed 5 years ago

Initialize the HRTF database lazily

Categories

(Core :: Web Audio, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(1 file)

HRTF used to be the default, so it made sense to initialize it when the engine was created, but the situation changed, so it now makes sense to delay its initialization, because it is computationally and specially expensive.
Assignee: nobody → padenot
Blocks: 1241099
I'm pretty sure
Rank: 17
Priority: -- → P1
Duplicate of this bug: 1241099
Comment on attachment 8710463 [details]
MozReview Request: Bug 1241499 - Initialize the HRTF database lazily. r?karlt

https://reviewboard.mozilla.org/r/31775/#review30211

::: dom/media/webaudio/PannerNode.cpp:67
(Diff revision 1)
> +  void InitializeHRTFDatabase()

This function does more than just initialize the database.  How would you feel about CreateHRTFPanner()?

::: dom/media/webaudio/PannerNode.cpp:276
(Diff revision 1)
> +    // We can directly call in the engine here, because it will not touch
> +    // mHRTFPanner until it receives the parameter below via a control message.

The reason described here is what provides the thread safety, thanks, but calling into the engine is not the risk because the engine is owned by the stream for the lifetime of the stream.  Threads can be confusing, so I think it would be helpful to document this clearly.

Perhaps "We can set the engine's mHRTFPanner from the main thread here because the engine will not touch ..."

I like to document thread access to variables at their declaration.  Can you say something like "mHTRFPanner is set on the main thread before and accessed on the graph thread after mPanningModelFunction first changes." at the declaration of mHRTFPanner please?
Attachment #8710463 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/865b01fa7947
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.