Skip to content

Fixing playlist behavior when track ends - #226

Open
Bakual wants to merge 1 commit into
mediaelement:masterfrom
Bakual:FixPlaylist
Open

Bakual wants to merge 1 commit into
mediaelement:masterfrom
Bakual:FixPlaylist

Conversation

@Bakual

@Bakual Bakual commented Jun 29, 2022

Copy link
Copy Markdown

This should both fix the issues #225 and #223

Means it properly detects the end of the playlist (since the currentPlaylistItem starts with 0) and it properly sets the Source-URL of the PlaylistItem, and not the whole object as new player source (same as it is done for prev/next buttons).

@anirudhatalmale6-alt

Copy link
Copy Markdown

Reproduced on a 3-item video playlist built from <source> children, with
mediaelement 7.0.7 and mediaelement-plugins 5.0.0.

The two lines involved are byte-identical in 5.0.0, in 5.1.0 and in current
master, so this still ships today.

One detail I have not seen spelled out on #223 or #225, which explains why
passing the object (rather than .src) fails specifically here. Core's
changeRenderer decides between object and string by key count,
src/js/core/mediaelement.js on master:

media = Object.keys(mediaFiles[0]).length > 2 ? mediaFiles[0] : mediaFiles[0].src

Three or more keys and the object is handed to the renderer. renderers/hls.js
and renderers/dash.js both unwrap it:

typeof value === 'object' && value.src ? value.src : value

but renderers/html5.js does not, it is a bare node[propName] = value. So the
<video> src attribute becomes the literal string [object Object], that
load 404s, and the browser silently falls back to the first <source> child,
which is item 1.

Measured on the player, varying only the argument:

3-key object {src, title, type}  ->  src attr = [object Object]
2-key object {src, type}         ->  src attr = /video/2.mp4      (plays)
plain string                     ->  src attr = /video/2.mp4      (plays)

And createPlayList_ only accepts a <source> carrying src AND type AND
title, so every source-derived playlist entry has at least three keys and
always takes the object branch. That is why this fails every time for
source-derived playlists rather than intermittently.

It also explains why the symptom reads as "the first item loops" rather than as
an error, which is probably part of why it has sat so long: the failed load is
silent and the <source> fallback looks like deliberate looping. Meanwhile
currentPlaylistItem has already advanced, which is exactly the "replays the
first one but displays the info from the second item" in #225.

Worth noting the plugin's own prevtrack, nexttrack and shuffle callbacks
all already pass .src; endedCallback is the only setSrc call in the file
that does not.

The separate off-by-one matters too: without the bound fix, the last item calls
setSrc(undefined) and throws Cannot read properties of undefined (reading 'src'), leaving the player dead rather than just stopped.

This PR fixes both defects. #260 does the same two things, guarding on
player.playlist[++i] with a decrement on overrun instead of the length - 1
bound, which matches the pattern nextPlaylistCallback already uses in this
file. Either one resolves it; they are equivalent in effect.

Flagging the overlap so whoever reviews does not have to work out whether these
two conflict. They do not, it is the same fix twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants