Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 11 months ago

#42643 closed defect (bug) (fixed)

FLV video format not playing

Reported by: studiosx's profile studiosx Owned by: clorith's profile 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 7 years ago.
small.flvโ€‹ (296.2 KB) - added by Clorith 7 years ago.
42643.2.patchโ€‹ (1.1 KB) - added by Clorith 7 years ago.
42643.3.patchโ€‹ (1.3 KB) - added by Clorith 7 years ago.
42643.4.patchโ€‹ (1.2 KB) - added by Clorith 7 years ago.
42643.5.patchโ€‹ (1.4 KB) - added by Clorith 7 years ago.

Download all attachments as: .zip

Change History (35)

#1 follow-up: @rafa8626
7 years 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 years 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 years 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 years 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 years 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 years ago

Thanks for the feed-back! :)

#7 @Clorith
7 years 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 years ago

#9 @obenland
7 years ago

@Clorith Are you able to work on this?

#10 @Clorith
7 years 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.


7 years ago

@Clorith
7 years ago

#12 follow-up: @Clorith
7 years 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
7 years ago

#13 @Clorith
7 years 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
7 years 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.


7 years ago

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


7 years ago

#17 in reply to: โ†‘ย 12 @kirasong
7 years 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
7 years ago

#18 @Clorith
7 years ago

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

#19 @adamsilverstein
7 years 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
7 years ago

#20 @Clorith
7 years 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
7 years 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 7 years ago by birgire (previous) (diff)

@Clorith
7 years ago

#22 @Clorith
7 years 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
7 years 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
7 years 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
7 years ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

@Clorith
7 years ago

#26 @Clorith
7 years ago

42643.5.patchโ€‹ takes care of the JS linting.

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


7 years ago

#28 @SergeyBiryukov
7 years 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
7 years 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.