495161 - Wave decoder deadlock looping between PLAYING and BUFFERING

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

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

I found a deadlock in the Wave decoder. Basically the Wave decoder keeps cycling between PLAYING and BUFFERING without releasing the decoder lock. The cycling happens because PLAYING starts buffering if mStream->GetCachedDataEnd(mPlaybackPosition) < mPlaybackPosition + len, and sets mBufferingEndOffset to mPlaybackPosition + TimeToBytes(mBufferingWait.ToSeconds()), then BUFFERING stops buffering as soon as mStream->GetCachedDataEnd(mPlaybackPosition) >= mBufferingEndOffset. It is possible for these to be true at the same time, when TimeToBytes(mBufferingWait.ToSeconds()) < len.

So this patch makes sure that mBufferingEndOffset is always at least >= mPlaybackPosition + len, which means we'll buffer at least until we receive the data whose absence caused us to start buffering in the first place.

Comment 1

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

Attachment: fix

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 380016 [details] [diff] [review]
fix

Looks fine, but it seems like the patch could just be:

+            mBufferingEndOffset = PR_MAX(mPlaybackPosition + len, mBufferingEndOffset);

Comment 3

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

It could be, but I think it's clearer this way since mPlaybackPosition + len occurs in two places.

Comment 4

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

Comment on attachment 380016 [details] [diff] [review]
fix

We should get this into 1.9.1. Very small safe fix, avoids leaking a CPU-eating thread, so pretty important.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 380016 [details] [diff] [review]
fix

a191=beltzner

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/62fd2a24b8ff

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cd8b3f5c9d5e

Comment 8

[Damon Sicore (:damons)]

Are we good here?  Did we forget a fixed-1.9.1 somewhere?

Comment 9

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

Verified fixed on trunk and 1.9.1 based on checkins and green boxes.