506434 - video playback regression

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rich Walsh]

Attachment: fix video playback regression

Bug 496147 attempted to fix cases where ogg videos with no audio data in the final frames failed to terminate.  It assumed that if the audio position reported by the backend fails to change from one iteration of a timing loop to the next, then there must not be any audio.

This assumption is only valid on platforms where audio position is reported in real-time.  On OS/2, Mac, and possibly Linux/Pulseaudio, it is only updated when the platform's audio system requests more input.  If the timing loop runs faster than these updates occur, the loop will exit prematurely.  On OS/2, at least, the result is stuttering video that soon freezes.

The primary patch achieves the same result as the original by checking whether the frame actually has any audio.  If not, it uses the existing code's no-audio timing mechanism.  The OS/2-only patch eliminates a spurious error that may occur when the final frames have no audio.

Comment 1

[Rich Walsh]

Attachment: OS/2-only - fix spurious error

Comment 2

[Peter Weilbacher]

Comment on attachment 390628 [details] [diff] [review]
OS/2-only - fix spurious error

Yes, this looks OK. To get it checked in, I think you have to add it to sydney_os2_base.patch, too.

Comment 3

[cajbir (:cajbir)]

Yes, you'll need to reference this bug in README_MOZILLA and either add it to the existing sydney_os2_base.patch or create a new one and update update.sh with it.

I'll review attachment 390627 [details] [diff] [review] tomorrow.

Comment 4

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

I wonder if this needs to take into account any pending audio data in the nsAudioStream as well? Shouldn't the hardware audio clock still advance if there's pending audio data in the stream (in nsAudioStream::mBufferOverflow) but no audio data in the frame we're trying to play?

Comment 5

[Rich Walsh]

(In reply to comment #4)
> Shouldn't the hardware audio clock still advance if there's pending
> audio data in the stream (in nsAudioStream::mBufferOverflow) but no
> audio data in the frame we're trying to play?

Very true, but how do you ascertain that the clock is advancing in a reliable, cross-platform way?  If this timing loop tests the audio position every, say, 20ms. but some of the backends only update the reading every 35ms, then the result of the test will be invalid much of the time.

In any case, remember that the original bug addressed the issue of videos that failed to terminate due to no audio in the final frames.  Like the earlier solution, this one resolves the issue but also provides consistent results across all platforms.

Comment 6

[Rich Walsh]

Attachment: update sydney_os2_base.patch

This applies the OS/2-only patch to media/libsydneyaudio/sydney_os2_base.patch

Comment 7

[Peter Weilbacher]

Comment on attachment 392123 [details] [diff] [review]
update sydney_os2_base.patch

Yes, thanks. This is the same as the OS/2 fix. I'll push them together.

Comment 8

[Peter Weilbacher]

All three parts pushed to trunk:

http://hg.mozilla.org/mozilla-central/rev/59d119675e49
http://hg.mozilla.org/mozilla-central/rev/c727737038c1
http://hg.mozilla.org/mozilla-central/rev/a6901991dbce

Comment 9

[Peter Weilbacher]

I guess this should land on 1.9.1, too. Does it depend on bug 496147 which has not landed there yet? Pushing only the OS/2 change doesn't make much sense, I guess.

Comment 10

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

Your patch removes all code added by the patch in bug 496147 and then re-implements it, so it's technically dependent. I'd resisted pushing 496147 to 1.9.1 because I knew it didn't fix the issue properly. I suggest that if we want this fix on 1.9.1, we should also take 496147 so that the changesets are the same on both branches.

Comment 11

[Rich Walsh]

As an alternative to applying this patch to v1.9.1, please see the consolidated
patch attached to Bug 523770.