Playback of small vorbis audio file induces issues within the player

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Audio/Video
--
major
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: aaronmt, Assigned: cajbir)

Tracking

({regression, testcase})

1.9.1 Branch
mozilla1.9.2a1
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(status1.9.1 ?)

Details

(Whiteboard: [needs 191 approval], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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
Flags: blocking1.9.1?
At this point, wouldn't hold back 1.9.1.  Will take patch, with risk assessment and a good test.
Flags: blocking1.9.1? → blocking1.9.1-
The ended event actually fires, which is why the test is passing.
It seems that the sound actually does play again if you ask it to, but no sound comes out.
Someone also pointed out that when you play the sound, you get two shots, but there should only be one.
Created attachment 381650 [details] [diff] [review]
testcase

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

Comment 6

9 years ago
(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?
(Assignee)

Comment 7

9 years ago
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.
(Assignee)

Comment 8

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

Comment 9

9 years ago
Created attachment 382656 [details] [diff] [review]
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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attachment #382656 - Flags: superreview?(roc)
Attachment #382656 - Flags: review?(chris)
Attachment #382656 - Flags: review?(chris) → review+
+    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?
(Assignee)

Comment 11

9 years ago
> 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.
(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.
(Assignee)

Comment 13

9 years ago
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.
(Assignee)

Comment 14

9 years ago
Created attachment 382893 [details] [diff] [review]
Address review comment
Attachment #382656 - Attachment is obsolete: true
Attachment #382893 - Flags: superreview?(roc)
Attachment #382893 - Flags: review?(chris)
Attachment #382656 - Flags: superreview?(roc)
Attachment #382893 - Flags: superreview?(roc) → superreview+
Attachment #382893 - Flags: review?(chris) → review+
http://hg.mozilla.org/mozilla-central/rev/b3c4e464fed7
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: wanted1.9.1+
Whiteboard: [needs 191 approval]
Blocks: 499928
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+?
Status: RESOLVED → VERIFIED
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Keywords: regression, testcase
Target Milestone: --- → mozilla1.9.2a1
blocking1.9.1: ? → ---
(Assignee)

Comment 17

8 years ago
See comment 9 for the throbber issue. I don't think it has been spun of into a separate bug yet.
Flags: in-testsuite? → in-testsuite+
Not worth checking for a regression range for an already fixed bug.
Keywords: regressionwindow-wanted
Blocks: 497604
You need to log in before you can comment on or make changes to this bug.