Skip to content

Commit

Permalink
Allow switching media players with many players loading
Browse files Browse the repository at this point in the history
fixes CNVS-16057

this problem was reproducible without any canvas wrapper at all as long
as we're dynamically throwing a ton of new players at the page every
time we click a link. By default it's causing each new player to try to
preload it's content, and that's causing issues for the browser
as it tries to decide which stream to continue, and it's not great at
breaking away from a queue of active downloads just beacuse a media
player pauses. There are probably smart and great ways to handle this
better inside mediaelement.js by improving that library, but what we
have found to be effective for mitigating the issue for Canvas is to use
the "preload='none'" attribute on each media element as it's added to
the page.  Since we immediately click on it anyway as part of activating
the media comment, it still kicks off the download immediately, but the
browser appears to be more conservative about trying to download all
the media elements on the page at once that way giving smoother
and more snappy transitions between playing various media elements

also did a little dependency breaking to move away from deeply nested
functions inside of "mediaComment.coffee" while we were in there
anyway.

TEST PLAN:
 - create a new page in pages
 - use "instructure record" tinymce plugin to insert like
    20 different media comments
 - go to the page as a consumer (someone visiting to read, not edit)
 - click on the first media comment, let it just start, then open
    another media comment, and after it's played for a second or two,
    open a third, etc.
 - each time you open a new media element it should cleanly shift
   playing focus to the latest engaged element without big lag

Change-Id: I13e2115f15e83fe7c367299865be2e1a16396492
Reviewed-on: https://gerrit.instructure.com/50336
Tested-by: Jenkins
Reviewed-by: Jacob Fugal <jacob@instructure.com>
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: Ethan Vizitei <evizitei@instructure.com>
  • Loading branch information
evizitei committed Mar 16, 2015
1 parent a2f7484 commit 9f0448b
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 8 deletions.
48 changes: 41 additions & 7 deletions app/coffeescripts/jquery/mediaComment.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,42 @@ define [
'str/htmlEscape'
], (I18n, _, pubsub, mejs, $, kalturaAnalytics, htmlEscape) ->

##
# a module for some of the transformation functions pulled out of the middle
# of this jQuery plugin to keep their dependencies light
#
# @exports
MediaCommentUtils = {
##
# given the type and source/track tags, build
# an html5 media element to replace our media comment when interacted
# with
#
# @private
#
# @param {string} tagType should be "audio" or "video" generally, this is
# used for the name of the tag but also to decide whether to include
# width and height
#
# @param {HTML string} st_tags the html for the source and track tags that we
# might want to include inside the media element
#
# @param {int} width the desired width of the element, only applicable for
# video tags
#
# @param {int} height the desired height of the element, only applicable for
# video tags
#
# @returns {jQuery Object} a new dom element (not yet attached anywhere)
# that is the media element
getElement: (tagType, st_tags, width, height) ->
dimensions = ""
if tagType is 'video'
dimensions = " width='#{width}' height='#{height}'"
html = "<#{tagType} #{dimensions} preload='none' controls>#{st_tags}</#{tagType}>"
$(html)
}

VIDEO_WIDTH = 550
VIDEO_HEIGHT = 448
$.extend mejs.MediaElementDefaults,
Expand Down Expand Up @@ -61,14 +97,10 @@ define [
createMediaTag = ({sourcesAndTracks, mediaType, height, width, mediaPlayerOptions}) ->
tagType = if mediaType is 'video' then 'video' else 'audio'
st_tags = sourcesAndTracks.sources.concat(sourcesAndTracks.tracks).join('')
getElement = (tagType) ->
html = "<#{tagType} #{if mediaType is 'video' then "width='#{width}' height='#{height}'" else ''} controls>#{st_tags}</#{tagType}>"
$(html)
willPlayAudioInFlash = ->
opts = $.extend({mode: 'auto'}, mejs.MediaElementDefaults, mejs.MepDefaults,mediaPlayerOptions)
playback = mejs.HtmlMediaElementShim.determinePlayback(getElement('audio')[0], opts, mejs.MediaFeatures.supportsMediaTag, !!'isMediaTag', null)
element = MediaCommentUtils.getElement('audio', st_tags)
playback = mejs.HtmlMediaElementShim.determinePlayback(element[0], opts, mejs.MediaFeatures.supportsMediaTag, !!'isMediaTag', null)
return playback.method != 'native'
# We only need to do this if we try to play audio in a flash player.
Expand All @@ -81,7 +113,7 @@ define [
mediaPlayerOptions.isVideo = false
mediaPlayerOptions.videoHeight = height = 30
getElement(tagType)
MediaCommentUtils.getElement(tagType, st_tags, width, height)
mediaCommentActions =
create: (mediaType, callback, onClose, defaultTitle) ->
Expand Down Expand Up @@ -173,3 +205,5 @@ define [
return console.log('Kaltura has not been enabled for this account') unless INST.kalturaSettings
mediaCommentActions[command].apply(this, restArgs)
this
MediaCommentUtils
26 changes: 25 additions & 1 deletion spec/coffeescripts/jquery/mediaCommentSpec.coffee
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
define [
'jquery'
'compiled/jquery/mediaComment'
], ($)->
], ($, MediaUtils)->

module 'mediaComment',
setup: ->
Expand Down Expand Up @@ -54,3 +54,27 @@ define [
equal @$holder.find('source[type=mp4]').attr('src'),"http://some_mp4_url.com", "Video contains the mp4 source"


module "MediaCommentUtils functions",
setup: ->
teardown: ->

test "getElement includes width and height for video elements", ->
$media = MediaUtils.getElement("video", "", 100, 200)
equal($media.attr("width"), 100)
equal($media.attr("height"), 200)

test "getElement doesnt care about width and height for audio elements", ->
$media = MediaUtils.getElement("audio", "", 100, 200)
equal($media.attr("width"), null)
equal($media.attr("height"), null)

test "getElement adds preload='none' to both types", ->
$video = MediaUtils.getElement("video", "", 100, 200)
$audio = MediaUtils.getElement("audio", "", 100, 200)
equal($video.attr("preload"), "none")
equal($audio.attr("preload"), "none")

test "getElement puts source tags inside the element", ->
st_tag = "<source src='something'></source>"
$audio = MediaUtils.getElement("audio", st_tag)
equal($audio.html(), "<source src=\"something\">")

0 comments on commit 9f0448b

Please sign in to comment.