496456 - Playback of small vorbis audio file induces issues within the player

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Aaron Train [:aaronmt]]

A regression in bug 461286 has resulted in a new problem.  

Testing with the aforementioned test file from bug 461286, (i.e.,
http://www.bluishcoder.co.nz/js8080/sound/shot.ogg), will play fine on
initial load. 

After playback, I am unable to play the file again (i.e, clicking the Play
button). 

In addition, I get a spinning throbber and the seek scrubber does not
reach the end of the timeline (from the short sound clip). The player goes all wonky until I decide to reload the tab.

Tested this on 1.9.1 branch and 1.9.2 trunk

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre)
Gecko/20090604 Shiretoko/3.5pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090604 Minefield/3.6a1pre

Expected results: Can playback audio file as many times as one may choose, no throbber, scrubber acts as expected

Actual results: Can only playback audio file on initial play, throbber, scrubber does not act as expected

Comment 1

[Damon Sicore (:damons)]

At this point, wouldn't hold back 1.9.1.  Will take patch, with risk assessment and a good test.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The ended event actually fires, which is why the test is passing.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems that the sound actually does play again if you ask it to, but no sound comes out.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Someone also pointed out that when you play the sound, you get two shots, but there should only be one.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Modifies test_bug495319 to replay the sound a few times. Fails to fire timeupdates, and the sound doesn't play enough times.

Comment 6

[Aaron Train [:aaronmt]]

(In reply to comment #3)
> It seems that the sound actually does play again if you ask it to, but no sound
> comes out.

Clicking Play the second time has no visual affect for me (i.e., no swap to the
Pause button, no scrubber movement). Is the Play button's event being fired?

Comment 7

[cajbir (:cajbir)]

Going back to commit 850c33bb7814 (which is when bug 461286 landed) the shot file plays once and can be replayed by pressing play multiple times. It's not a regression introduced by 461286. The UI does show the throbber constantly however but the sound plays and replays. Something landing after this breaks it.

Comment 8

[cajbir (:cajbir)]

The commmit for Bug 493958 (a983f0d35311) is the one that introduced the problem whereby the shot sound plays twice when pressing play.

Comment 9

[cajbir (:cajbir)]

Attachment: Fix

This fixes the issue where the end time of the playback was incorrect and the problem of the second playback request not working. It also fixes a problem where the playback sound is played twice which came up as a result of these fixes.

The problem of the throbber remaining at the first playback appears to be a controls issue. It seems to happen if only one timeupdate event is raised. The second playback has two timeupdate events (one due to the seek back to position zero, and the second for the completed playback time). I think it should be spun off into a separate bug.

There is another issue which I'll raise a bug for. The first playback has slightly incorrect sound. The second and subsequent playbacks are fine. This appears to be due to liboggplay returning different audio data on the first time through the file vs that obtained from seeking to position zero then reading. I suspect it's actually bug 496684 where we decode vorbis data before the headers are completely read. I'll raise a new bug or comment in that one depending on what I find.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (frame) {
+      audioData.AppendElements(frame->mAudioData);
+      audioTime += frame->mAudioData.Length() /
+        (float)mAudioRate / (float)mAudioChannels;
+    }

Why did you move this?

+        mCurrentFrameTime = PR_MAX(mCurrentFrameTime + mCallbackPeriod, mDuration / 1000.0);

If mDuration is -1, I guess this "works", but I'd prefer an explicit "if" statement.

What problem is this fixing?

Comment 11

[cajbir (:cajbir)]

> Why did you move this?

This code appends the current frames audio data to a temporary buffer. If it is in the original location when it breaks out of the loop the current frame in the 'frame' variable is the same frame as the audio data that was copied. This means the later code that prepends to the frames audio buffer is actually prepending the frames audio data to itself. This is why the sound was played twice.

By moving it to the beginning of the loop this ensures that the same frame never has it's own audio data copied. We don't need to save that frame since we are prepending to it.

> If mDuration is -1, I guess this "works", but I'd prefer an explicit "if"
statement.

The PR_MAX is clearer for the problem being fixed. This fixes the issue where at the end of playback of a small audio file the currentTime does not equal the duration. This is because mCurrentTime is computed by adding the callback period. In the case of audio, the first frame can contain more data than a single callback period. The MAX ensures that we always have a currentTime equal to the larger duration value in this case.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> By moving it to the beginning of the loop this ensures that the same frame
> never has it's own audio data copied. We don't need to save that frame since we
> are prepending to it.

Great, that makes sense.

> > If mDuration is -1, I guess this "works", but I'd prefer an explicit "if"
> statement.
> 
> The PR_MAX is clearer for the problem being fixed. This fixes the issue where
> at the end of playback of a small audio file the currentTime does not equal the
> duration. This is because mCurrentTime is computed by adding the callback
> period. In the case of audio, the first frame can contain more data than a
> single callback period. The MAX ensures that we always have a currentTime equal
> to the larger duration value in this case.

OK. But I'm saying that if mDuration is < 0 (i.e. "unknown") we shouldn't be using it in arithmetic or comparisons, we should just ignore it. It just so happens that PR_MAX(mCurrentFrameTime + mCallbackPeriod, -0.001) is going to give us mCurrentFrameTime + mCallbackPeriod, but that's really just luck and I don't think our code should do that.

Comment 13

[cajbir (:cajbir)]

I'm happy to change it but I dispute that it's just luck. It's the reason for picking the -1 sentinel to be able to do things like that.

Comment 14

[cajbir (:cajbir)]

Attachment: Address review comment

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/b3c4e464fed7

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on OS X and Windows with builds like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090801 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090801042759

Why the scrubber stops in the middle of the timeline? Is there another bug for this issue already?

The test has been checked in on bug 461286 with http://hg.mozilla.org/mozilla-central/rev/d70d123a365f. Can we mark in-testsuite+?

Comment 17

[cajbir (:cajbir)]

See comment 9 for the throbber issue. I don't think it has been spun of into a separate bug yet.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not worth checking for a regression range for an already fixed bug.