dusty-2011 Reporter
	Description • 11 years ago

In Firefox 20, the video playback on tumblr was choppy. And replaying the same video was broken. See bug 868630: https://bugzilla.mozilla.org/show_bug.cgi?id=868630.

In the latest nightly (23.0a1), the video playback on tumblr is no longer choppy. And replaying the same video is no longer broken. However, the audio of the tumblr videos now distorts. This audio problem was not present in Firefox 20, and is also not present in IE9 or Chrome 26.

Here are two sample videos, which clearly have distorted sound:
http://videohall.tumblr.com/post/45234276692/dairy-cows-in-holland-are-let-out-to-pasture-for#notes
http://videohall.tumblr.com/post/44032559382/car-goes-through-ice-ok#notes

The distortion is clearly noticeable in the first video, where the song Beautiful Day by U2 plays. There is a clear cracking sound noticeable throughout the video, a distortion. Also in the second tumblr video, the sound distorts in the latest nightly build. The person's voice is surrounded by cracking noises.

	dusty-2011 Reporter
	Comment 1 • 11 years ago

I have tested whether this sound distortion problem is present on other sites as well in the latest nightly build. I have tested these videos: 
https://www.youtube.com/watch?v=qqIIW7nxBgc
http://www.zie.nl/video/algemeen/Fors-meer-geevacueerden-Wetteren-dan-gedacht/m1nzerif0vja
http://vimeo.com/65576562

There were no audio distortion problems in the above videos. The audio distortion problems seems to be isolated to the tumblr video player.

	Paul Silaghi, QA [:pauly]
	Comment 2 • 11 years ago

Confirmed.
The video is choppy in FF 20.0.1.
The video is no longer choppy in FF 21 - 23, but the audio is distorted.

	dusty-2011 Reporter
	Comment 3 • 11 years ago

Firefox 21.0 was released today. Firefox 21.0 suffers from the distorted sound on the tumblr video player. Again, these are two samples: 
http://videohall.tumblr.com/post/45234276692/dairy-cows-in-holland-are-let-out-to-pasture-for#notes
http://videohall.tumblr.com/post/44032559382/car-goes-through-ice-ok#notes

It's a shame this bug is now present in the stable release version of Firefox.

Paul Silaghi, what are the next steps? We just wait forever until someone assigns himself to work on this bug?

	cajbir (:cajbir)
	Comment 4 • 11 years ago

Based on comment 2 this is a regression. Can we get a regression range?

	Alice0775 White
	Comment 5 • 11 years ago

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/8f4ec9046b03
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130205 Firefox/21.0 ID:20130205021221
Bad:
http://hg.mozilla.org/mozilla-central/rev/1eab3e0d7f53
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130205 Firefox/21.0 ID:20130205072423
Pushlog;
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8f4ec9046b03&tochange=1eab3e0d7f53


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b894118e1c77
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130204 Firefox/21.0 ID:20130204132322
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d4ce9051566e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130204 Firefox/21.0 ID:20130204134321
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b894118e1c77&tochange=d4ce9051566e

Triggered by:
d4ce9051566e	Chris Pearce — Bug 837859 - Enable Windows Media Foundation support for H.264,AAC and MP3 playback on Windows 7 and later. r=roc


The page uses the following video source:
[Prior to landing Bug 837859]
<object type="application/x-shockwave-flash" data="/javascript/video-js/video-js.swf" id="tumblr_video_player_44032559382_flash_api" name="tumblr_video_player_44032559382_flash_api" class="vjs-tech" width="100%" height="100%"><param name="movie" value="/javascript/video-js/video-js.swf"><param name="flashvars" value="readyFunction=_V_.flash.onReady&amp;eventProxyFunction=_V_.flash.onEvent&amp;errorEventProxyFunction=_V_.flash.onError&amp;autoplay=false&amp;preload=none&amp;loop=false&amp;muted=false&amp;src=http%3A%2F%2Fvideohall.tumblr.com%2Fvideo_file%2F44032559382%2Ftumblr_mit3i0lSV71r6ego3&amp;"><param name="allowScriptAccess" value="always"><param name="allowNetworking" value="all"><param name="wmode" value="opaque"><param name="bgcolor" value="#000000"></object>

[After landing Bug 837859]
<video tabindex="0" src="http://videohall.tumblr.com/video_file/44032559382/tumblr_mit3i0lSV71r6ego3" preload="none" id="tumblr_video_player_44032559382_html5_api" class="vjs-tech">
</video>

So I cannot say this is Firefox's fault.
Probably, I think that the html5 video source has a problem.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 6 • 11 years ago

Thank you Alice!

I downloaded the video at http://videohall.tumblr.com/video_file/44032559382/tumblr_mit3i0lSV71r6ego3 and it plays fine in Windows Media Player, so we must be doing something wrong here in our H.264/AAC backend.

	Alice0775 White
	Comment 7 • 11 years ago

(In reply to Alice0775 White from comment #5)
> So I cannot say this is Firefox's fault.
> Probably, I think that the html5 video source has a problem.
This is wrong.

Steps To Reproduce:
1. Open http://videohall.tumblr.com/video_file/44032559382/tumblr_mit3i0lSV71r6ego3

Actual Results:
 *Sound has a problem in Firefox21-24
 *Works ok in Opera12.15 and IE9 (QuickTime plugin both)

So, this is definitely Firefox 'Windows Media Foundation' problem

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 8 • 11 years ago

The problem with these videos is that their audio tracks are timestamped wrong.

Each audio IMFSample reports its duration as approximately 2 more samples than it actually has, and this results in us writing silence to the AudioStream to make up the missing samples, which is the crackling we hear. This sample underrun seems to  also be reflected in the timestamp, which is cumulatively off by about 2 more samples for every IMFSample. The videos are mono, 8bit PCM, and encoded by Lavf54.59.106, which presumably must be buggy.

I guess we should calculate the audio samples' timestamps ourselves based on the sum of the samples, and reset the count when we encounter a sample with its MFSampleExtension_Discontinuity attribute set.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 9 • 11 years ago

Calculate audio timestamps/duration of WMF audio samples based on the number of samples decoded, rather than trusting WMF's timestamps.

	Paul Adenot (:padenot)
	Comment 10 • 11 years ago

Would a test be easy enough to write ?

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5a9cd5032d

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 12 • 11 years ago

(In reply to Paul Adenot (:padenot) from comment #10)
> Would a test be easy enough to write ?

hmmm... We'd need to create a buggy file and then capture the decoded output and check that we're not injecting silence into it to cover the gaps...

	Paul Adenot (:padenot)
	Comment 13 • 11 years ago

Maybe using Web Audio and |decodeAudioData| (since that uses WMF)+ AnalyzerNode. Maybe it's not worth the trouble.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 11 years ago

Backed out for Windows mochitest-1 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db7bc3a2631

https://tbpl.mozilla.org/php/getParsedLog.php?id=23255518&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=23255534&tree=Mozilla-Inbound

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 11 years ago

And assertions.
https://tbpl.mozilla.org/php/getParsedLog.php?id=23255623&full=1&branch=mozilla-inbound#error0

04:22:35     INFO -  [Parent 3568] ###!!! ASSERTION: Seek target should lie inside the first audio block after seek: '!audio || (audio->mTime <= seekTime && seekTime <= audio->mTime + audio->mDuration)', file e:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/content/media/MediaDecoderStateMachine.cpp, line 1959

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 16 • 11 years ago

So the problem here is that when we seek(0), WMF doesn't mark the stream as a discontinuity, so we don't re-capture the audio frame counts after a seek(0), and thus the timestamps are double what they're supposed to be on the second play through, hence the failure in test_play_twice. We need to re-capture the audio frame counts after a seek.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 17 • 11 years ago

Reset the audio sample/sum after a seek. This is based on top of the previous patch. I'll merge this into the previous patch before landing, but I'm putting it up separately for easier review.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 18 • 11 years ago

Landed rolled-up patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a569c522b1

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/77a569c522b1

	dusty-2011 Reporter
	Comment 20 • 11 years ago

This is fixed in the current Nightly, thanks!! The sample videos in my original post playback smoothly and with good sound.

Any chance this can get uplifted to Firefox 22 and 23? I would hate having to wait for Firefox 24 to release; and having to use a different browser for tumblr in the meantime.

	Scoobidiver (away)
	Comment 21 • 11 years ago

(In reply to dusty-2011 from comment #20)
> Any chance this can get uplifted to Firefox 22 and 23?
It depends on the benefit vs. risk ratio.

	dusty-2011 Reporter
	Comment 22 • 11 years ago

(In reply to Scoobidiver from comment #21)
> (In reply to dusty-2011 from comment #20)
> > Any chance this can get uplifted to Firefox 22 and 23?
> It depends on the benefit vs. risk ratio.

In that case, I would like to comment on the benefits: there are thousands of tumblr blogs which use the same type of videos (with wrong audio timestamps), all of these videos (probably millions of them) have distorted sound in Firefox 21.

In fact, I think it is the standard video player of tumblr which exihibits this behavior (timestamping the videos wrong), so all videos that bloggers upload for use on this standard video player play with distorted sound in Firefox 21.

So there are clear benefits for uplifting this to Firefox 23 or 22 for the millions of tumblr users and bloggers.

I'm not an expert on the risks of uplifting this patch to aurora or beta, you would have to ask someone else about this. I just wanted to point out that there are very significant benefits of uplifting to aurora or beta.

	Paul Silaghi, QA [:pauly]
	Comment 23 • 11 years ago

Verified fixed nightly 24.0a1 (2013-06-02) win 7 x64

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 24 • 11 years ago

Rolled up patch, that I landed on m-i.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 25 • 11 years ago

Comment on attachment 757674 [details] [diff] [review]
Patch: Calculate audio timestamps based on samples written

[Approval Request Comment]
Bug caused by (feature/regressing bug #): H.264/AAC video support on Windows
User impact if declined: "Lots" of videos on tumblr won't play correctly, they'll have crackles in their audio output.
Testing completed (on m-c, etc.): This has been on Nightly for a few days.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/82a41a9e888a

	Paul Silaghi, QA [:pauly]
	Comment 27 • 11 years ago

Verified fixed FF 23.0a2 (2013-06-09) Win 7 x64