Closed Bug 1203836 Opened 6 years ago Closed 6 years ago

Clicks with looping media element source stream

Categories

(Core :: Web Audio, defect, P1)

43 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 --- fixed

People

(Reporter: karlt, Assigned: padenot)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

Load https://bugzilla.mozilla.org/attachment.cgi?id=831235

Expected:
ting ting ting ting ...

Actual:
ting click ting click ting click ting click ...
This sounds similar to what I hear trying to reproduce bug 1203671, but I don't have reliable steps to reproduce that, so I can't be sure it is the same bug.
See Also: → 1203671
Assignee: nobody → padenot
This fixes this bug and also bug 1203671. I'm trying to think of a test now.
Attachment #8659891 - Flags: review?(karlt)
Attachment #8659891 - Flags: review?(karlt) → review+
Component: WebRTC: Audio/Video → Web Audio
Attached patch testSplinter Review
I verified this fails without the patch, and passes with the patch.
Attachment #8661390 - Flags: review?(karlt)
Comment on attachment 8661390 [details] [diff] [review]
test


Hmm.  I clearly recalled long ago a policy of no review requirement for tests
in order not to slow down development of tests.  (I guess it is assumed that
people maintain their own tests and I would usually request the author's
review when modifying an existing test.)  However, I can't find that policy
now and looking at recent commits only people who have been around a long time
commit tests without review, so I wonder if there is a policy now.

Regardless, I'm happy to look over if that's helpful.

Using media recorder and then comparing the played to expected seems a good
approach.

IIUC the bug was with silence chunks when not all chunks were silent, so I
guess it would demonstrate when the audio starts or ends at a point not on a
web audio block boundary?

But the audio in this test is one second long so I guess the non-block
alignment comes from resampling due to the difference in sample rates?  Can
the OfflineAudioContext be given a rate unlikely to match the online rate?

Perhaps the recording could be shorter given the fact that the bug happens
only at start and stop.

>+    if (frequencyArray[i] > -an.minDecibels + 10) {

minDecibels is used for getByteFrequencyData().
Is its default value relevant here?
Inverting y = 20 log_10(x) for y = 110 gives a magnitude of about 316000.
That might detect uninitialized values I guess, but can the test be tighter
than that?  I assume we expect significantly < 1?

>+      // We want to check the we have the expected audio for at least one loop of
>+      // the HTMLMediaElement. The file is one second, and we use the default FFT
>+      // size. Double the number for good measure.
>+      var checksCount = Math.round((ac.sampleRate / an.fftSize) * 2);
>+      audioElement.onplaying = function() {
>+        audioElement.ontimeupdate = function() {
>+          if (checkFrequency(an)) {
>+            ok(true, "Found correct audio signal during analysis");
>+            if (checksCount-- == 0) {
>+              audioElement.ontimeupdate = null;
>+              SimpleTest.finish()

I don't understand checksCount here.
timeupdate is dispatched every 15 to 250 ms, which is not related to fftSize.
Perhaps use setInterval or a scriptprocessor aligned with fftSize?
Attachment #8661390 - Flags: review?(karlt) → review+
Perhaps a scriptprocessor connected to the media element source stream could send its buffers through offline contexts for analysis.  Then every piece of audio can be checked, but I guess the window would kind of ignore half of it, so ideally previous blocks need to be included too, and so it gets complicated.
Bug 983062 may make it harder to write a reliable test.
If that is fixed or the stream to be resampled has a long enough tail of zeros then output could be directly compared with expected except for an offset from the scheduling of play().  That offset could be calculated by finding the centre of energy of the output and expected.
(In reply to Karl Tomlinson (ni?:karlt) from comment #6)
> Comment on attachment 8661390 [details] [diff] [review]
> test
> 
> 
> Hmm.  I clearly recalled long ago a policy of no review requirement for tests
> in order not to slow down development of tests.  (I guess it is assumed that
> people maintain their own tests and I would usually request the author's
> review when modifying an existing test.)  However, I can't find that policy
> now and looking at recent commits only people who have been around a long
> time
> commit tests without review, so I wonder if there is a policy now.

I've been having my tests reviewed for as long as I remember (four years maybe ?), but I don't know what is the official policy.

> 
> Regardless, I'm happy to look over if that's helpful.
> 
> Using media recorder and then comparing the played to expected seems a good
> approach.
> 
> IIUC the bug was with silence chunks when not all chunks were silent, so I
> guess it would demonstrate when the audio starts or ends at a point not on a
> web audio block boundary?

The idea here is that the looping of the HTMLMediaElement is not seamless, so there is null chunks in the 

> But the audio in this test is one second long so I guess the non-block
> alignment comes from resampling due to the difference in sample rates?  Can
> the OfflineAudioContext be given a rate unlikely to match the online rate?
> 
> Perhaps the recording could be shorter given the fact that the bug happens
> only at start and stop.
> 
> >+    if (frequencyArray[i] > -an.minDecibels + 10) {
> 
> minDecibels is used for getByteFrequencyData().
> Is its default value relevant here?
> Inverting y = 20 log_10(x) for y = 110 gives a magnitude of about 316000.
> That might detect uninitialized values I guess, but can the test be tighter
> than that?  I assume we expect significantly < 1?

Yes, it can be anything really. When enabling the debugging canvas, and when the glitch happens, all bins are at maximum value (i.e. the canvas is all black, and then goes down slowly because of the windowing). That said, having a '-' in there is a mistake. I meant "10 more than the minimum", as you say.

> >+      // We want to check the we have the expected audio for at least one loop of
> >+      // the HTMLMediaElement. The file is one second, and we use the default FFT
> >+      // size. Double the number for good measure.
> >+      var checksCount = Math.round((ac.sampleRate / an.fftSize) * 2);
> >+      audioElement.onplaying = function() {
> >+        audioElement.ontimeupdate = function() {
> >+          if (checkFrequency(an)) {
> >+            ok(true, "Found correct audio signal during analysis");
> >+            if (checksCount-- == 0) {
> >+              audioElement.ontimeupdate = null;
> >+              SimpleTest.finish()
> 
> I don't understand checksCount here.
> timeupdate is dispatched every 15 to 250 ms, which is not related to fftSize.
> Perhaps use setInterval or a scriptprocessor aligned with fftSize?

Yeah it's wrong, this is from an earlier version. I'm going to go with a ScriptProcessorNode.
(In reply to Paul Adenot (:padenot) from comment #9)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #6)
> > I don't understand checksCount here.
> > timeupdate is dispatched every 15 to 250 ms, which is not related to fftSize.
> > Perhaps use setInterval or a scriptprocessor aligned with fftSize?
> 
> Yeah it's wrong, this is from an earlier version. I'm going to go with a
> ScriptProcessorNode.

In fact I can just use `HTMLMediaElement.currentTime`.
https://hg.mozilla.org/mozilla-central/rev/68d05b03e9f1
https://hg.mozilla.org/mozilla-central/rev/abdbbdfa88e8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Tracking for 43. Paul, is this something you may want to uplift to aurora?
Flags: needinfo?(padenot)
Indeed, I almost forgot about it !
Flags: needinfo?(padenot)
Comment on attachment 8659891 [details] [diff] [review]
Properly handle silent chunks in AudioNodeExternalInputStream. r=

Approval Request Comment
[Feature/regressing bug #]: bug 901633
[User impact if declined]: audio glitch when playing an HTMLMediaElement through an AudioContext. This is rather common.
[Describe test coverage new/current, TreeHerder]: a mochitest has been added
[Risks and why]: little risk, cause and fix are well understood
[String/UUID change made/needed]: none
Attachment #8659891 - Flags: approval-mozilla-aurora?
Comment on attachment 8661390 [details] [diff] [review]
test

(See previous comment, this is just the test).
Attachment #8661390 - Flags: approval-mozilla-aurora?
Comment on attachment 8659891 [details] [diff] [review]
Properly handle silent chunks in AudioNodeExternalInputStream. r=

Fix for audio issue, comes with tests!
Attachment #8659891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8661390 [details] [diff] [review]
test

Let's also uplift the tests for this fix to aurora.
Attachment #8661390 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1203671
You need to log in before you can comment on or make changes to this bug.