WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 5 months ago

#42643 closed defect (bug) (fixed)

FLV video format not playing

Reported by: studiosx Owned by: Clorith
Milestone: 4.9.3 Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch
Focuses: javascript Cc:

Description

After updating my site to Wordpress 4.9, FLV video files are no longer supported. The video player does not load and gets the following error: NetworkError: Exception Failed to fetch

Maybe You can fix that in the next update.

Otherwise, You are doing a great job!

Have a nice day!

Attachments (6)

42643.patch (772 bytes) - added by Clorith 6 months ago.
small.flv (296.2 KB) - added by Clorith 6 months ago.
42643.2.patch (1.1 KB) - added by Clorith 5 months ago.
42643.3.patch (1.3 KB) - added by Clorith 5 months ago.
42643.4.patch (1.2 KB) - added by Clorith 5 months ago.
42643.5.patch (1.4 KB) - added by Clorith 5 months ago.

Download all attachments as: .zip

Change History (35)

#1 follow-up: @rafa8626
7 months ago

MediaElement.js now has the ability to load FLV natively but not all FLVs will work. If you add to your configuration something like this it should work:

$('video, audio').mediaelementplayer({
     renderers: ['html5', 'flash_video', 'native_flv', 'youtube_iframe'],
     // other configuration
})

#2 in reply to: ↑ 1 @studiosx
7 months ago

Thank you for your answer. Where should I add it?

Replying to rafa8626:

MediaElement.js now has the ability to load FLV natively but not all FLVs will work. If you add to your configuration something like this it should work:

$('video, audio').mediaelementplayer({
     renderers: ['html5', 'flash_video', 'native_flv', 'youtube_iframe'],
     // other configuration
})

#3 @rafa8626
7 months ago

I think your best choice is to add it in your footer file. If you wanna spread it into the Embeds, you might have to do it in the script-loader.php where the settings for MeJS are set (I think it's after the translations for the plugin)

#4 @Matoo
7 months ago

Hello @studiosx , does it work? I have the same issue, but I don't really know where to add the "snippet" advised by @rafa8626 (Ok I saw the MeJS settings but I'm scared. :D)

#5 @studiosx
7 months ago

No @Matoo . For me is not working. @rafa8626 Thank You anyway!

I have downgrade WordPress to 4.8.3. I hope that this issue will be solved in the future (maybe next update).

#6 @Matoo
7 months ago

Thanks for the feed-back! :)

#7 @Clorith
7 months ago

  • Focuses javascript added
  • Milestone changed from Awaiting Review to 4.9.2

The MediaElement update added support for FLV files as mentioned earlier, the main problem here is that the previous version would not try to render a file, but instead offered a download link right away.

The issue seems to be that the player starts rendering like it should, and then hits something incompatible in the file (likely the encoding used to create it), I wonder if we could possibly grab this with the error event in MEJS and convert the player markup to a download instead as a solution...

This ticket was mentioned in Slack in #core by clorith. View the logs.


7 months ago

#9 @obenland
7 months ago

@Clorith Are you able to work on this?

#10 @Clorith
6 months ago

  • Owner set to Clorith
  • Status changed from new to assigned

Yeah I'll grab it 👍

This ticket was mentioned in Slack in #core-media by clorith. View the logs.


6 months ago

@Clorith
6 months ago

#12 follow-up: @Clorith
6 months ago

  • Keywords has-patch added

42643.patch implements a customError handler for any MEJS player, this restores the previous behavior of displaying a download link if a FLV file can't be rendered for any reason.

It's worth noting that the new render options in MEJS means it'll output the render errors as notices to the JS console no matter what, as they are only notices, they don't prevent other JS from running as intended. That knowledge, along with them only appearing if a rendering fails makes me think that's a non-issue for most cases, as authors are more likely to sort out their videos if they don't display.

This approach would also mean that any other format that encounters an error gets a download link, this wasn't the previous behavior, but it makes sense to not limit this fallback only to flash videos.

It would be great if those affected by the issue in the ticket had a chance to test the patch as well, so we can try to spot any edge cases that may pop up, I've so far done tests with FLV and mp4 files and they at least seem fine in major browsers (including the notorious IE11).

@Clorith
6 months ago

#13 @Clorith
6 months ago

small.flv is the flash file used for testing, it's invalid for playback due to the audio codec used, and will therefor call the customError from 42643.patch, as well as the mentioned console.error() calls from the logger.js file bundled with flv.js.

flv.js does have a way to disable the logger, but I didn't see a way to forward the disable flag from MEJS.

#14 @Clorith
6 months ago

So, we can pass in and disable debugging and verbose logging in flv.js, unfortunately the enableError (which is what is actually triggering here) isn't made available to us.

https://github.com/WordPress/WordPress/blob/4.9-branch/wp-includes/js/mediaelement/mediaelement-and-player.js#L6259-L6260

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


5 months ago

#17 in reply to: ↑ 12 @mikeschroder
5 months ago

Replying to Clorith:

42643.patch implements a customError handler for any MEJS player, this restores the previous behavior of displaying a download link if a FLV file can't be rendered for any reason.

<snip>

This approach would also mean that any other format that encounters an error gets a download link, this wasn't the previous behavior, but it makes sense to not limit this fallback only to flash videos.

I’m a little concerned that a download link might not be something that users embedding seemingly-to-be-streaming media would expect to show up upon error, even though files are, of course, downloadable manually anyway.

For this reason, I'd personally prefer a fix specific to FLV, if I'm understanding properly, but would love to hear other thoughts on this.

@Clorith
5 months ago

#18 @Clorith
5 months ago

42643.2.patch ensures the error handling and download fallback only applies to flash/flv renders.

#19 @adamsilverstein
5 months ago

@Clorith patch looks good, nice work! Took me a bit to figure out I had to be on the front end to see the error.

42643.2.patch could use few small changes: tabs please instead of spaces, and more whitespace in node.src.substring. finally, JSDocs for the parameters and return would also be great.

I tested with the flv you provided, here are some screenshots for a visual record of this change:

Before the patch: https://cl.ly/1n2Z3N3a3u1x/Image%202018-01-12%20at%204.19.57%20PM.png

After the patch I see the custom error and its linked:

https://cl.ly/393O0j1j302l/Image%202018-01-12%20at%204.18.35%20PM.png

@Clorith
5 months ago

#20 @Clorith
5 months ago

Those tabs falling out when I switch editors will be the end of me :)

42643.3.patch Fixes indentations, spacing added to parameters, and adds JSDocs. I've not done JSDocs before, so do let me know if there's anything wrong with them :)

#21 @birgire
5 months ago

Hi @Clorith, I tested the patch in 42643.3.patch with small.flv and here are few notes:

  1. I get 139 js errors: flv.min.js:6 [TransmuxingController] > DemuxException: type = CodecUnsupported, info = Flv: Unsupported codec in video frame: 2, no matter if the patch 42643.3.patch is applied or not. Maybe it's just my test install?
  2. Would it be possible to use e.g. media.rendererName.match( /(flv|flash)/i ) (returns null or array) instead of multiple indexOf() like: -1 !== media.rendererName.indexOf( 'flash' ) || -1 !== media.rendererName.indexOf( 'flv' ) ?
  3. Shouldn't we have the Download string translatable? So in my language (Icelandic) I would see e.g.<a href="http://test.localhost/wp-content/uploads/2018/01/small.flv?_=1">Sækja small.flv?_=1</a>. Maybe one could even reuse the 'mejs.download-video' string from mejsL10n in /wp-includes/script-loader.php ?
  4. Should we remove the ?_=1 in the link text, e.g. <a href="http://test.localhost/wp-content/uploads/2018/01/small.flv?_=1">Download small.flv</a>, since the extra ?_=1 might be confusing to the user and since the downloaded file is also without it: small.flv?
Last edited 5 months ago by birgire (previous) (diff)

@Clorith
5 months ago

#22 @Clorith
5 months ago

@birgire the notices you are seeing from Transmux are unfortunately what I was referencing in comment:14 which there's no way to pass logging-parameters for. They are just notices, and thus do not prevent further JavaScript from running as intended, but there's nothing we can do about these without a patch upstream (I've created an issue for it).

For your 2nd question, I've always heard that indexOf is better performance wise than using match, so unless more regex is needed for checking, it's better to keep this approach.

You are quite right about the translatable portion though, let's restore the previous behavior and use the mejsL10n.strings["mejs.download-video"] string, that'll cut out point 4 completely, but is a direct restoration of the previous behavior, so I like that better.

I've made the string changes in 42643.4.patch

#23 @birgire
5 months ago

  1. ok I see, seems to be from this line: e.ENABLE_ERROR && (console.error ? console.error(i) : console.warn ? console.warn(i) : console.log(i))
  2. yes it's probably better to use indexOf() here - if maybe other formats need to be handled in the future, then one could reconsider.
  3. great
  4. nice

I thought mejs.download-video would be used elsewhere, but I couldn't find other use cases. So I guess it's a good thing to use it then ;-)

I'm not well versed in the javascript coding standard, so I wonder if either mejsL10n.strings["mejs.download-video"] or mejsL10n.strings['mejs.download-video'] is preferred?

42643.4.patch translates the string and looks otherwise good to me ;-)

#24 @birgire
5 months ago

ps: I tested

$grunt jshint:core --file=src/wp-includes/js/mediaelement/wp-mediaelement.js
Running "jshint:core" (jshint) task

   src/wp-includes/js/mediaelement/wp-mediaelement.js
     65 |                    return '<a href="' + node.src + '">' + mejsL10n.strings["mejs.download-video"] + '</a>';
                                                                                                          ^ Strings must use singlequote.
     65 |                    return '<a href="' + node.src + '">' + mejsL10n.strings["mejs.download-video"] + '</a>';
                                                                    ^ 'mejsL10n' is not defined.

>> 2 errors in 1 file
Warning: Task "jshint:core" failed. Use --force to continue.

Aborted due to warnings.

so it seems to prefer strings within single quotes.

I'm not sure about the second error, since it works as a global on my install. But maybe it could be injected differently to silence jshint?

#25 @dd32
5 months ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

@Clorith
5 months ago

#26 @Clorith
5 months ago

42643.5.patch takes care of the JS linting.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 months ago

#28 @SergeyBiryukov
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42582:

Media: Introduce a custom error handler for MediaElement.js to display a download link if a FLV file cannot be rendered for any reason.

Props Clorith.
Fixes #42643.

#29 @SergeyBiryukov
5 months ago

In 42583:

Media: Introduce a custom error handler for MediaElement.js to display a download link if a FLV file cannot be rendered for any reason.

Props Clorith.
Merges [42582] to the 4.9 branch.
Fixes #42643.

Note: See TracTickets for help on using tickets.