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

Save in .png instead of .jpg #44

Merged
merged 3 commits into from
Sep 21, 2022
Merged

Save in .png instead of .jpg #44

merged 3 commits into from
Sep 21, 2022

Conversation

0x1355
Copy link
Contributor

@0x1355 0x1355 commented Sep 21, 2022

Hey @nateraw,

I was waiting for the resuming feature to merge. Didn't want to muddle the waters.

I ran some comparisons on saving in .png versus .jpg. In general, png format has smoother color gradients and less artifacts. This is especially obvious after upsampling.

See the example below. Both generated at 960x512 and then upsampled 4x. Screeshot at 100% zoom. png on the left and jpg on the right. Click to view full-size.

image

The only drawback of png is larger size. At default compression levels, png is 8-9mb at 4k resolution while jpg is at 500-750kb. But considering the compute and time spent on generating these images, I feel we should prioritize image quality over file size.

I also tried different png compression levels. The visual difference is minimal so I went with the default.

What do you think?

.png format has less artifacts than .jpg. This is especially obvious after upsampling.
@nateraw
Copy link
Owner

nateraw commented Sep 21, 2022

Nice!! Meant to look into this as well, but haven't had bandwidth. I think I'll go ahead an merge this after #45 .

Would be nice to parametrize this too, but might be unnecessary (so we can do it in future if folks ask)

@0x1355
Copy link
Contributor Author

0x1355 commented Sep 21, 2022

Thanks! Really looking forward to #45 !

@nateraw
Copy link
Owner

nateraw commented Sep 21, 2022

Merged! I can fix up this PR if you'd like

@0x1355
Copy link
Contributor Author

0x1355 commented Sep 21, 2022

I think I fixed it but can you take a look just to be sure?

@nateraw
Copy link
Owner

nateraw commented Sep 21, 2022

only consideration I have on .jpg is that you wont be able to easily resume those from runs generated before this change hits.

@0x1355
Copy link
Contributor Author

0x1355 commented Sep 21, 2022

Huh... As long as they don't pull the code or update the package in-between it would work. Anyway we can 'warn' the users?

@nateraw
Copy link
Owner

nateraw commented Sep 21, 2022

Yea...and package wasn't released at all since we added resume support so probably a non-issue for the most part. Will run this real quick as sanity check, then merge. If I feel adventurous, I'll parametrize it so folks dont run into issues w/ old runs

(CC @codefaux - this may effect you if resuming .jpg runs, so giving you a heads up)

@0x1355
Copy link
Contributor Author

0x1355 commented Sep 21, 2022

Feeling adventurous?

Forgive my shameless self-promotion but it is just the perfect moment.

@nateraw
Copy link
Owner

nateraw commented Sep 21, 2022

Video is so cool! Excellent applied use case there. Thank you for sharing!!

Merging this as is for now! found one small spot where I needed to add .png, other than that I left as is.

@nateraw nateraw merged commit cdbda82 into nateraw:main Sep 21, 2022
@0x1355
Copy link
Contributor Author

0x1355 commented Sep 21, 2022

Thank you ser!

@0x1355 0x1355 deleted the save-in-png branch September 21, 2022 05:22
@codefaux
Copy link
Contributor

Thanks for the heads up. I'll keep a stable local copy until this run is done. 46% after several days, still 'only' on a 1080ti lol.

Can't see any reason to argue for jpg to remain as an option, but easy resume support for previous runs would be check if first frame exists in either format when resuming, use that for this run but default to png for me.

@nateraw
Copy link
Owner

nateraw commented Sep 21, 2022

@codefaux I'll see if I can add that real quick

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.

None yet

3 participants