628665 - Unnecessary buffering when playing at start of load

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Sander Plas]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre

Probably related to bug 623637 . 

Some (live) streams take a long time to start playing because of buffering problems. (in fact, they do start playing but almost immediately run out of buffered data causing it to buffer for +/- 35 seconds before it starts playing again)

The problem of bug 623637 has been solved for the simple case of a static html audio tag with 'autoplay' enabled. 

However, the same kind of problem still seems to occur when an audio element is created through javascript and instead of using 'autoplay', play() is immediately called. Example: http://www.oele.net/audio_js_start.html

It seems that Opera 11 and Firefox 3.6 interpret a javascript 'play' call as
"start playing as soon as enough data is buffered" whereas Firefox 4.0b10pre
immediately starts playing. 

I'm not entirely sure what the "correct" behaviour is but Opera's and Firefox
3.6's behaviour seems more useful to me - i think it's rather pointless to ever
allow a media element to start "playing" without having enough buffered data.

Reproducible: Always

Steps to Reproduce:
Try http://www.oele.net/audio_js_start.html in Opera 11, Firefox 3.6 and Firefox 4.0b10pre. 
Actual Results:  
The stream starts playing almost immediately in Opera 11 + Firefox 3.6 but only after 35 seconds on Firefox 4.0b10pre. 

Expected Results:  
Firefox 4 should start playing as fast as the other browsers.

Comment 1

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

Attachment: Testcase which logs events

A reliable way to start playback would be to add the @autoplay attribute to your audio element, then playback would begin as soon as it had enough data to play smoothly. e.g. add "ymAudioElement.autoplay = true;" to your script. For me, this results in a smooth stream start about 3 seconds after loading the page. That would be a reasonable work around until we change behaviour here.

The testcase starts playback before the download has had a chance to download anything, and we're switching to buffering mode straight away. We don't really need to buffer in this case, and it makes us look bad compared to the other browsers.

If I log the events which are sent when loading this stream (as per the attached edited testcase) in different browsers, I get:

Chrome 9.0.597.67
emptied
play
waiting
loadedmetadata
loadeddata
canplay
canplaythrough
playing

Firefox 4b10pre 2011-01-20
emptied
play
loadedmetadata
loadeddata
canplay
playing
waiting
canplay
playing
canplaythrough

Opera 10.63
emptied
play
waiting
loadedmetadata
loadeddata
canplay
playing
canplaythrough

Our behaviour is indeed different to Opera and Chrome. They pretty much begin playback immediately, whereas we fire canplay, start playback, and then immediately stop to buffer for 30s before resuming playback.

We should really be firing a "waiting" event as soon as play() is called when we're not in readyState >= HAVE_FUTURE_DATA too.

Because the decode loop now runs ahead of the playback position, it looks like when the state machine starts up we're immediately starting playback (like we should in this case) but the decode loop is immediately exhausting the downloaded data (because it runs ahead), and triggering buffering mode. I've noticed we can hit this case immediately after we finish a seek as well. It would be nice if we didn't stop to buffer when data is exhausted while we're just starting up the decode on a new request.

Comment 2

[u279076]

Reproducible in the latest nightly.  Marking as NEW.  Thanks for filing this bug Sander, and thanks for the testcase Chris.

Comment 3

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

Attachment: Patch 1/1: fire waiting when play() called in readyState HAVE_NOTHING

* Fire a waiting event when HTMLMediaElement.play() is called when in readyState HAVE_NOTHING.
* Brings "waiting" event behaviour into compliance with the spec [1], and into conformance with our behaviour with the other browsers.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-play

Comment 4

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

Attachment: Patch 2/2: buffering fast exit

* Reduce our threshold to exit buffering if we enter buffering within 2s of beginning decoding. Seems to be the lesser of all evils.
* Resurrect code which put us into buffering mode when we're low on decoded data.
* Go into buffering mode when we're low on decoded data, and we're also low on undecoded data (as reported by the GetBuffered() implementation).
* Greenish on TryServer: http://tbpl.mozilla.org/?tree=MozillaTry&rev=13fd0e98d8ba

Comment 5

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

+static const int QUICK_BUFFER_EXIT_MS = 2000;

How did you choose this value? I was thinking of a value more like 100-500ms.

If it's not too hard to do, it might be better to split this patch up into three patches: 1) resurrect code to buffer when we're low on decoded data, 2) add code to only buffer if we have less than LOW_DATA_THRESHOLD_MS per GetBuffered(), 3) the "fast exit" code.

Is this something we need to take for FF4? If we don't absolutely need it I'd like to take it post-FF4.

Comment 6

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

(In reply to comment #5)
> +static const int QUICK_BUFFER_EXIT_MS = 2000;
> 
> How did you choose this value? I was thinking of a value more like 100-500ms.

I tested with a local bandwidth limited server and it seemed to work.

If the download rate is similar to the decode rate, then if QUICK_BUFFER_EXIT_MS is too low, we can escape buffering when HasLowDecodedData() and HasLowUndecodedData() still return true. Note HasLowUndecodedData() uses a 2000ms threshold too. If those return true we'll jump straight back into buffering, but because QUICK_BUFFER_EXIT_MS is low we'll then jump straight back out. So we'll end up jumping back and forth between buffering and not buffering, and playback stutters.

Basically we can't have the logic to exit buffering (the check which uses QUICK_BUFFER_EXIT_MS) disagree with the logic to enter buffering (HasLowDecodedData() and HasLowUndecodedData()).

I think we can get around this by detecting when we've just come out of buffering-quick-exit in our entering-buffering logic, and special casing it, then we can use a low QUICK_BUFFER_EXIT_MS. I'm just testing now...

> If it's not too hard to do, it might be better to split this patch up into
> three patches: 1) resurrect code to buffer when we're low on decoded data, 2)
> add code to only buffer if we have less than LOW_DATA_THRESHOLD_MS per
> GetBuffered(), 3) the "fast exit" code.

It's only a problem in that each individual part is can't stand by itself as a complete changeset; buffering only behaves correctly when all the pieces are put together.

> Is this something we need to take for FF4? If we don't absolutely need it I'd
> like to take it post-FF4.

We don't absolutely need to take it to FF4, it fixes an annoying case, but if we have a release soon after FF4, then it can wait until then...

Comment 7

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

(In reply to comment #6)
> I tested with a local bandwidth limited server and it seemed to work.
> 
> If the download rate is similar to the decode rate, then if
> QUICK_BUFFER_EXIT_MS is too low, we can escape buffering when
> HasLowDecodedData() and HasLowUndecodedData() still return true. Note
> HasLowUndecodedData() uses a 2000ms threshold too. If those return true we'll
> jump straight back into buffering, but because QUICK_BUFFER_EXIT_MS is low
> we'll then jump straight back out. So we'll end up jumping back and forth
> between buffering and not buffering, and playback stutters.

Maybe in "quick buffering" mode we should simply buffer only when/while HasLowDecodedData()?

I think stuttery playback during QUICK_BUFFER_THRESHOLD is the price we have to pay for aggressive playback.

> It's only a problem in that each individual part is can't stand by itself as a
> complete changeset; buffering only behaves correctly when all the pieces are
> put together.

If that's the only problem then I think we should break up the patches, so they'll be easier to understand.

> We don't absolutely need to take it to FF4, it fixes an annoying case, but if
> we have a release soon after FF4, then it can wait until then...

Then it sounds to me like this shouldn't go into FF4 at this point.

Comment 8

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

Attachment: Patch 2/5: Resurrect HasLowDecodedData()

* Go into buffering mode when we're low on decoded data.

Comment 9

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

Attachment: Patch 3/5: Buffer media when low on undecoded data

* Buffer media when low on undecoded data, as per the GetBuffered() implementation.

Comment 10

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

Attachment: Patch 4/5: Quick exit buffering

* If we enter buffering within 2s of starting load, go into "quick buffering" mode, where we exit as soon as !HasLowDecodedData().
* If we re-enter buffering after exiting "quick buffering" mode, just go into normal buffering. This stops us flip-flopping between buffering and not buffering when the download rate is similar to decode rate.

Comment 11

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

Attachment: Patch 5/5: Don't enqueue late Theora frames for playback

When we're about to enter buffering, the audio decode can block, causing the video decode to fall behind the current playback position. We then continue to decode video after we enter buffering, but fill up the video frame queue with frames that are behind the current playback position. When we come to play after resuming from buffering, and the queue is then filled with late frames, all the late frames will be dropped, and we will then start skipping to the next keyframe, since we're trying to play but now have an empty video queue.

This patch ensures the Ogg reader doesn't enqueue any frames which are behind the playback position. The WebM reader already does this, and so doesn't suffer from this problem.

Comment 12

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

These patches I just uploaded are greenish on Try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=7a3f7aaf3d5a

Comment 13

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

Comment on attachment 513910 [details] [diff] [review]
Patch 2/5: Resurrect HasLowDecodedData()

As discussed, we should move the buffering check into AdvanceFrame, where we know how long we're going to wait.

Comment 14

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

Comment on attachment 513911 [details] [diff] [review]
Patch 3/5: Buffer media when low on undecoded data

+static const PRInt64 LOW_DATA_THRESHOLD_MS = 2 * AMPLE_AUDIO_MS;

I think this should just be its own value. I think 2 seconds is a bit low ... we don't want to play two seconds, buffer, play two seconds, buffer, ..., do we? OTOH maybe it doesn't matter since we're in "the user is going to have a bad experience no matter what" territory. But I'd make it five seconds, maybe.

Comment 15

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

+static const double QUICK_BUFFER_THRESHOLD = 2.0;

I think this should be a PRUint32 in ms to be consistent with the other tunables.

I'm not sure whether it's good to prevent more than one quick-buffer episode per play/seek. I understand the desire not to bounce between states, but it seems to me that stuttering (for at most two seconds, as currently configured) is the price of having as-fast-as-possible playback during the ramp-up phase.

Comment 16

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

If we allow only one stutter within the 2s interval, then I think we need to make sure that (after the changes of part 2 have been done) our check for HasLowDecodedData in quick-buffering uses a higher threshold than the usual threshold for entering buffering.

Comment 17

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

Attachment: Patch 2: Resurrect HasLowDecodedData()

* Resurrect HasLodDecodedData(). Go into buffering when it looks like we won't have enough data to play through to the next frame, plus a bit.

Comment 18

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

Attachment: Patch 3/5: Buffer media when low on undecoded data

Rebased ontop of changes in lower patches. One issue I didn't address here is ensuring that GetBuffered() was threadsafe. I'll do that in a subsequent patch. Carrying forward r=roc.

Comment 19

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

Attachment: Patch 4/5: Quick exit buffering

* With check for HasLowDecodedData() in quick buffering exit using higher threshold as requested.

Comment 20

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

Attachment: Patch 6: squish compiler warning

I'm getting sick of seeing compiler warnings in content/media/ due to potential loss of precision which we don't care about. This patch adds some casts to suppress them.

Comment 21

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

FYI, I have based these patches I just uploaded on top of the patches in bug 580531.

Comment 22

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

Comment on attachment 516759 [details] [diff] [review]
Patch 2: Resurrect HasLowDecodedData()

   if (mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING) {
-    if (!IsPlaying()) {
-      StartPlayback();
-      mDecoder->GetMonitor().NotifyAll();
-    }
 
     if (HasAudio() && mAudioStartTime == -1 && !mAudioCompleted) {

Remove blank line

Comment 23

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

Comment on attachment 516762 [details] [diff] [review]
Patch 4/5: Quick exit buffering

I still think we might be better off allowing "flipping" in and out of quickbuffering during the first two seconds, but we shall see.

Comment 25

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

http://hg.mozilla.org/mozilla-central/rev/1e33df0cbd02
http://hg.mozilla.org/mozilla-central/rev/c00dc9b33e1d
http://hg.mozilla.org/mozilla-central/rev/32dfc1dcdc83
http://hg.mozilla.org/mozilla-central/rev/1591cd866727
http://hg.mozilla.org/mozilla-central/rev/6ef5427cdd97

Comment 26

[u279076]

I now get the following when running the attached testcase:
emptied
play
waiting
loadedmetadata
loadeddata
canplay
playing
canplaythrough

Is this expected?

Comment 27

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

Yes. That also matches behaviour with Chrome and Opera.

Comment 28

[u279076]

Thanks -- VERIFIED FIXED