Closed Bug 1271847 Opened 8 years ago Closed 8 years ago

Vine plays one frame and then stops, in Nightly

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 + fixed
firefox49 + verified

People

(Reporter: dholbert, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:
 1. Visit https://vine.co/v/eYKTOzJnub0

ACTUAL RESULTS:
Video plays 1 frame (i.e. the bison moves once, a teensy bit) and then it stops.

EXPECTED RESULTS:
Video should play.


This worked until very recently. Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=099b513a04274332d18fd8dad11ce1a0aba1b176&tochange=61091afacd2312e8d7500722414689c70ee4bd1f
Flags: needinfo?(jyavenard)
There are two commits in that range:
> Bug 1269325: [mp4] Recalculate dts after adjusting cts. r=kentuckyfriedtakahe,r=gerald
...and:
> Backed out changeset d14dc334734adccc6741093c388e13c7099a8a38 (Bug 1268868: [MSE] P1. Re-enable gap detection within a media segment. r=gerald)

I'm not sure which of those two commits is responsible for this bug.  Hoping jya can figure that out. :)

I initially thought that maybe this bug was just a dupe of bug 1268868 (re-exposed via that backout), but I don't think it is -- at least, I get EXPECTED RESULTS (no bug) in a Nightly build from a few days before that bug's fix landed, launched via: mozregression --launch=2016-04-28 -a "https://vine.co/v/eYKTOzJnub0"

So AFAICT this is a *new* regression -- not a recurrence of a bug which has been knowingly reintroduced via a backout.
[Tracking Requested - why for this release]: Recent regression, breaking video playback on a popular video site (Vine).

Also: not sure if this is platform-specific, but if it matters, I'm on Ubuntu 16.04.
I can also repro this bug on Mac Nightly.
[Tracking Requested - why for this release]:

I expect this bug might soon affect Aurora48, since Comment 1's first possibly-guilty commit (bug 1269325) was backported to Aurora48 yesterday, and the other possibly-guilty commit is a backout of a cset that never made it to Aurora48 in the first place.
Tentatively assuming this is a backout from bug 1269325, based on the regression range in comment 1.
Blocks: 1269325
I'll be damned!
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
This web site is just horrible. The use of the MSE API is sub-par. It will append data over and over without stop.

The media segments they add is not compliant with ISO/IEC 14496-12.
Also the samples it contains have negative start time.

Per
https://w3c.github.io/media-source/index.html#sourcebuffer-coded-frame-processing

"If presentation timestamp is less than appendWindowStart, then set the need random access point flag to true, drop the coded frame, and jump to the top of the loop to start processing the next coded frame. "

appendWindowStart has a default value of 0. So all negative frames will be dropped and then because it has removed a frame, all future frames are now non-decodable and we have to drop until the next keyframe.

As such, we now have a gap at startup, playback can't start.

Bug 1268868 made the append window more strict with the spec.

I'd be keen on closing this bug as invalid. The video content is badly formatted.

How it managed to ever worked is a wonder.
> I'd be keen on closing this bug as invalid.

That's not really a solution -- from a user's perspective, that just ends up looking like Firefox broke a popular site, even if technically we're now more spec-compliant.  (Note that Vine is operated by Twitter, and it's ranked #754 in the US on Alexa: http://www.alexa.com/siteinfo/vine.co )

If you're sure our change was in the direction of spec-copmliance and the problem is in Vine's court, we should perhaps reclassify this bug as Tech Evangelism (indicating that it's a bug in the site & we need to get them to fix their code).  And hopefully, they'll be able to apply a fix before this change makes it to very many Firefox users.  (And the more specific we can be about our suggestions for what they need to fix, the better.)

So: feel free to reclassify as Tech Evang, if you think it's appropriate.  astevenson (from Web Compat team) may be able to help get in touch with Vine, or punt to someone else who can help.
Flags: needinfo?(jyavenard)
Of course I wasn't going to do that :)

I did make a mistake in bug 1269325 and I have a fix; I made an invalid assumption starting on code that wasn't correct to start with.

But per spec, this video shouldn't even start.

On the other hand, the first frame of the audio track is close to our massage threshold. It starts at 100ms and luckily we allow gap of 150ms.
Flags: needinfo?(jyavenard)
Attachment #8751131 - Flags: review?(ajones) → review+
Comment on attachment 8751131 [details]
MozReview Request: Bug 1271847: Only adjust the samples once we have a complete moof. r?kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/51871/#review48675
Comment on attachment 8751131 [details]
MozReview Request: Bug 1271847: Only adjust the samples once we have a complete moof. r?kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51871/diff/1-2/
https://hg.mozilla.org/mozilla-central/rev/c541b055d7af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8751131 [details]
MozReview Request: Bug 1271847: Only adjust the samples once we have a complete moof. r?kentuckyfriedtakahe

Approval Request Comment
[Feature/regressing bug #]: 1269325
[User impact if declined]: Regression. Bug 1269325 was uplifted to aurora. Unfortunately it broke some sites.
[Describe test coverage new/current, TreeHerder]: Manual tests, lots of MSE mochitest.
[Risks and why]: Hopefully none. Bug 1269325 was a workaround, this is the proper fix.
[String/UUID change made/needed]: None
Attachment #8751131 - Flags: approval-mozilla-aurora?
tracking for 48/49 - based on bug comments it would be good for this to be fixed.
Comment on attachment 8751131 [details]
MozReview Request: Bug 1271847: Only adjust the samples once we have a complete moof. r?kentuckyfriedtakahe

Recent regression, playback problems, Aurora48+
Attachment #8751131 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8751131 [details]
MozReview Request: Bug 1271847: Only adjust the samples once we have a complete moof. r?kentuckyfriedtakahe

Approval Request Comment
[Feature/regressing bug #]: 1271847
[User impact if declined]: playback will fail for some site as bug 1269325 got uplift for beta approved.
[Describe test coverage new/current, TreeHerder]: manual test, mochitest in central.
[Risks and why]: same as aurora uplift request
[String/UUID change made/needed]: none
Attachment #8751131 - Flags: approval-mozilla-beta?
Verified fixed in latest Nightly. 49.0a1 (2016-05-12)
Status: RESOLVED → VERIFIED
Comment on attachment 8751131 [details]
MozReview Request: Bug 1271847: Only adjust the samples once we have a complete moof. r?kentuckyfriedtakahe

Complete moofs, what more could we ask for.
Fix verified in nightly, it applied ok to aurora, let's put it in beta 5 though it may only land in time for beta 6 next week.
Attachment #8751131 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1272916
QA Whiteboard: [good first verify]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61091afacd2312e8d7500722414689c70ee4bd1f#l1.38 was using the decode time duration of the last valid trun to adjust decode times of samples from all truns in the moof, which would have produced smaller than intended sample decode durations. Presentation times were unaffected but perhaps the TrackBuffersManager expected contiguous decode times and durations (bug 1272916).

https://hg.mozilla.org/mozilla-central/rev/c541b055d7af2289095cdb36b2a773f9edda8343#l1.169 corrected the decode durations (for valid Moofs) and also changed an invalid Moof with one or more successfully parsed truns to have a zero mTimeRange.

No longer blocks: 1269325
Regressions: 1269325
You need to log in before you can comment on or make changes to this bug.