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

QOI Image Support #91263

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

QOI Image Support #91263

wants to merge 1 commit into from

Conversation

basicer
Copy link
Contributor

@basicer basicer commented Apr 28, 2024

@DeeJayLSP

This comment was marked as resolved.

modules/qoi/qoi.cpp Outdated Show resolved Hide resolved
modules/qoi/resource_saver_qoi.cpp Outdated Show resolved Hide resolved
modules/qoi/resource_saver_qoi.cpp Outdated Show resolved Hide resolved
servers/movie_writer/movie_writer_pngwav.h Outdated Show resolved Hide resolved
modules/qoi/resource_saver_qoi.cpp Outdated Show resolved Hide resolved
modules/qoi/resource_saver_qoi.cpp Outdated Show resolved Hide resolved
modules/qoi/image_loader_qoi.cpp Outdated Show resolved Hide resolved
@basicer
Copy link
Contributor Author

basicer commented Apr 29, 2024

Thanks for the comments! I opened this as a draft as I knew I had a few more passes to go though before it was ready for review.

My biggest question is what should be done about MovieWriterPNGWAV? What I did makes a lot of sense, but then the name of the classes is a bit odd? Options as I see it are:

  1. Don't change anything.
  2. Rename this class to MovieWriterImgWAV
  3. Make a new class MovieWriterQOIWAV, with mostly duplicate code.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Apr 29, 2024

  1. Rename this class to MovieWriterImgWAV

Renaming classes is considered compatibility break, and should only be done between major versions.

  1. Make a new class MovieWriterQOIWAV, with mostly duplicate code.

I see some parallels with my QOA implementation. The original version was a module to import and play QOA files, but since QOA isn't a popular format to be converted to (unlike QOI, although not widely adopted as something like JPEG XL) I added a converter from WAV, with a lot of duplicated code.

But because I was never satisfied with that implementation, I ended up rewriting it inside the WAV code, as a compression mode rather than a separate format. No class renames and most importantly: no duplicated code.

IMHO, the best alternative would be 1. Making a new class and duplicating code has a considerable impact on binary size.

@Calinou
Copy link
Member

Calinou commented Apr 29, 2024

I suggest keeping the class name as-is, while adding a note in the class reference about why the class is named PNGWAV even if it supports QOI.

For instance, you can add this at the bottom of doc/classes/MovieWriterPNGWAV.xml's <description> tag:

[b]Note:[/b] For historical reasons, this class is named MovieWriterPNGWAV even though it also supports writing the QOI image format.

@basicer
Copy link
Contributor Author

basicer commented May 3, 2024

  1. Rename this class to MovieWriterImgWAV

Renaming classes is considered compatibility break, and should only be done between major versions.

  1. Make a new class MovieWriterQOIWAV, with mostly duplicate code.

I see some parallels with my QOA implementation. The original version was a module to import and play QOA files, but since QOA isn't a popular format to be converted to (unlike QOI, although not widely adopted as something like JPEG XL) I added a converter from WAV, with a lot of duplicated code.

But because I was never satisfied with that implementation, I ended up rewriting it inside the WAV code, as a compression mode rather than a separate format. No class renames and most importantly: no duplicated code.

IMHO, the best alternative would be 1. Making a new class and duplicating code has a considerable impact on binary size.

I noticed in the QOA patch, you added the header file to misc instead of making a separate folder. I suppose I should do that here too.

@basicer
Copy link
Contributor Author

basicer commented May 5, 2024

MovieWriterPNGWAV

Seems like that class isnt documented because its not reflected directly.

@basicer basicer marked this pull request as ready for review May 5, 2024 06:57
@basicer basicer requested review from a team as code owners May 5, 2024 06:57
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally on https://github.com/Calinou/godot-movie-maker-demo, it works as expected. Using an optimized editor build (optimize=speed lto=full).

PC specifications
  • CPU: Intel Core i9-13900K
  • GPU: NVIDIA GeForce RTX 4090
  • RAM: 64 GB (2×32 GB DDR5-5800 C30)
  • SSD: Solidigm P44 Pro 2 TB
  • OS: Linux (Fedora 39)

Results:

Format Time to record 30s Total file size (audio + video)
AVI MJPEG quality 75 27s 166.8 MiB
AVI MJPEG quality 100 51s 861.2 MiB
PNG 6m19s 1.7 GiB
QOI 22s 1.9 GB

The output file size is a bit larger, but it's greatly made up for by having much faster write speeds – faster than MJPEG at quality 75, while being lossless. This makes QOI the best format for Movie Maker output now.

Stripped binary size grows by 12 KB with this PR, which is pretty reasonable considering the big difference in Movie Maker usability this PR provides.

I took a quick look at the code and it looks good to me.

modules/qoi/image_loader_qoi.h Outdated Show resolved Hide resolved
@basicer basicer requested a review from akien-mga August 19, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support of QOI images
5 participants