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

Implement QP 0 lossless x264 encoding #2055

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Quackdoc
Copy link
Contributor

@Quackdoc Quackdoc commented Oct 8, 2022

Adds a lossless box to x264/5 encoding. do this by enforcing QP 0, since crf 0 is not true lossless for 10bit+ content. it's also nice to have a specific toggle for it for UI anyways.

@itsmattkc
Copy link
Contributor

Is there sufficient demand for lossless H.264? Keep in mind the goal is not to support a lot of codecs, it's to support a few codecs very well.

@Quackdoc
Copy link
Contributor Author

Quackdoc commented Oct 9, 2022

Is there sufficient demand for lossless H.264? Keep in mind the goal is not to support a lot of codecs, it's to support a few codecs very well.

I cannot comment for other people, but at least within my bubble and really the only valid use of it IMO, is for exporting to be imported into a dedicated encoder. (preservation based storage would be better suited for something like ffv1).

the files are significantly smaller, faster export, and faster decode when using x264 lossless compared to something like PNG or even ffv1. though like I said, I certainly cannot comment on other folks workflows. as for lossless h264, as seen in the PR, it's simply a matter of telling the encoder to do qp 0 instead of crf 0.

so im not sure if their is sufficient demand for it, but I did try to make it as non intrusive as possible while still being fairly user friendly.

@ThomasWilshaw
Copy link
Collaborator

Olive is about to undergo some largish code refactoring and I wanted to check on the status of old PRs before we do that. Do you plan to update/continue with the PR? Thanks

@Quackdoc
Copy link
Contributor Author

I plan on keeping this up to date at least

@itsmattkc
Copy link
Contributor

This looks good to me, though the explanation string "For true lossless, verify the pixel format before exporting" could use some further elaboration. It's not immediately clear to me (and presumably other users) what this is warning me to avoid. My guess is it's warning that a conversion from, say, RGB to 4:2:0 is lossy despite the lossless compression, though that's an assumption I've had to make based on prior knowledge. This should probably be clearer if so.

@Quackdoc
Copy link
Contributor Author

woops, forgot about this until I was asked about this until now by a 3rd party, how would be a better way to put it?
how about

Verify Pixel format and Range in Advanced menu are set properly prior to exporting.

the goal is just to warn people to set the appropriate pixel format and range prior to encoding. the issue is I don't really expect there to be an easy to judge this, and I'm not sure the average user would be able to figure out which to use,

we could recommend yuv444p10le as that will typically be a safe bet, however im not sure about range. since both limited range and full range are quite popular now. and messing this up can have some really nasty consequences depending on how it happens.

that being said, I dont really expect people who don't know what they are doing to export lossless. in that case we could simply use

Lossless is only recommended if you explicitly need it, Verify Pixel Format and Range in Advanced menu before exporting.

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