942751 - stop tone_player oscNode when done

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P1)

Description

[Karl Tomlinson (:karlt)]

oscNode is stopped when shortPress is set [1] but otherwise not stopped before
being disconnected and released [2].

You would expect unobservable subgraphs to be garbage collected, but bug
897796 means that this doesn't happen.

Currently a stop() is required before the nodes can be collected.

[1] https://github.com/mozilla-b2g/gaia/blob/add12c96c3e9281baa7f483f7ba751ce63df5749/apps/communications/dialer/js/tone_player.js#L48
[2] https://github.com/mozilla-b2g/gaia/blob/add12c96c3e9281baa7f483f7ba751ce63df5749/apps/communications/dialer/js/tone_player.js#L59

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: pull request

Allowing the tone to play out fixes the major part of the release phase problem noticed in bug 943138 comment 6.

Comment 2

[Etienne Segonzac (:etienne)]

Comment on attachment 8345130 [details] [review]
pull request

r=me with one comment:

Connecting the gain node last was done deliberately as part of bug 924454.
Can we keep doing this or get an explanation of why we shouldn't? :)

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to Etienne Segonzac (:etienne) from comment #2)
> Connecting the gain node last was done deliberately as part of bug 924454.
> Can we keep doing this or get an explanation of why we shouldn't? :)

Ah, I thought that must have been accidentally included in the "Make the busy tone longer and clearer" commit, because changing the order of connections makes no difference to the sound.

I've read bug 924454 comment 13 now.  That comment is correct that we don't hear the tones at |currentTime|, but later when the various operations on the graph are processed.  However, all operations are processed atomically when the js returns to the event loop, so the order of connections makes no difference.

I suspect the start of the first tone sounds different to the other tones because there is more processing happening during the event that starts the first tone, than in the events that run the other tones.  |currentTime| is set in a previous event, so time spent in the telephone_helper event even before starting the tone means that the difference between currentTime and the time when sound starts increases.  The attack is unfortunately scheduled to start at currentTime regardless of when the sound actually starts.  As the difference in times increases, more of the attack phase has passed, and the tone starts at full volume instead of fading in gently.
I'm addressing this issue in bug 943138.

I moved the envelope gain node connection so that all code to set up this node is together.

Comment 4

[Etienne Segonzac (:etienne)]

Thanks!

https://github.com/mozilla-b2g/gaia/commit/577f46a9a8f3f16540c007fa420bd5630c5f2ae3

Comment 5

[Jason Smith [:jsmith]]

Is this required to fix bug 937505? If so, we need to nominate this for 1.3?

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Jason Smith [:jsmith] from comment #5)
> Is this required to fix bug 937505? If so, we need to nominate this for 1.3?

Yep trying to keep track of what needs to be uplifted with dependencies.

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

triage: blocks a 1.3+ blocker. 1.3+

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 577f46a9a8f3f16540c007fa420bd5630c5f2ae3 to:
v1.3: 7e84e2342b81cca00694efa5245d6e0790228f27