Hang on https://www.mozilla.org/en-US/contribute/ when playing video

VERIFIED FIXED in Firefox 20

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: mayankleoboy1, Assigned: padenot)

Tracking

({hang, regression})

20 Branch
mozilla21
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox20+ verified, firefox21 verified)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Posted file about support.txt
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121207 Firefox/20.0
Build ID: 20121207030741

Steps to reproduce:

1. Create a new profile
2. Go to https://www.mozilla.org/en-US/contribute/
3. Click on the video to start playing.


Actual results:

Firefox hangs. Well not exactly hangs, but I cant do any UI operations like scroll, right click , open/close tab and others.
The firefox.exe uses 100% CPU usage.


Expected results:

No hang, duh.
*************************
Can repro with HWA on and off.
(Reporter)

Updated

7 years ago
Summary: Hang on https://www.mozilla.org/en-US/contribute/ → Hang on https://www.mozilla.org/en-US/contribute/ when playing video
Last good nightly: 2012-11-22
First bad nightly: 2012-11-23

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ec9014f220&tochange=d8e4f06198dc

=> Trunk only Regression.
=> no sure about Candidates as Regressor (there's Bug 814292 e.g.)
Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
Product: Firefox → Core

Comment 3

7 years ago
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd00a3bad021
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121122020052
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a20b32f37e7a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121122023852
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c6af8da7bfa3&tochange=a20b32f37e7a

Suspected:  Bug 495040
Blocks: 495040
Severity: normal → critical
Component: General → Video/Audio

Updated

7 years ago
OS: Windows 7 → All

Comment 4

6 years ago
This seems like an important regression!
Assignee: nobody → paul
If we can't get this resolved in the next couple of weeks, we should really consider backing out bug 495040 before 20 even hits Aurora.
(Assignee)

Comment 6

6 years ago
I've got a patch for this. I want to dig a little deeper before uploading it, because I have the feeling it is patching the symptoms and not the cause.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 819828
(Assignee)

Comment 8

6 years ago
This patch patches the root cause for bug 822952. The thing is, by offseting the
clock two times using the |mStartTime|, |mRemainingTime| was repeatidly
negative, then clamped to zero, causing tons of messages dispatched to the main
thread as you noted.

The other part is setting the media as playing in the state machine if it is not
marked as such, but the MediaDecoder says it is, so we can take query the audio
clock sooner and avoid having fake values.
Attachment #694325 - Flags: review?(kinetik)
Attachment #694325 - Flags: review?(kinetik) → review+
Comment on attachment 694325 [details] [diff] [review]
Don't offset the clock by the start time of the video two times, and set the media as playing earlier. r=

Review of attachment 694325 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch Paul.

::: content/media/MediaDecoderStateMachine.cpp
@@ +2341,5 @@
>      }
>      // Current frame has already been presented, wait until it's time to
>      // present the next frame.
>      if (frame && !currentFrame) {
>        int64_t now = IsPlaying() ? clock_time : mPlayDuration;

Correct me if I'm wrong, but I think this line needs to be:

    int64_t now = IsPlaying() ? clock_time : (mPlayDuration + mStartTime);

Otherwise in the !IsPlaying() case we'll be subtracting a value which doesn't include mStartTime (|now|, which has value mPlayDuration), from a value doesn't (frame->mTime) and so meaning remainingTime will be incorrectly large and we'll have the opposite problem and end up sleeping the state machine thread for too long.

This will only matter in the case where the media has a non-zero start time, but they do exist; live streams for example.
(In reply to Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 694325 [details] [diff] [review]
> [...]
> Otherwise in the !IsPlaying() case we'll be subtracting a value which
> doesn't include mStartTime (|now|, which has value mPlayDuration), from a
> value doesn't (frame->mTime)

Oops, typo, that should have read:

Otherwise in the !IsPlaying() case we'll be subtracting a value which
doesn't include mStartTime (|now|, which has value mPlayDuration), from a
value *which does* (frame->mTime)


(i.e. VideoData::mTime timestamps include mStartTime, but |now| doesn't in the !IsPlaying() case)
(Assignee)

Comment 11

6 years ago
Chris, you were right, this changeset includes the modification you propose.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5add564d4546
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 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7f8d86e592

I bisected the queue on try, this patch is safe.
https://hg.mozilla.org/mozilla-central/rev/904caf7811e4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Comment 15

6 years ago
Comment on attachment 694325 [details] [diff] [review]
Don't offset the clock by the start time of the video two times, and set the media as playing earlier. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 495040
User impact if declined: Freeze of the browser when playing an html5 video, the user needs to force-close it.
Testing completed (on m-c, etc.): Baked for a long time on central.
Risk to taking this patch (and alternatives if risky): Little risk, very high CPU consumption when playing video if not taken.
String or UUID changes made by this patch: none.

(requesting approval in reaction to a release-mgmt email).
Attachment #694325 - Flags: approval-mozilla-aurora?
Comment on attachment 694325 [details] [diff] [review]
Don't offset the clock by the start time of the video two times, and set the media as playing earlier. r=

Approving on aurora considering it is Fx20 regression and has had good bale-time on m-c.

Adding qawanted,verifyme to help with QA verification.
Attachment #694325 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: qawanted, verifyme
QA Contact: jbecerra
(In reply to Paul Adenot (:padenot) from comment #15)
> Comment on attachment 694325 [details] [diff] [review]
> Don't offset the clock by the start time of the video two times, and set the
> media as playing earlier. r=
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 495040
> User impact if declined: Freeze of the browser when playing an html5 video,
> the user needs to force-close it.
> Testing completed (on m-c, etc.): Baked for a long time on central.
> Risk to taking this patch (and alternatives if risky): Little risk, very
> high CPU consumption when playing video if not taken.
> String or UUID changes made by this patch: none.
> 
> (requesting approval in reaction to a release-mgmt email).

Paul, please keep a look out for any regression's from this uplift..In case we find regressions due to this & are not resolved before Fx20 hits beta we should consider backing out Bug 495040.
I backed this out for Windows bustage: https://hg.mozilla.org/releases/mozilla-aurora/rev/de299ce2b2c1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Verified with Firefox 20 beta 1 on: Mac OSX 10.7.5, Ubuntu 12.04 32-bit, Windows 7 32-bit.

Build ID: 20130220104816
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0

On https://www.mozilla.org/en-US/contribute/ there is no video now so I tested the video from http://careers.mozilla.org/en-US/ and the issue is not reproducible so it should be verified fixed. If the video is not similar with the one given in comment 0 then this is not verified as fixed.
Flags: needinfo?
Tested on Firefox 21 beta 3 (buildID: 20130416200523).
Flags: needinfo?

Comment 24

6 years ago
The following url is reproducible the problem in the bad build of comment#2 and comment#3

http://www.mozilla.org/en-US/firefox/23.0a1/firstrun/
Flags: needinfo?
(In reply to Alice0775 White from comment #24)
> The following url is reproducible the problem in the bad build of comment#2
> and comment#3
> 
> http://www.mozilla.org/en-US/firefox/23.0a1/firstrun/

Verified as fixed on Firefox 21 beta 3 (buildID: 20130416200523) using this url.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.