search

mafintosh / playback

Video player built using electron and node.js
https://mafintosh.github.io/playback/
MIT License
2.01k stars 237 forks source link

Split out server/controller process from renderer process (and more) #86

Open bmathews opened 8 years ago

bmathews commented 8 years ago

There's a lot here, but I'd like to get some feedback and have people test this out. This improves code quality, manageability, and stability/behavior of the app. Browse the code here: https://github.com/bmathews/playback/tree/refactor-rebase/app

The new command to run things is npm run dev

Changes:

image

image

image

image

image

seungjulee commented 8 years ago

+1 except for the original default screen of cat. Great work!!

mafintosh commented 8 years ago

@bmathews is this LGTM?

bmathews commented 8 years ago

@mafintosh I'd say so, yeah.

mafintosh commented 8 years ago

@freeall do you have any final comments on this? Otherwise I'm gonna merge it tomorrow

freeall commented 8 years ago

I'm a little conflicted on this one. It's really good to see some activity on the project, and I love the fact that it's rewritten to React. On the other hand I don't like that in the same PR the code's been rewritten to ES6 and we now use Babel.

If I were to comment on just the style it's more frontend javascript than node which is probably fine in itself, but it changes the whole codebase and I would rather have that discussion in a PR for itself.

Just to get more activity on this I'd say we should merge it, though. Thoughts?

bmathews commented 8 years ago

@freeall I'd be happy to remove the Babel usage for everything other than the react compilation! It'll add some code, but shouldn't be too bad. Happy to fix/change/remove anything you're not stoked on, though.

Thanks!

zeke commented 8 years ago

It's kind of scary to make changes this big on an app that doesn't have a test suite, but I agree that forward momentum is also important. I'm not really a stakeholder here; just leaving my ¢¢.

it's more frontend javascript than node

Can you elaborate on this, @freeall?

freeall commented 8 years ago

@bmathews Can't we remove Babel altogether? @mafintosh says that electron has es6 support

Edit: misspelling

bmathews commented 8 years ago

@freeall We're taking advantage of electron's ES6 support and using a few extra babel transforms for things that aren't in it yet: import, destructuring, rest params, class properties, and the spread operator.

@zeke :100: The tricky part with the old code base was that the server and view were really coupled, so it would have been difficult to create any tests at all. After splitting these things up, we'll be in a much better place to add tests.

mafintosh commented 8 years ago

ah i didn't notice that this changes all the code to es6. i'm :-1: on that in general.

bmathews commented 8 years ago

@mafintosh @freeall Thanks for the feedback! I'll drop the babel transforms for everything but the React JSX and we'll give it another review.

mafintosh commented 8 years ago

the design is looking awesome though! really wanna pull that in. great job on that @bmathews

bmathews commented 8 years ago

@mafintosh @freeall Pushed changes that remove babel compilation for everything but the React JSX and I switched the eslint config to https://github.com/feross/standard instead of airbnb's preset.

feross commented 8 years ago

FWIW, we're using Electron for Webtorrent.app and it supports spreads and the rest operator natively (actually pretty useful). So you shouldn't need Babel for that. import is just silly and doesn't give you anything over require, IMO.

Also, instead of JSX you might consider substack's hyperx which just uses ES6 template strings so there's no preprocessor required.

freeall commented 8 years ago

I hadn't seen hyperx before. I really like it.

ppeczek commented 8 years ago

Why isn't it merged? ;<

phated commented 8 years ago

Too complicated (should have been small PRs) and now way out of sync.

bmathews commented 8 years ago

Yeah, it was a big architectural change needed to split out the view from the 'server' and still support the 'follow' functionality.

I'm happy with my changes though. :) It solves about half of the open issues, and the VLC/MKV support is nice.

phated commented 8 years ago

@bmathews still, it doesn't merge cleanly and should all be squashed

bmathews commented 8 years ago

It did in February and can easily be squashed. That's the least of the people's concerns, I think. On Jun 18, 2016 6:54 PM, "Blaine Bublitz" notifications@github.com wrote:

@bmathews https://github.com/bmathews still, it doesn't merge cleanly and should all be squashed

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mafintosh/playback/pull/86#issuecomment-226974850, or mute the thread https://github.com/notifications/unsubscribe/AAzx2_ZGcoybK0RrpmzyHvzbpHy-Df3Wks5qNKFugaJpZM4HdZD3 .

overlookmotel commented 7 years ago

@bmathews I know this PR has been quiet a long time and I guess is sadly not getting merged.

I haven't looked through the code - obviously there's a lot of it. But one of your comments above got me intrigued - you said "the VLC/MKV support is nice". I opened an issue to discuss how to support other codecs https://github.com/mafintosh/playback/issues/124. Can you elaborate on how you managed VLC/MKV support, or point me in the direction of the right bit of code please?

bmathews commented 7 years ago

The architecture I put together allowed for swappable 'players'. E.g., the normal HTML <video />, or a chromecast, etc. This allowed me to create a 'player' that used WebChimera (libVLC) to render individual frames of videos that I'd then display on an html canvas.

https://github.com/bmathews/playback/blob/refactor-chimera-rebase/app/players/WebChimera.js

It'd probably need a bit of work to resurrect at this point though, and I'd probably go down this route to improve performance: https://github.com/webtorrent/webtorrent-desktop/issues/938#issuecomment-280462646

overlookmotel commented 7 years ago

@bmathews Thanks loads for these links. I was totally unaware of WebChimera - looks awesome.

I also wonder if a similar thing could be done with FFMPEG? I'm thinking FFMPEG decoding video to raw frames which are passed to Electron and then written pixel-by-pixel into a canvas element. If I've understand correctly, this is roughly what WebChimera is doing.

I have a need to play some codecs which VLC does not handle e.g. Apple Prores4444 and JPEG2000 (used in Digital Cinema DCP format).

Your comment you linked to about OpenGL is also interesting. Was the high CPU usage related to rendering the canvas in JS?

Please excuse my newbie questions. This is an area I'm interested in but I have no knowledge at all of OpenGL and the like. I'm trying to feel out whether it'd take a massive amount of time for me to learn enough and implement to achieve what I'm after, and to what degree the foundations are already out there in the open source world.