lbryio / lbry-desktop

A browser and wallet for LBRY, the decentralized, user-controlled content marketplace.
https://lbry.tech
MIT License
3.56k stars 413 forks source link

Support more media formats in the built in player. #1003

Closed xorgy closed 3 years ago

xorgy commented 6 years ago
## The Issue Content with codecs not supported by the default build of Electron is not playable within LBRY. ### Expected behaviour LBRY should be able to play all the reasonably well tested common codecs supported by FFMPEG (which is what Electron/Chromium uses to demux and decode video/audio). ### Actual behaviour Sometimes a play button is initially shown, but once enough of the file has buffered in it will say that it can not play the file. ## Anything Else [A thread on the Electron Github project](https://github.com/electron/electron/issues/3259) suggests that more codecs can be supported by whitelisting them in the Electron build. ## Screenshots ![axed](https://user-images.githubusercontent.com/1272018/35950541-f2289c02-0c6e-11e8-8ac0-368313e5a827.png)
xorgy commented 6 years ago

I'm looking for a good source of information on which codecs would be lower or higher risk (security/robustness wise) to support. I think Chrome OS maintains a whitelist for the separate Video player (not for browser windows) which might be close to what I'm looking for.

kauffj commented 6 years ago

@xorgy I think it's less about security and more about what's possible to support. I don't see how <video src="xxx.ext" /> can be an attack vector regardless of what it's pointing to.

xorgy commented 6 years ago

@kauffj Well, it can be an attack vector if the demuxer or decoder is compromised in some way. Many people, especially Google, have done a tremendous amount of work pounding serious vulnerabilities out of FFMPEG for the formats which are important to the web, but considerably less effort has gone into finding and fixing vulnerabilities in the codepaths that handle other formats.

Demuxing and decoding media files is fundamentally a parsing task, and parsers are almost always an attack vector.

Some of this may be mostly addressed by the sandboxing code which is in Chromium (and I think Electron builds with sandboxing intact), but you know, there's always a risk.

That said, I tend to agree that it's probably fine to have most of the common formats supported, and it probably isn't any more risky than users opening it in whatever video player they have installed.

kauffj commented 6 years ago

I would assume that Chrome's sandboxing leaves us fully covered here. If I can use a <video> tag as an attack vector in our app, then I should be able to use that same attack vector on the web against any Chrome browser.

xorgy commented 6 years ago

@kauffj The difference is codec support. Chrome has a fairly short whitelist of formats, whereas this feature involves whitelisting more of them (some of which have lower quality implementations than the ones whitelisted by Chrome); and while I have trust in Google's seccomp-fu, I wouldn't say the additional risk is zero.

But I'll go ahead anyway, because the chances are that people will open the file in another player if it's not supported by the internal one.

tzarebczan commented 5 years ago

Some videojs plugins to consider: https://npmjs.org/package/videojs-contrib-dash-s1 https://npmjs.org/package/videojs-contrib-hls https://npmjs.org/package/videojs-gifplayer

Sounds like other formats/codecs like AVI/MKV/WMV/MOV/MPEG may require a separate video player if we want to support them in app.

eggplantbren commented 5 years ago

If I recall correctly, FLAC audio used to work in the old player, but doesn't now. It would be nice to have this back if it's possible and not too tricky. Example FLAC file: lbry://@iykury#c/spoopy#b

xorgy commented 5 years ago

@eggplantbren FWIW, ordinary chrome plays FLAC natively, so it's strange to me that this CEF application wouldn't.

eggplantbren commented 5 years ago

@xorgy It's because they replaced the Chrome media player with a generally-better videojs player. But your comment inspired me to try this:

lbry://flac-test#c

It's a little embedded web page that plays a flac file, and it works.

tzarebczan commented 5 years ago

I believe part of the issue is that we were missing the flac/audio type ... with the new player, we are stricter on what formats we try (the old one used to use the extension). Opened https://github.com/lbryio/lbry-sdk/pull/2466 and will re-try with that. I wonder if these work with range requests, and if not, we'll need to do something special (i.e. download the file / actually read from it) for these cases.

DispatchCommit commented 3 years ago

video.js gives the most versatility, and anything short of that should be handled by the ingestion process.