m1k1o / go-transcode

On-demand transcoding origin server for live inputs and static files in Go using ffmpeg. Also with NVIDIA GPU hardware acceleration.
Apache License 2.0
212 stars 39 forks source link

Some refactoring #3

Closed klahaha closed 2 years ago

klahaha commented 2 years ago

feedback welcome (i'm new to golang)

also there's a lot of stuff in root dir it's littlee confusing, do you like to move source to src/ (not sure golang likes that), build/test scripts (dev/ data/ and tests) to bin/, and profiles mixed together, like this:

m1k1o commented 2 years ago

I would omit those points:

Reverse proxy should be encouraged, I agree, but regarding that is it basically almost no overhead in implementing, it could stay as an option. Even cconnecting to a reverse proxy might benefit from HTTPs (for various usecases).

Proxy flag is for the future, if X-Forwarded-For headers should be trusted or not.

My main inspiration in code skeleton comes from other project that I maintain https://github.com/m1k1o/neko and https://github.com/m1k1o/neko-rooms where I try to match patterns.

Regarding project layout, I am trying to stick to the offical standard: https://github.com/golang-standards/project-layout

klahaha commented 2 years ago

Proxy flag is for the future, if X-Forwarded-For headers should be trusted or not.

4

remove TLS support (Server.Key and Server.Cert) encourage reverse proxy

I think it's bad security to have TLS in static build which does not receive system security upgrades. i can add nginx reverse proxy to docker by default if good with you... or add warning log that TLS mode should never be trusted?

Regarding project layout, I am trying to stick to the offical standard

that link is not a standard it's more like a starter kit (explained in readme), but was made before go modules existed. i don't know too much for golang so if you're happy with structure i'll just add some "architecture" lines to readme to understand easier

m1k1o commented 2 years ago

or add warning log that TLS mode should never be trusted?

That is a fair point. We can add warning message when starting a server.

that link is not a standard it's more like a starter kit (explained in readme), but was made before go modules existed. i don't know too much for golang so if you're happy with structure i'll just add some "architecture" lines to readme to understand easier

Regarding the architecture, I would like to keep is similar to the other projects I am maintaining. It is then easier for me to change between projects. But I am also not against new folder structure, if this project could benefit from that.

klahaha commented 2 years ago

We can add warning message when starting a server.

Done

I would like to keep is similar to the other projects I am maintaining

that's fair, do you think:

so instead of bin/data/dev we have bin or dev . less visual information when browsing repository what do you think? is it compatible with your architecture?

klahaha commented 2 years ago

most boxes are checked

m1k1o commented 2 years ago

remove bin or dev (use one folder not two)

Dev is used only as helpers during development. Bin is meant for build artifacts (not checked to repostitory).

so instead of bin/data/dev we have bin or dev . less visual information when browsing repository what do you think? is it compatible with your architecture?

I think at this point, /bin is unused, since our only build artifact is one executable binary named go-transcode and not checked in repository.

move data/test* to tests/ (what are these files for?)

Those two test files simulate with transcoding for testing purposes. Normally, you would call profile with file source. These test files have just dummy input.

move data/play.html to internal/api/ where it's served.. maybe directly in source code (it's just a demo users will not use it) or use compile time macro to load it?

We can use go:embed (I used it here) to add it to binary.

do you want to do docker stuff? i can do it but i don't have nvenc so i cant test nvidia profiles

Yes, I would like to try it with docker and add my changes. I already have some refactoring ideas in my mind. If you don't mind, I'll push it to this branch, github allows it for PRs (if user does not explicitly disable it).

klahaha commented 2 years ago

If you don't mind, I'll push it to this branch

Sure, let's try not force push and make sure we don't push broken things

klahaha commented 2 years ago

i noticed 99% of code in ffmpeg scripts is the same, so maybe we can all do in one direcotory

on this branch, should i push it to this PR? or something you don't like?

edit: it makes easy to update settings to an entire profile for example http_h264.sh not have to edit 4 different files for qualities

m1k1o commented 2 years ago

it makes easy to update settings to an entire profile for example http_h264.sh not have to edit 4 different files for qualities

Oh yeah, that is a good way how to remove duplication of the code. I would like to keep it as simple as posibble though, don't know if that does not complicate it more. Maybe creating some more general ffmpeg command builder would be a good idea.

klahaha commented 2 years ago

OK i

i think only blocking issue is panic from review; rest can be written TODO or opened another issue:

agree?

m1k1o commented 2 years ago

i think only blocking issue is panic from review; rest can be written TODO or opened another issue.

Yes, I agree, that we should merge changes and then address new changes in different PRs.

Although one thing, that changed with this PR is name of the default config file. Before, it was streams.yaml and now it is transcode.yaml. Maybe, we could rename it to config.yaml, because context of the software, where it is running already says directory (e.g. /etc/transcode/transcode.yaml vs /etc/transcode/config.yaml).

klahaha commented 2 years ago

ok are we good?