Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set bitrate limits for transcoding (fixes #638) #1135

Merged
merged 28 commits into from
Oct 8, 2018

Conversation

Nutomic
Copy link
Contributor

@Nutomic Nutomic commented Sep 28, 2018

I haven't tested this yet, because I already spent way too much time researching all the different recommendations for video bitrates. I haven't even tested this yet...

Turns out this topic is extremely complicated, probably enough for someone to write a computer science master thesis on optimizing it.

TODO:

  • write tests
  • verify quality of output videos
  • write a script to transcode old, high bitrate videos with these bitrate values
  • make optimize-old-videos.ts optimize owned videos only
  • make optimize-old-videos.ts regenerate the associated torrents (and other metadata in the db?)
  • write documentation for optimize-old-videos.ts

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 28, 2018

I also found out about Adaptive bitrate streaming. Does Peertube implement this already?

@rigelk rigelk self-assigned this Sep 28, 2018
@rigelk rigelk added the Status: Requires Tests Either requires manual test, or writing tests, or both label Sep 28, 2018
@rigelk
Copy link
Collaborator

rigelk commented Sep 28, 2018

We use our own implementation of adaptative bitrate streaming. Since we don't use HTTP directly but webtorrent, that's more practical than adapting existing ABS implementations.

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 28, 2018

Hmm, for some reason transcoding is not working on my local dev instance. ffmpeg version is n4.0.2 (on Manjaro Linux), is that too new?

Also, I noticed that there aren't any tests for transcoding or other helpers yet. And I don't understand Typescript well enough to write that from scratch.

@rigelk
Copy link
Collaborator

rigelk commented Sep 28, 2018

@Nutomic hopefully we have tests https://github.com/Chocobozzz/PeerTube/blob/develop/server/tests/api/videos/video-transcoder.ts (and we spent some decent time on them ^^)

Your version of ffmpeg should be fine. Newer versions of ffmpeg have never introduced problems, but rather improved performance. I myself run the same version if I recall.


// Force a keyframe every 2 seconds, and on scene cuts.
// https://superuser.com/a/1098329
command.outputOptions('-force_key_frames "expr:gte(t,n_forced*2)"')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is probably the one causing the error. Try without it first.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually running your code without that part passes the test. Since it's out of scope of the current issue at stake, I suggest you remove it so that we can merge the rest. We can always deal with that option in another PR once you have made it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I noticed that too. No idea why it is failing, as the same parameter works fine on the command line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably clashes with some other option in use in PeerTube ; but to properly debug it from within our library to ffmpeg (fluent-ffmpeg), we have yet to attach a logger.

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 29, 2018

Wrote a test (didn't find that file before), added a script to optimize old videos, and moved the keyframe stuff into a new issue.

Transcoding is still not working if I upload a video into the dev instance, but it works fine from the script. Transcoding is enabled in Peertube settings. Maybe it will help if I reset the database etc.

package.json Show resolved Hide resolved
@rigelk
Copy link
Collaborator

rigelk commented Sep 29, 2018

Okay - that looks overall good - great work! I will do a proper review tomorrow (it's quite late here)

If in the meantime you still have the error after upload, that needs to be investigated, because that means our test suite is not covering an important case.

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 29, 2018

Thanks :)

I'm not getting any upload error, Peertube just shows "waiting for transcoding", but doesn't start FFmpeg. It might just be a problem with my setup.

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 29, 2018

I just realized that the optimize videos script still has some problems:

  • redundant videos from other servers from other servers should be ignored
  • torrents need to be regenerated after running the script

That means simply going over the files won't be enough, and we have to get the info from the database somehow.

@rigelk
Copy link
Collaborator

rigelk commented Sep 29, 2018

Both very good points. The prune script makes the difference between "owned" and other videos that come from the redundancy, so at least you have an example of that. Torrents are generated in

async createTorrentAndSetInfoHash (videoFile: VideoFileModel) {

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 29, 2018

Thanks! It seems like I can get the VideoModel with VideoModel.loadByUUIDWithFile(uuid), and then call createTorrentAndSetInfoHash() on it. But how do I get the VideoFileModel for that? It would be really good if there were some comments in the video.ts file (which has 1500 lines!).

@rigelk
Copy link
Collaborator

rigelk commented Sep 29, 2018

Alright. From within a failing transcoding job's logger instance, I get the following stderr (stdout is empty):

Stream mapping:
  Stream #0:0 -> #0:0 (vp8 (native) -> h264 (libx264))  
  Stream #0:1 -> #0:1 (vorbis (native) -> aac (native))
[aac @ 0x562aaaf0dec0] [Eval @ 0x7ffc31f4a640] Undefined constant or missing '(' in 'undefined'
[aac @ 0x562aaaf0dec0] Unable to parse option value \"undefined\"
[aac @ 0x562aaaf0dec0] Error setting option maxrate to value undefined.
Error initializing output stream 0:1 -- Error while opening encoder for output stream #0:1 - maybe incorrect parameters such as bit_rate, rate, width or height
Conversion failed!"

The input file is a .webm from the tests. Most likely this is due to you getting metadat.format.bit_rate, which might return undefined for no apparent reason… 😞

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 29, 2018

Okay that one should be fixed. Not sure if tests are working now because they are timing out for me, but that might just be because of my slow laptop.

I still need help figuring out how to recreate the torrent (and update the file size?). Also, are the checks in optimize-old-videos.ts enough to make sure it doesn't touch any files that are currently being uploaded or transcoded?

@rigelk
Copy link
Collaborator

rigelk commented Sep 29, 2018

@Nutomic a VideoModel holds an array of the files related to it (the different versions of a same video, each with a different resolution/fps pair). It's accessible as VideoModel.VideoFiles: VideoFileModel[]. I guess we don't document it because it follows the syntax defined by sequelize (which has a rich documentation), but that's still difficult for me ;)

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 29, 2018

Thanks, I think I figured it out for the most part. This is all pretty complicated because I never touched Nodejs before. So I have to look up every little thing that I wouldn't even have to think about in Java...

There are still some problems:

  • It looks like I really broke the transcoding somehow, that's why the tests are failing. But I have no clue what could be the problem, and I can't see any errors reported.
  • optimize-old-videos.ts:48 throws an error, for which I couldn't find any solution online: Error: Model not initialized: "ApplicationModel" needs to be added to a Sequelize instance before "findOne" can be called.
  • Repeating from my last comment in case you missed it: are the checks in optimize-old-videos.ts enough to make sure it doesn't touch any files that are currently being uploaded or transcoded?

// quality according to Google Live Encoder: 3000 - 6000 Kbps
// Quality according to YouTube Video Info: 3277 Kbps
return 3300 * 1000
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that Peertube supports 60 fps video, so I set a higher bitrate for that. Unfortunately I can't use the VIDEO_TRANSCODING_FPS constant from server/initializers because that throws an error when I import it here (probably because this is in the shared/ code).

@rigelk
Copy link
Collaborator

rigelk commented Sep 30, 2018

@Nutomic I leave aside the questions I can't answer as of now, but don't worry I didn't forget about them. I'll let @Chocobozzz answer them next week.

Copy link
Collaborator

@rigelk rigelk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the file …is 3.5MBytes 😅

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 30, 2018

Okay I can probably just convert one of the existing files.

@Nutomic
Copy link
Contributor Author

Nutomic commented Sep 30, 2018

So unless I just put a 240p or 360p video, the file will always be pretty large. Or should I convert a video on the fly inside the test?

@rigelk
Copy link
Collaborator

rigelk commented Sep 30, 2018

@Nutomic you could generate one on the fly with ffmpeg for the test, yes.

@Nutomic
Copy link
Contributor Author

Nutomic commented Oct 8, 2018

Just rebased on develop. Tests are still failing, but that seems completely unrelated to my changes...

@Chocobozzz
Copy link
Owner

Thanks @Nutomic

@Chocobozzz Chocobozzz merged commit edb4ffc into Chocobozzz:develop Oct 8, 2018
// https://slhck.info/video/2017/03/01/rate-control.html
// https://trac.ffmpeg.org/wiki/Limiting%20the%20output%20bitrate
const targetBitrate = getTargetBitrate(options.resolution, fps, VIDEO_TRANSCODING_FPS)
command.outputOptions([`-b:v ${ targetBitrate }`, `-maxrate ${ targetBitrate }`, `-bufsize ${ targetBitrate * 2 }`])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question: what happens if the input file has an average bitrate < targetBitrate?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nutomic won't the -b:v ${ targetBitrate } increase the bitrate of the video if it is lower than the targetBitrate? I feel like we should have just kept maxrate and bufsize to benefit from ffmpeg's automatic tuning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn you're right. I just checked and it works fine if you remove the -b:v parameter (tested with low bitrate, high bitrate and very high bitrate like in the unit test). I was having some bug during development, where FFmpeg generated videos with a too high bitrate for no reason, but that is gone now.

Can one of you just make a commit to remove that parameter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure :) no worries, it's my fault for not double checking that

@Nutomic
Copy link
Contributor Author

Nutomic commented Oct 8, 2018

Those are maximum values, so if possible, it transcodes with a lower bitrate. See my tests here.

Thanks for all the help figuring this stuff out, @rigelk @Chocobozzz :)

@rigelk
Copy link
Collaborator

rigelk commented Oct 8, 2018

Should be fixed in e1d7b98

@Chocobozzz
Copy link
Owner

Perfect thanks @Nutomic & @rigelk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants