825329 - Properly get the playbackRate from the AudioClock.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

[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)]

Attachment: 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 2

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f3ef647920
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d7b5722d4f

Comment 3

[Ed Morley [:emorley]]

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)]

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)]

Attachment: 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]]

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)]

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 8

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78c3801aed19

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Sorry, backed out for bug 844129.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c344441241

Comment 12

[Paul Adenot (:padenot)]

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]]

https://hg.mozilla.org/mozilla-central/rev/1bb461d484b2

Comment 14

[mjh563]

Would this qualify for uplift to 22 beta? We're still early in the cycle, and it's a pretty glaring bug...

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(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)]

We can.

Comment 18

[Paul Adenot (:padenot)]

We can.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Setting target-milestone to mozilla23 since development of mozilla24 starting on May 13 and this patch got merged to m-c on May 6.