Closed Bug 556893 Opened 14 years ago Closed 14 years ago

Broke readyState transition to HAVE_ENOUGH_DATA calculation

Categories

(Core :: Audio/Video, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: cpearce, Assigned: cpearce)

References

()

Details

Attachments

(1 file)

We're not playing the video at the daily motion URL smoothly since the new backend landed. I suspect the buffering logic is firing too often. We're also ending up in COMPLETED state with insufficent frames to play the remaining media.
Could this also be responsible for my audio problems?  Audio now sounds like mostly a steady stream of static.
Any daily motion videos I try to play won't actually play until the entire video is downloaded. How are you seeing the buffering?
Not in my case.  Video was buffered to 60 % before it started playing.  Audio, however, sounds like crap: stuttering, static-sounding (can't make out anything): http://www.dailymotion.com/openvideodemo

One thing though, if I leave the video playing and switch to another tab, audio playback is significantly better; thus my suspicion that the audio component is not given anywhere near the priority it should.

Video playback is very jerky, but that's because this Firefox video backend has always demanded way too much CPU.

What Adobe does is skip video frames, but not compromise audio playback.

Significantly, plugins perform many orders of magnitude better than Firefox's built-in decoder; therefore, there seems to be a problem with priority.  Even pure audio OGG streams stutter with the built-in decoder.

Are there even any assembly optimizations with the new decoder?  The old decoder had some optimizations.
(In reply to comment #2)
> Any daily motion videos I try to play won't actually play until the entire
> video is downloaded.

I also see this behaviour. On the same video yesterday I saw the audio loop end early, stopping video playback altogether.

> How are you seeing the buffering?

By setting breakpoints in DecodeLoop() on the lines which set "mBufferExhausted = PR_TRUE;".

I will look into this on Tuesday.
On a little audio app that I have this is what happens:

Load starts, and after a couple of seconds I get the "canplaythrough" event.

As soon as I do that, I call play() on the element and it immediately generates the "play" event and then the "waiting" event.

It then buffers a huge amount of data and eventually calls "canplaythrough" again.

When that happens the play() call actually plays (although it doesn't generate the "play" event again since I guess it was still in the play state?)

See bug 557426 for a url that's easy to debug.
It seems the DailyMotion controls don't start playback unless we're in readyState >= HAVE_ENOUGH_DATA.

The new ogg decoder backend does not update nsOggDecoder::mPlaybackPosition, so the calculations in nsHTMLMediaElement::UpdateReadyStateForData() which determine whether to change into HAVE_ENOUGH_DATA state has bad input, i.e. they're wrong. We must change the new decoder to set nsOggDecoder::mPlaybackPosition somewhat accurately during playback.
Summary: New ogg decoder buffering logic is trigger happy → Broke readyState transition to HAVE_ENOUGH_DATA calculation
A more thorough explanation...

I think the logic in the nsHTMLMediaElement::UpdateReadyStateForData() readyState calculation is conservative but correct.

The reason that DailyMotion appears to buffer for longer in trunk builds, is that the download is slower, or perhaps the download is being reported as being slower, and so we're not getting into HAVE_ENOUGH_DATA as soon as in FF3.6, and DailyMotion won't play unless it's video's readyState is HAVE_ENOUGH_DATA.

I'm not sure why our download appears slower in trunk builds. I'm doing an opt build to see if that makes a difference. Possibly we've got more lock contention.

So currently the only time we update nsOggDecoder::mPlaybackPosition is in nsOggDecoder::StartProgressUpdates(); after a we've loaded headers, and after a seek. So the readyState transition calculation in nsHTMLMediaElement::UpdateReadyStateForData() won't see how far we are from the end of playback, it'll always assume we've got the entire remaining resource to play. 

This means that if the download slows down, such that we're no longer downloading at a rate fast enough to playthrough without buffering, the calculation in nsHTMLMediaElement::UpdateReadyStateForData() can't detect this, and we won't drop from HAVE_ENOUGH_DATA back to HAVE_FUTURE_DATA (perhaps until we actually switch to BUFFERING state in the play state machine).

e.g. if we start playback at 50s from the end, and at 30s from the end of playback we calculate that the download will take 40s to download, we'll still think "50s to play, and 40s to download => HAVE_ENOUGH_DATA". But we should actually switch to HAVE_FUTURE_DATA.

When we play a frame/audio sample, we should update nsOggDecoder::mPlaybackPosition to the approximate byte-offset of that frame/sample. Then nsHTMLMediaElement::UpdateReadyStateForData() will know the byte offset the playback is up to in the file. 

This won't make downloads faster on DailyMotion, that's caused by something else.
Just a note for later - I wish that we had the API that we could expose to developers so they could make these decisions, or automatically switch to a lower-bandwidth version.
(In reply to comment #7)
> This won't make downloads faster on DailyMotion, that's caused by something
> else.

Hhmmm... I timed how long it takes for that video in the bug URL to load to readyState HAVE_ENOUGH_DATA when served from a rate-controlled local webserver. I see no significant difference between FF3.6.3 and locally built mozilla-central opt FF. I'd say we're getting hit by the variableness of internet traffic and caches, not by a perf regression in the new decoder.
Attachment #438635 - Flags: review?(chris.double) → review+
http://hg.mozilla.org/mozilla-central/rev/20cb5fba0089
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: