Last Comment Bug 825329 - Properly get the playbackRate from the AudioClock.
: Properly get the playbackRate from the AudioClock.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: Paul Adenot (:padenot)
:
Mentors:
: 858997 866911 874618 887262 901727 (view as bug list)
Depends on: 828901
Blocks: 840745 844129
  Show dependency treegraph
 
Reported: 2012-12-28 14:01 PST by Paul Adenot (:padenot)
Modified: 2013-08-05 15:28 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix a stupid error in playbackRate. r= (6.79 KB, patch)
2012-12-28 14:03 PST, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Splinter Review
Fix a stupid error in playbackRate. r= (1.33 KB, patch)
2013-02-14 05:52 PST, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review

Description Paul Adenot (:padenot) 2012-12-28 14:01:21 PST
[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
Comment 1 Paul Adenot (:padenot) 2012-12-28 14:03:21 PST
Created attachment 696407 [details] [diff] [review]
Fix a stupid error in playbackRate. r=

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.
Comment 3 Ed Morley [:emorley] 2013-01-16 07:20:37 PST
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
Comment 4 Paul Adenot (:padenot) 2013-01-24 11:32:47 PST
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).
Comment 5 Paul Adenot (:padenot) 2013-02-14 05:52:07 PST
Created attachment 713882 [details] [diff] [review]
Fix a stupid error in playbackRate. r=

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.
Comment 6 Matthew Gregan [:kinetik] 2013-02-14 14:53:38 PST
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.
Comment 7 Paul Adenot (:padenot) 2013-02-22 01:46:40 PST
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
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-02-22 08:46:43 PST
Sorry, backed out for bug 844129.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c344441241
Comment 10 ivan 2013-04-08 21:23:45 PDT
*** Bug 858997 has been marked as a duplicate of this bug. ***
Comment 11 Paul Adenot (:padenot) 2013-05-06 02:36:13 PDT
*** Bug 866911 has been marked as a duplicate of this bug. ***
Comment 12 Paul Adenot (:padenot) 2013-05-06 04:37:46 PDT
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
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-05-06 11:35:20 PDT
https://hg.mozilla.org/mozilla-central/rev/1bb461d484b2
Comment 14 mjh563 2013-05-25 10:41:50 PDT
Would this qualify for uplift to 22 beta? We're still early in the cycle, and it's a pretty glaring bug...
Comment 15 Maria Grazia Alastra [:mga] 2013-05-27 04:56:34 PDT
*** Bug 874618 has been marked as a duplicate of this bug. ***
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2013-06-14 07:31:05 PDT
(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?
Comment 17 Paul Adenot (:padenot) 2013-06-14 09:06:16 PDT
We can.
Comment 18 Paul Adenot (:padenot) 2013-06-14 09:06:23 PDT
We can.
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2013-06-14 09:22:54 PDT
Setting target-milestone to mozilla23 since development of mozilla24 starting on May 13 and this patch got merged to m-c on May 6.
Comment 20 jcampbell1 2013-06-26 17:55:16 PDT
*** Bug 887262 has been marked as a duplicate of this bug. ***
Comment 21 mjh563 2013-08-05 15:28:59 PDT
*** Bug 901727 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.