Closed Bug 942751 Opened 11 years ago Closed 11 years ago

stop tone_player oscNode when done

Categories

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

x86_64
Linux
defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

(deleted), text/x-github-pull-request
etienne
: review+
Details
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
Assignee: nobody → karlt
Attached file pull request (deleted) —
Allowing the tone to play out fixes the major part of the release phase problem noticed in bug 943138 comment 6.
Attachment #8345130 - Flags: review?(etienne)
Blocks: 943138
Comment on attachment 8345130 [details] 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? :)
Attachment #8345130 - Flags: review?(etienne) → review+
(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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Is this required to fix bug 937505? If so, we need to nominate this for 1.3?
Flags: needinfo?(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.
blocking-b2g: --- → 1.3?
Flags: needinfo?(etienne)
triage: blocks a 1.3+ blocker. 1.3+
blocking-b2g: 1.3? → 1.3+
Priority: -- → P1
Uplifted 577f46a9a8f3f16540c007fa420bd5630c5f2ae3 to: v1.3: 7e84e2342b81cca00694efa5245d6e0790228f27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: