Skip to content

Hotfix/multiple videos bug #835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
valera-rozuvan wants to merge 1 commit into from
Closed

Hotfix/multiple videos bug #835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions common/lib/xmodule/xmodule/css/video/display.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ div.video {
padding-bottom: 56.25%;
position: relative;

div {
&.hidden {
display: none;
}
}

object, iframe {
border: none;
height: 100%;
Expand All @@ -48,6 +54,15 @@ div.video {
top: 0;
width: 100%;
}

h3 {
text-align: center;
color: white;

&.hidden {
display: none;
}
}
}

section.video-controls {
Expand Down Expand Up @@ -516,6 +531,12 @@ div.video {
height: 0px;
}

article.video-wrapper section.video-player {
h3 {
color: black;
}
}

ol.subtitles {
width: 0;
height: 0;
Expand Down Expand Up @@ -563,6 +584,12 @@ div.video {
position: static;
}

article.video-wrapper section.video-player {
h3 {
color: white;
}
}

div.tc-wrapper {
@include clearfix;
display: table;
Expand Down
2 changes: 2 additions & 0 deletions common/lib/xmodule/xmodule/js/fixtures/video.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
data-end=""
data-caption-asset-path="/static/subs/"
data-autoplay="False"
data-yt-test-timeout="1500"
data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
>
<div class="tc-wrapper">
<article class="video-wrapper">
Expand Down
2 changes: 2 additions & 0 deletions common/lib/xmodule/xmodule/js/fixtures/video_all.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
data-webm-source="xmodule/include/fixtures/test.webm"
data-ogg-source="xmodule/include/fixtures/test.ogv"
data-autoplay="False"
data-yt-test-timeout="1500"
data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
>
<div class="tc-wrapper">
<article class="video-wrapper">
Expand Down
2 changes: 2 additions & 0 deletions common/lib/xmodule/xmodule/js/fixtures/video_html5.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
data-webm-source="xmodule/include/fixtures/test.webm"
data-ogg-source="xmodule/include/fixtures/test.ogv"
data-autoplay="False"
data-yt-test-timeout="1500"
data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
>
<div class="tc-wrapper">
<article class="video-wrapper">
Expand Down
2 changes: 2 additions & 0 deletions common/lib/xmodule/xmodule/js/fixtures/video_no_captions.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
data-end=""
data-caption-asset-path="/static/subs/"
data-autoplay="False"
data-yt-test-timeout="1500"
data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
>
<div class="tc-wrapper">
<article class="video-wrapper">
Expand Down
16 changes: 14 additions & 2 deletions common/lib/xmodule/xmodule/js/spec/helper.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,24 @@ jasmine.stubbedHtml5Speeds = ['0.75', '1.0', '1.25', '1.50']
jasmine.stubRequests = ->
spyOn($, 'ajax').andCallFake (settings) ->
if match = settings.url.match /youtube\.com\/.+\/videos\/(.+)\?v=2&alt=jsonc/
if settings.success
status = match[1].split('_')
if status and status[0] is 'status'
{
always: (callback) ->
callback.call(window, {}, status[1])
error: (callback) ->
callback.call(window, {}, status[1])
done: (callback) ->
callback.call(window, {}, status[1])
}
else if settings.success
# match[1] - it's video ID
settings.success data: jasmine.stubbedMetadata[match[1]]
else {
always: (callback) ->
callback.call(window, {}, 'success');
callback.call(window, {}, 'success')
done: (callback) ->
callback.call(window, {}, 'success')
}
else if match = settings.url.match /static(\/.*)?\/subs\/(.+)\.srt\.sjson/
settings.success jasmine.stubbedCaption
Expand Down
60 changes: 16 additions & 44 deletions common/lib/xmodule/xmodule/js/spec/video/general_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,46 +55,6 @@
expect(this.state.speed).toEqual('0.75');
});
});

describe('Check Youtube link existence', function () {
var statusList = {
error: 'html5',
timeout: 'html5',
abort: 'html5',
parsererror: 'html5',
success: 'youtube',
notmodified: 'youtube'
};

function stubDeffered(data, status) {
return {
always: function(callback) {
callback.call(window, data, status);
}
}
}

function checkPlayer(videoType, data, status) {
this.state = new window.Video('#example');
spyOn(this.state , 'getVideoMetadata')
.andReturn(stubDeffered(data, status));
this.state.initialize('#example');

expect(this.state.videoType).toEqual(videoType);
}

it('if video id is incorrect', function () {
checkPlayer('html5', { error: {} }, 'success');
});

$.each(statusList, function(status, mode){
it('Status:' + status + ', mode:' + mode, function () {
checkPlayer(mode, {}, status);
});
});

});

});

describe('HTML5', function () {
Expand Down Expand Up @@ -154,10 +114,22 @@

it('parse Html5 sources', function () {
var html5Sources = {
mp4: 'xmodule/include/fixtures/test.mp4',
webm: 'xmodule/include/fixtures/test.webm',
ogg: 'xmodule/include/fixtures/test.ogv'
};
mp4: null,
webm: null,
ogg: null
}, v = document.createElement('video');

if (!!(v.canPlayType && v.canPlayType('video/webm; codecs="vp8, vorbis"').replace(/no/, ''))) {
html5Sources['webm'] = 'xmodule/include/fixtures/test.webm';
}

if (!!(v.canPlayType && v.canPlayType('video/mp4; codecs="avc1.42E01E, mp4a.40.2"').replace(/no/, ''))) {
html5Sources['mp4'] = 'xmodule/include/fixtures/test.mp4';
}

if (!!(v.canPlayType && v.canPlayType('video/ogg; codecs="theora"').replace(/no/, ''))) {
html5Sources['ogg'] = 'xmodule/include/fixtures/test.ogv';
}

expect(state.html5Sources).toEqual(html5Sources);
});
Expand Down
116 changes: 89 additions & 27 deletions common/lib/xmodule/xmodule/js/src/video/01_initialize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,6 @@ function (VideoPlayer) {
if (state.parseYoutubeStreams(state.config.youtubeStreams)) {
state.videoType = 'youtube';

state.fetchMetadata();
state.parseSpeed();
return true;
}
return false;
Expand All @@ -157,9 +155,7 @@ function (VideoPlayer) {
// function _prepareHTML5Video(state)
// The function prepare HTML5 video, parse HTML5
// video sources etc.
function _prepareHTML5Video(state) {
state.videoType = 'html5';

function _prepareHTML5Video(state, html5Mode) {
state.parseVideoSources(
{
mp4: state.config.mp4Source,
Expand All @@ -168,20 +164,39 @@ function (VideoPlayer) {
}
);

if (html5Mode) {
state.speeds = ['0.75', '1.0', '1.25', '1.50'];
state.videos = {
'0.75': state.config.sub,
'1.0': state.config.sub,
'1.25': state.config.sub,
'1.5': state.config.sub
};
}

// We must have at least one non-YouTube video source available.
// Otherwise, return a negative.
if (
state.html5Sources.webm === null &&
state.html5Sources.mp4 === null &&
state.html5Sources.ogg === null
) {
state.el.find('.video-player div').addClass('hidden');
state.el.find('.video-player h3').removeClass('hidden');

return false;
}

state.videoType = 'html5';

if (!state.config.sub || !state.config.sub.length) {
state.config.sub = '';
state.config.show_captions = false;
}

state.speeds = ['0.75', '1.0', '1.25', '1.50'];
state.videos = {
'0.75': state.config.sub,
'1.0': state.config.sub,
'1.25': state.config.sub,
'1.5': state.config.sub
};

state.setSpeed($.cookie('video_speed'));

return true;
}

function _setConfigurations(state) {
Expand All @@ -205,7 +220,7 @@ function (VideoPlayer) {
// The function set initial configuration and preparation.

function initialize(element) {
var _this = this;
var _this = this, tempYtTestTimeout;
// This is used in places where we instead would have to check if an element has a CSS class 'fullscreen'.
this.isFullScreen = false;

Expand All @@ -231,28 +246,61 @@ function (VideoPlayer) {
webmSource: this.el.data('webm-source'),
oggSource: this.el.data('ogg-source'),

ytTestUrl: this.el.data('yt-test-url'),

fadeOutTimeout: 1400,

availableQualities: ['hd720', 'hd1080', 'highres']
};

// Check if the YT test timeout has been set. If not, or it is in
// improper format, then set to default value.
tempYtTestTimeout = parseInt(this.el.data('yt-test-timeout'), 10);
if (!isFinite(tempYtTestTimeout)) {
tempYtTestTimeout = 1500;
}
this.config.ytTestTimeout = tempYtTestTimeout;

if (!(_parseYouTubeIDs(this))) {
// If we do not have YouTube ID's, try parsing HTML5 video sources.
_prepareHTML5Video(this);
if (!_prepareHTML5Video(this, true)) {
// Non-YouTube sources were not found either.
return;
}

_setConfigurations(this);
_renderElements(this);
} else {
this.getVideoMetadata()
if (!this.youtubeXhr) {
this.youtubeXhr = this.getVideoMetadata();
}

this.youtubeXhr
.always(function(json, status) {
var err = $.isPlainObject(json.error) ||
(status !== "success" && status !== "notmodified");

if (err){
(status !== 'success' && status !== 'notmodified');
if (err) {
// When the youtube link doesn't work for any reason
// (for example, the great firewall in china) any
// alternate sources should automatically play.
_prepareHTML5Video(_this);
_this.el.find('a.quality_control').hide();
if (!_prepareHTML5Video(_this)) {
// Non-YouTube sources were not found either.

_this.el.find('.video-player div').removeClass('hidden');
_this.el.find('.video-player h3').addClass('hidden');

// If in reality the timeout was to short, try to
// continue loading the YouTube video anyways.
_this.fetchMetadata();
_this.parseSpeed();
} else {
// In-browser HTML5 player does not support quality
// control.
_this.el.find('a.quality_control').hide();
}
} else {
_this.fetchMetadata();
_this.parseSpeed();
}

_setConfigurations(_this);
Expand Down Expand Up @@ -298,7 +346,13 @@ function (VideoPlayer) {
// Take the HTML5 sources (URLs of videos), and make them available explictly for each type
// of video format (mp4, webm, ogg).
function parseVideoSources(sources) {
var _this = this;
var _this = this,
v = document.createElement('video'),
sourceCodecs = {
mp4: 'video/mp4; codecs="avc1.42E01E, mp4a.40.2"',
webm: 'video/webm; codecs="vp8, vorbis"',
ogg: 'video/ogg; codecs="theora"'
};

this.html5Sources = {
mp4: null,
Expand All @@ -308,7 +362,14 @@ function (VideoPlayer) {

$.each(sources, function (name, source) {
if (source && source.length) {
_this.html5Sources[name] = source;
if (
Boolean(
v.canPlayType &&
v.canPlayType(sourceCodecs[name]).replace(/no/, '')
)
) {
_this.html5Sources[name] = source;
}
}
});
}
Expand All @@ -325,7 +386,9 @@ function (VideoPlayer) {

$.each(this.videos, function (speed, url) {
_this.getVideoMetadata(url, function(data) {
_this.metadata[data.data.id] = data.data;
if (data.data) {
_this.metadata[data.data.id] = data.data;
}
});
});
}
Expand Down Expand Up @@ -362,12 +425,11 @@ function (VideoPlayer) {
if (typeof url !== 'string') {
url = this.videos['1.0'] || '';
}

successHandler = ($.isFunction(callback)) ? callback : null;
xhr = $.ajax({
url: 'https://gdata.youtube.com/feeds/api/videos/' + url + '?v=2&alt=jsonc',
timeout: 500,
url: this.config.ytTestUrl + url + '?v=2&alt=jsonc',
dataType: 'jsonp',
timeout: this.config.ytTestTimeout,

@polesye polesye Aug 30, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wedaly In case, when the youtube link doesn't work for any reason (for example, the great firewall in china) any alternate sources should automatically play.
For checking Youtube existence we use JSONP. To decide youtube exist or not we add timeout. If response isn't received in "this.config.ytTestTimeout" ms it means youtube server doesn't exist and we restart our video in HTML5 mode.
For now, we wait 1500 ms is it enough for this reasons? Do you have any other suggestions how to do that?

@wedaly wedaly Aug 30, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem in every distributed system: is a service unavailable or just taking a long time? The correct timeout is going to depend on the user's internet connection and willingness to wait.

1.5 sec seems reasonable to me. If this is an issue affecting edX instances that other organizations are running, then we should make this a feature flag and document how they can configure it.

success: successHandler
});

Expand Down
Loading