Skip to content

Fix endedCallback to use sources correctly and not to read after last index - #260

Open
svoboi wants to merge 1 commit into
mediaelement:masterfrom
svoboi:fix/playlist-endedCallback
Open

svoboi wants to merge 1 commit into
mediaelement:masterfrom
svoboi:fix/playlist-endedCallback

Conversation

@svoboi

@svoboi svoboi commented May 12, 2024

Copy link
Copy Markdown

Now the function used after video reaches end sets only the url as a source for fetching, not the whole object.

Also the function now stops at the correct index.

@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, and guarding on player.playlist[++i] with a
decrement on overrun matches the pattern nextPlaylistCallback already uses in
the same file, so it reads consistently with its neighbours.

#226 fixes the same two things via a length - 1 bound. Either one resolves it;
they are equivalent in effect.

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