Closed
Bug 1020538
Opened 7 years ago
Closed 7 years ago
Fix and re-enable test_playback_rate.html
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: RyanVM, Assigned: jwwang)
References
Details
Attachments
(4 files)
4.71 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
9.41 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
20.95 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
5.21 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
With bug 921622 fixed, I took a look at re-enabling test_playback_rate.html. It looks pretty green on opt builds, but debug builds still assert like crazy. Android 4.0 Panda try debug test mochitest-3 on 2014-06-04 11:26:20 PDT for push 2fb25f5fc810 06-04 11:52:12.125 I/Gecko ( 2250): [2250] ###!!! ASSERTION: Should be on state machine or decode thread.: 'OnStateMachineThread() || OnDecodeThread()', file /builds/slave/try-and-d-00000000000000000000/build/content/media/MediaDecoderStateMachine.cpp, line 559 06-04 11:52:16.203 I/Gecko ( 2250): [2250] ###!!! ASSERTION: Clock should go forwards if the playback rate is > 0.: 'mCurrentFrameTime <= clock_time || mPlaybackRate <= 0', file /builds/slave/try-and-d-00000000000000000000/build/content/media/MediaDecoderStateMachine.cpp, line 2379 06-04 11:52:26.851 E/GeckoConsole( 2250): [JavaScript Warning: "Invalid URI. Load of media resource failed." {file: "http://mochi.test:8888/tests/content/media/test/test_playback_rate.html" line: 0}] I'm feeling inclined to re-enable test_playback_rate.html on opt builds once I get some more green retriggers, but debug builds are still going to need some help.
Reporter | ||
Comment 1•7 years ago
|
||
Windows 7/8 opt builds still hit bug 814533 as well.
Blocks: 814533
Summary: test_playback_rate.html asserts when re-enabled → Fix and re-enable test_playback_rate.html
Reporter | ||
Comment 2•7 years ago
|
||
The "Should be on state machine or decode thread" asserts happen on all platforms. Linux also hits the "Clock should go forwards if the playback rate is > 0" assert. However, the test was already annotated on all platforms for assertions before, so maybe we just need to bump the allowed number (assuming it's the same asserts as before, just firing more often).
Reporter | ||
Comment 3•7 years ago
|
||
Occasional Android timeouts, but hard to say it's any worse than any other test in content/media in that respect... https://tbpl.mozilla.org/php/getParsedLog.php?id=41054576&tree=Try
Reporter | ||
Comment 4•7 years ago
|
||
Pretty flaky on B2G emulator with the following happening frequently: 1453 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback_rate.html | Current time should not change when playbackRate is null (5.207732 4.88848). 1468 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback_rate.html | Current time should not change when playbackRate is null (3.233 2.8). 1522 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback_rate.html | The element should not be in paused state. https://tbpl.mozilla.org/php/getParsedLog.php?id=41056722&tree=Try
Reporter | ||
Comment 5•7 years ago
|
||
Ouch, Android debug hit anywhere from 3-94 assertions. That's too wide for me to comfortably annotate it away.
Reporter | ||
Comment 6•7 years ago
|
||
I filed bug 1020707 for the Android timeouts. They're infrequent enough that they'd be tolerable IMO (given Android's already crappy orange rate for media tests).
Reporter | ||
Comment 7•7 years ago
|
||
Definitely not intended to be the final fix for this bug, but it gets the ball rolling anyway. This patch re-enables test_playback_rate.html on Linux, OSX, and Android opt. It also updates the assertion annotations for all desktop platforms. Due to the aforementioned issues in this bug, Windows, B2G, and Android debug remain disabled. Try runs: https://tbpl.mozilla.org/?tree=Try&rev=588f84523a36 https://tbpl.mozilla.org/?tree=Try&rev=f9ae9b880cb0
Attachment #8434609 -
Flags: review?(cpearce)
Updated•7 years ago
|
Attachment #8434609 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 8•7 years ago
|
||
I got this assertion when running on my local Linux box. #0 0x00007ffff2169597 in mozilla::MediaDecoderStateMachine::NeedToDecodeAudio (this=0x7fffbfc97840) at /home/jwwang/codebase/mozilla-central2/content/media/MediaDecoderStateMachine.cpp:685 #1 0x00007ffff21697ba in mozilla::MediaDecoderStateMachine::DispatchDecodeTasksIfNeeded (this=this@entry=0x7fffbfc97840) at /home/jwwang/codebase/mozilla-central2/content/media/MediaDecoderStateMachine.cpp:1580 #2 0x00007ffff2169b82 in mozilla::MediaDecoderStateMachine::StopPlayback (this=this@entry=0x7fffbfc97840) at /home/jwwang/codebase/mozilla-central2/content/media/MediaDecoderStateMachine.cpp:1107 #3 0x00007ffff2169c8d in mozilla::MediaDecoderStateMachine::StartBuffering (this=0x7fffbfc97840) at /home/jwwang/codebase/mozilla-central2/content/media/MediaDecoderStateMachine.cpp:2684 #4 0x00007ffff2162bbf in mozilla::MediaDecoder::Resume (this=0x7fffc452d4e0, aForceBuffering=<optimized out>) at /home/jwwang/codebase/mozilla-central2/content/media/MediaDecoder.cpp:1373 #5 0x00007ffff20c6974 in mozilla::dom::HTMLMediaElement::Play (this=0x7fffbfeb5400, aRv=...) at /home/jwwang/codebase/mozilla-central2/content/html/content/src/HTMLMediaElement.cpp:2124 #6 0x00007ffff1840412 in mozilla::dom::HTMLMediaElementBinding::play (cx=0x7fffcd0176a0, obj=..., self=<optimized out>, args=...) at /home/jwwang/codebase/mozilla-central2/obj-x86_64-unknown-linux-gnu/dom/bindings/HTMLMediaElementBinding.cpp:930 I don't understand why MediaDecoderStateMachine::NeedToDecodeVideo|NeedToDecodeAudio should only be called on state or decode threads.
Reporter | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e8837e166a
Assignee: nobody → ryanvm
Flags: in-testsuite+
Whiteboard: [leave open]
Reporter | ||
Updated•7 years ago
|
Assignee: ryanvm → nobody
Comment 10•7 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #8) > I don't understand why > MediaDecoderStateMachine::NeedToDecodeVideo|NeedToDecodeAudio should only be > called on state or decode threads. Given that the decoder monitor should be held here, I agree, these functions should be safe to call on any thread.
Reporter | ||
Comment 12•7 years ago
|
||
FYI, we see the "Clock should go forwards if the playback rate is > 0" asserts quite a bit in test_playback_rate_playpause.html.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jwwang
Assignee | ||
Comment 13•7 years ago
|
||
I've fixed all failures on all platforms except the B2G emualtor. https://tbpl.mozilla.org/php/getParsedLog.php?id=42030206&tree=Try&full=1#error2 TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback_rate.html | Current time should not change when playbackRate is null (2.681958 2.058891). MediaDecoder::PlaybackPositionChanged() updates the official playback position (mCurrentTime) and dispatches 'timeupdate' event asynchronously. It is possible for the current playback position to change again before 'timeupdate' is fired. It is observed on B2G emulator that the official playback position is 2.058891 and current playback position is 2.667270 when 'timeupdate' is fired. The check fails at http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/test/test_playback_rate.html#l96 for the difference is over 0.3 sec. It is also observed on B2G emulator that the latency between 'timeupdate' is dispatched and fired could go as high as 2.46867 sec for B2G emulator sometimes is very slow. I think we could either have a higher tolerance on B2G or update the official playback position again before 'timeupdate' is fired to minimize the difference between current/official playback position.
Flags: needinfo?(cpearce)
Comment 14•7 years ago
|
||
I'd suggest we increase the latency tolerance on B2G. Timeupdate isn't supposed to be highly accurate. 'timeupdate' is defined to fire loosely, approx 4 times per second during playback. MediaDecoder::PlaybackPositionChanged() passes "true" to MediaDecoderOwner::FireTimeUpdate to signal a "periodic" time update, i.e. to only fire a 'timeupdate' if one hasn't fired in the last ~250ms. We could be hitting that, and then our task to dispatch the 'timeupdate' event to HTML could be getting delayed in the event queues.
Flags: needinfo?(cpearce)
Assignee | ||
Comment 15•7 years ago
|
||
Fix the position calculation algorithm in MediaDecoderStateMachine::GetVideoStreamPosition().
Attachment #8444917 -
Flags: review?(cpearce)
Assignee | ||
Comment 16•7 years ago
|
||
Fix frame position adjustment algorithm in AudioStream.
Assignee | ||
Comment 17•7 years ago
|
||
Enable test_playback_rate.html.
Attachment #8444919 -
Flags: review?(cpearce)
Assignee | ||
Comment 18•7 years ago
|
||
Comment on attachment 8444918 [details] [diff] [review] 1020538_fix_test_playback_rate_part2-v1.patch Review of attachment 8444918 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioStream.cpp @@ +979,5 @@ > } > > uint8_t* wpos = reinterpret_cast<uint8_t*>(aBuffer); > double playbackRate = static_cast<double>(mInRate) / mOutRate; > + uint32_t toPopBytes = FramesToBytes(ceil(aFrames * playbackRate)); When playback rate is 0.5, libSoundTouch will turn 100 frames into 200 frames. Therefore we should only pop 200 * 0.5 = 100 frames. However it doesn't really matter for the while loop below will ensure we've process enough frames.
Attachment #8444918 -
Flags: review?(kinetik)
Comment 19•7 years ago
|
||
Comment on attachment 8444918 [details] [diff] [review] 1020538_fix_test_playback_rate_part2-v1.patch Review of attachment 8444918 [details] [diff] [review]: ----------------------------------------------------------------- This is much nicer than the existing code.
Attachment #8444918 -
Flags: review?(kinetik) → review+
Comment 20•7 years ago
|
||
Note that the following assertion count needs to be corrected before these patches can land: 22:46:51 INFO - 5130 ERROR TEST-UNEXPECTED-PASS | /tests/content/media/test/test_playback_rate_playpause.html | Assertion count 0 is less than expected range 2-7 assertions.
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Matthew Gregan [:kinetik] from comment #20) > Note that the following assertion count needs to be corrected before these > patches can land: > > 22:46:51 INFO - 5130 ERROR TEST-UNEXPECTED-PASS | > /tests/content/media/test/test_playback_rate_playpause.html | Assertion > count 0 is less than expected range 2-7 assertions. They will be removed in bug 897108.
Comment 22•7 years ago
|
||
Okay, then this can't land until after bug 897108.
Updated•7 years ago
|
Attachment #8444917 -
Flags: review?(cpearce) → review+
Updated•7 years ago
|
Attachment #8444919 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 23•7 years ago
|
||
Bug 1020538 and bug 897108 should be landed at the same time. If we land bug 1020538 first, we will hit comment 20. If we land bug 897108 first, test_playback_rate_playpause.html will fail from assertions count > 0. Is there any way to tell Sheriff to land these patches altogether?
Comment 24•7 years ago
|
||
Oh, the dependency is circular? In that case, it's better to post all the patches in a single bug (or perhaps even roll the fixes together, otherwise bisect becomes painful). There's probably some way to signal this in the whiteboard for needs-landing too, but I don't know what it is. I'll land them now, anyway.
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a896fedd7cec https://hg.mozilla.org/integration/mozilla-inbound/rev/3d78475581e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/614bfb13a731
Status: NEW → ASSIGNED
Whiteboard: [leave open]
Assignee | ||
Comment 26•7 years ago
|
||
Thanks, Matthew. 1020538_fix_test_playback_rate_enable_test-v1.patch and 897108_fix_test_playback_rate_playpause_html_timeout-v1.patch were originally one patch which was split for easier review. Next time I should take both review and check-in into account when merge/split patches.
Comment 27•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a896fedd7cec https://hg.mozilla.org/mozilla-central/rev/3d78475581e2 https://hg.mozilla.org/mozilla-central/rev/614bfb13a731
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reporter | ||
Comment 28•7 years ago
|
||
Not sure what the backport situation is here. Please request approval on whatever branches you feel this is safe for.
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → fixed
status-firefox-esr24:
--- → wontfix
Assignee | ||
Comment 29•7 years ago
|
||
The patches can't apply cleanly. The code changed quite a bit even since then. Since this bug fixes the problem in playback rate changes which most user will not encounter, I guess we are fine with not uplifting to aurora/beta.
Reporter | ||
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•