force correct duration from YouTube

RESOLVED WONTFIX

Status

Webmaker
popcorn.js
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: thecount, Assigned: thecount)

Tracking

Details

(Whiteboard: s=20130722 p=1)

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
YouTube duration data is rounded. If a video ends at 3.5, duration is going to report 4.

We can  get correct duration time if we get the time after an ended event.

Ended events still trigger at the desired 3.5. On top of that, all it takes is one ended event and YouTube corrects the duration time stored internally, so we just need to be sure we've triggered an ended event before we start asking YouTube for duration.

It is the most complete solution we have to this problem.
(Assignee)

Updated

5 years ago
Assignee: nobody → scott
(Assignee)

Comment 1

5 years ago
Created attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

Builds on bug 893957.

If we land this first, we can just make bug 893957 as resolved.
Attachment #775944 - Flags: review?(schranz.m)
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

I went to the YouTube Wrapper test page and all I saw was a gigantic forest fire.

The code *seems* logical, but only on one condition. I can't remember if seekTo's trigger plays with YouTube. Otherwise it seems to me like it never actually would have that initial play to trigger ended.
Attachment #775944 - Flags: review?(schranz.m) → review-
(Assignee)

Comment 3

5 years ago
Yeah, initial seekTo's do trigger a play.

The advantage to this hack is it kinda replaces another.

I'll look into the test, worth looking into. Not sure if we'll fix them in this ticket. I'll update tomorrow.
We should fix the tests here. This patch is literally causing at least half of them to time out (I made it to about the 17th block, which 16 of those timed out) before just stopping.
(Assignee)

Comment 5

5 years ago
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

There was a handful of tests, give or take 5, some were just really slow.

I think I got them all working now.

I know I said tomorrow, but I lied.

Might take until tomorrow to get an r+, though.
Attachment #775944 - Flags: review- → review?(schranz.m)
(Assignee)

Comment 6

5 years ago
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

Fixed your pull request comments. They were good calls.
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

I can't find things wrong with this personally, but I kind of want someone else to go over it as well.

Not sure who that should be.
Attachment #775944 - Flags: review?(schranz.m) → review+
(Assignee)

Comment 8

5 years ago
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

One more review.
Attachment #775944 - Flags: review+ → review?(chris)
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

Tested the YT Wrapper unit tests on Firefox, Chrome, Safari and Opera, all pass. Lint passes too.

R+ with a small comment fix noted in the pull request.
Attachment #775944 - Flags: review?(chris) → review+
(Assignee)

Comment 10

5 years ago
Comment on attachment 775944 [details] [review]
https://github.com/mozilla/popcorn-js/pull/334

Fixed a handful of comment related issues.
Attachment #775944 - Flags: review+ → review?(chris)

Updated

5 years ago
Attachment #775944 - Flags: review?(chris) → review+
(Assignee)

Comment 11

5 years ago
Staged: https://github.com/mozilla/popcorn-js/commit/0231f67a801488d34ce91bf513ba03dc131c6483
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

5 years ago
For some reason this clip never loads now: http://www.youtube.com/watch?v=TSOyy_8iwz8

Probably others. Not sure why yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Whiteboard: s=20130722 p=1
(Assignee)

Comment 13

5 years ago
Created attachment 779376 [details] [review]
https://github.com/mozilla/popcorn-js/pull/336

The issue was leaving firstPlay inside the stateChange event.

We now check for mediaReady instead, which is much better.

It seems only some YouTube videos first an initial play on seek, so leaving that in there was not safe, and would end up being hit on a valid play call.

We now wait until we have mediaReady before we consider play calls from YouTube to be valid. Any of our potential firstPlay calls happen before mediaReady can ever be true, so we're now safe without it.
Attachment #779376 - Flags: review?(schranz.m)
Comment on attachment 779376 [details] [review]
https://github.com/mozilla/popcorn-js/pull/336

R+ with nits answered/taken care of.
Attachment #779376 - Flags: review?(schranz.m) → review+
(Assignee)

Comment 15

5 years ago
I think my fix, while it works, probably is not the best idea right now.

We have to live with incorrect duration, and simply fix the fringe cases that come from it.

My reasoning for this is it adds extra time to YouTube clips loading, just not worth it.

We have to fix the fringe cases anyway because of old projects.

There are bits form this ticket I'm going to pull into other tickets and fixes and I now have a full understanding of this, so it's not a waste, but as a whole, this is WONTFIX.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → WONTFIX
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.