1179110 - twit.tv mp4 video would not autostart

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

Reproduced : always

Steps to reproduce:
1. Open direct link of mp4, http://www.podtrac.com/pts/redirect.mp4/twit.cachefly.net/video/sn/sn0512/sn0512_h264m_864x480_500.mp4
  (or original link https://twit.tv/shows/security-now/episodes/513?autostart=true )
2. Wait for 10-20 sec (depended on network speed), the video should auto start

Actual results:
video would not autostart


Expected results:
The video should auto start after for a while(10-20 sec)
(Although Firefox stutter badly. due to Bug 1178063)


Regression window:
m-c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1fb3ebf68777&tochange=efe86609e776

m-i
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ce11a9ce7600&tochange=a5eb0b1fcf39

Regressed by: Bug 1172264

Comment 1

[Alice0775 White]

Attachment: media logging

set NSPR_LOG_MODULES=timestamp,MediaDecoder:5,MediaSource:5,MediaPromise:5,MP4Demuxer:5,nsMediaElement:5,nsMediaElementEvents:5

wait about 50-60sec, but the video did not start.and then I quit browser.

Comment 2

[Bobby Holley (:bholley)]

Thanks for the awesome bug report Alice!

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Use a Maybe<> to store start time, rather than using -1 as a sentinel. v1

This allows us to properly support videos with a negative start time, which
is happening here.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Fix ComputePlaybackRate. v1

This is a regression from https://hg.mozilla.org/mozilla-central/rev/3ead3466f84a

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8628553 [details] [diff] [review]
Part 1 - Use a Maybe<> to store start time, rather than using -1 as a sentinel. v1

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

Excellent, this had been on my todo list for a while... (bug 1131305)

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8628554 [details] [diff] [review]
Part 2 - Fix ComputePlaybackRate. v1

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

Doh!

Comment 8

[Jean-Yves Avenard [:jya]]

this needs to be uplifted to 41.

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c3eeecda43c

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4aa3b58f6a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/be64b22e2d97

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8628554 [details] [diff] [review]
Part 2 - Fix ComputePlaybackRate. v1

Request applies to both patches in the bug. Part 2 fixes the specific regression in this bug, but Part 1 is necessary to make the testcase run properly on my machine, so I think we should uplift it too.

Approval Request Comment
[Feature/regressing bug #]: bug 1172264
[User impact if declined]: Playback issues
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Low risk.
[String/UUID change made/needed]: None

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e4aa3b58f6a6
https://hg.mozilla.org/mozilla-central/rev/be64b22e2d97

Comment 13

[Alice0775 White]

WOW!
This also fixes Bug 1178063, see Bug 1178063 Comment 8.
Can you uplift this to 40beta and 38ESR?

Comment 14

[Jean-Yves Avenard [:jya]]

I don't believe this will be possible. There's a lot that happened in the background in 41. Uplifting to 40 would be significant effort (and risk). 

The handling of negative start time may be enough though.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> The handling of negative start time may be enough though.

Yes, that part could be backportable, though it would take a bit of work.

Comment 16

[Bobby Holley (:bholley)]

(To be clear, I'm not working on media stuff anymore, so I don't have the cycles to do this backport myself)

Comment 17

[Bobby Holley (:bholley)]

jya, do you have the bandwidth to uplift part 2?

Comment 18

[Jean-Yves Avenard [:jya]]

I can once approved... The bug is only in 41 though...

Comment 19

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #18)
> I can once approved... The bug is only in 41 though...

Yes, but bug 1178063 goes further back, and Alice says this push fixed that - presumably because of the negative start time handling.

Comment 20

[Kate Glazko]

Tracking for 41, 42 because affected. Please keep posted on potential uplift to 41 Aurora.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8628554 [details] [diff] [review]
Part 2 - Fix ComputePlaybackRate. v1

Approving for uplift to Aurora based on my discussion with Sylvestre.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Requesting QE team to verify this fix.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e6fead2c1f3
https://hg.mozilla.org/releases/mozilla-aurora/rev/09219bc6153f

Comment 24

[Eric Rahm [:erahm]]

Considering the lack of traction on bug 1183888 I think we may need to back this out.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Eric Rahm [:erahm] from comment #24)
> Considering the lack of traction on bug 1183888 I think we may need to back
> this out.

That would be hard and pretty silly. r=me on a patch to replace the NS_ENSURE with an |if|.

(Sorry for not doing it myself - I'm not working on media anymore, and am about to head out on PTO).

Comment 26

[Ada [:adalucinet]]

Reproduced with 41.0a2 from 2015-06-30, under Windows 7 64-bit.
Verified fixed with 41.0b2 (Build ID: 20150817163452) and latest Developer Edition 42 (from 2015-08-17), under Windows 7 64-bit and Windows 10 32-bit; confirming that bug 1178063 is fixed too, as Alice also mentioned in comment 13.