Closed
Bug 825329
Opened 11 years ago
Closed 10 years ago
Properly get the playbackRate from the AudioClock.
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: padenot, Assigned: padenot)
References
Details
Attachments
(2 files)
6.79 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
Details | Diff | Splinter Review |
[1] is wrong. Setting the playback rate to 0.5 and then to 1.0 does not make the media play at it's intrinsic rate. [1]: http://mxr.mozilla.org/mozilla-central/source/content/media/AudioStream.cpp#1138
Assignee | ||
Comment 1•11 years ago
|
||
This fixes the issue, remove the now useless mPlaybackRate member (not that it was super-useful anyways), and adds a test case. I've also reenabled the test on Linux to check if it was the cause of the super-frequent orange, we will see on try. And there is a bunch of dump()s that I'll have to remove.
Attachment #696407 -
Flags: review?(kinetik)
Updated•11 years ago
|
Attachment #696407 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 2•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f3ef647920 https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d7b5722d4f
Comment 3•11 years ago
|
||
Push backed out for Windows pgo-only mochitest-1 timeouts in media tests, since the backout of just 1abf4c88f8f1 didn't work (see bug 793274 comment 12 for example logs): https://hg.mozilla.org/integration/mozilla-inbound/rev/f2912b7e727a
Assignee | ||
Comment 4•11 years ago
|
||
This changeset was the culprit of the orange when doing a PGO build. This patch is trivial, so I'm a bit sad. I'll try to find something (already tried the #pragma to disable optimizations, it did not work).
Assignee | ||
Comment 5•11 years ago
|
||
So, this fixes the issue, and does not trigger a PGO failure. I tested a bunch of things, and keeping a redundant |double mPlaybackRate| member in the class is the only thing that worked. This patch is functionnaly equivalent. We can revisit if/when we disable PGO.
Attachment #713882 -
Flags: review?(kinetik)
Comment 6•11 years ago
|
||
Do you know where PGO is introducing the problem? I'd like to see some analysis like what bug 768333 has before introducing workarounds. At the very minimum, the patch needs a comment added so that some poor sucker doesn't come along later and remove mPlaybackRate while doing some innocent cleanup work.
Updated•10 years ago
|
Attachment #713882 -
Flags: review?(kinetik)
Assignee | ||
Comment 7•10 years ago
|
||
So, I went and retriggered PGO builds on try with the original patch [1] to have a look at the functions, and it went green this time [2]. I'll be landing the original patch. [1]: https://bugzilla.mozilla.org/attachment.cgi?id=696407 [2]: https://tbpl.mozilla.org/?tree=Try&rev=75e26f8270e0
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c3801aed19
Comment 9•10 years ago
|
||
Sorry, backed out for bug 844129. https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c344441241
Assignee | ||
Comment 12•10 years ago
|
||
Pushed without the test, too much people are complaining about this bug, and I have no time as per now to make the test pass on try. Leaving this open so I can push the test sometime. https://hg.mozilla.org/integration/mozilla-inbound/rev/1bb461d484b2
Whiteboard: [leave-open]
Comment 14•10 years ago
|
||
Would this qualify for uplift to 22 beta? We're still early in the cycle, and it's a pretty glaring bug...
Comment 16•10 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #12) > Pushed without the test, too much people are complaining about this bug, and > I have no time as per now to make the test pass on try. Leaving this open so > I can push the test sometime. Since ongoing work for the test isn't here and it's been a while, can we close this bug and file a follow-up for the test?
Updated•10 years ago
|
Flags: needinfo?(paul)
Assignee | ||
Comment 17•10 years ago
|
||
We can.
Assignee | ||
Comment 18•10 years ago
|
||
We can.
No longer blocks: 883186
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(paul)
Resolution: --- → FIXED
Whiteboard: [leave-open]
Comment 19•10 years ago
|
||
Setting target-milestone to mozilla23 since development of mozilla24 starting on May 13 and this patch got merged to m-c on May 6.
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•