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

vo_gpu_next: raise LUT file max size and report an error if exceeded #15146

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

MoSal
Copy link
Contributor

@MoSal MoSal commented Oct 21, 2024

First of all, lutdata was passed to pl_lut_parse_cube() even if
it was empty. This caused a bogus error message to be printed by
the LUT parsing code in libplacebo.

"[vo/gpu-next/libplacebo] Missing LUT size specification?"

One of the reasons why lutdata can be empty is if the LUT file size
exceeds the max_size limit passed to stream_read_file().

So in this commit, lutdata is checked first before getting passed to
libplacebo, and a non-bogus error message is printed if it's empty.

And secondly, the max size allowed is raised from 100MB to 600MB, which
should be more than enough for opening 256*256*256 LUT files. This LUT
size (256) covers the RGB 24bit color range, and is helpful for testing
different 3D interpolation algorithms.

video/out/vo_gpu_next.c Outdated Show resolved Hide resolved
video/out/vo_gpu_next.c Outdated Show resolved Hide resolved
video/out/vo_gpu_next.c Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Oct 21, 2024

Download the artifacts for this pull request:

Windows
macOS

 First of all, `lutdata` was passed to `pl_lut_parse_cube()` even if
 it was empty. This caused a bogus error message to be printed by
 the LUT parsing code in libplacebo.

 "[vo/gpu-next/libplacebo] Missing LUT size specification?"

 One of the reasons why `lutdata` can be empty is if the LUT file size
 exceeds the `max_size` limit passed to `stream_read_file()`.

 So in this commit, `lutdata` is checked first before getting passed to
 libplacebo, and a non-bogus error message is printed if it's empty.

 And secondly, the max size allowed is raised from the quite limiting
 value of 100MB to 1.5GiB. The new max matches the limit in LUT cache
 which is used to support up to 512x512x512 LUTs.

Signed-off-by: Mohammad AlSaleh <[email protected]>
@MoSal
Copy link
Contributor Author

MoSal commented Oct 22, 2024

Updated the commit message alongside the suggested changes.

@kasper93 kasper93 merged commit d027e02 into mpv-player:master Oct 22, 2024
24 checks passed
@MoSal
Copy link
Contributor Author

MoSal commented Oct 24, 2024

It's probably not important, but I was wondering where the 1.5GiB number came from. Then I figured it's (512^3)*12, where 12 is the size of the triple f32s. But that has nothing to do with the LUT text file size. Such LUT files can actually size ~4GiB (using ryu float formatting). So the comment about matching the cache limit is bogus.

@kasper93
Copy link
Contributor

Are they really? Well, you can bump it to whatever you want. It is not like this limit has significance, it is just a limit to not try to load unexpectedly large files.

@MoSal
Copy link
Contributor Author

MoSal commented Oct 24, 2024

Are they really?

An identity LUT using your favorite %g format specifier:

#include <stdio.h>

int main() {
  int size = 512;
  float step = 1.0f / (size - 1);
  printf("# Identity 3D LUT using %%g format specifier\n");
  printf("Title identity\n");
  printf("LUT_3D_SIZE %d\n\n", size);
  for (int b_c=0; b_c < size; b_c++) {
    for (int g_c=0; g_c < size; g_c++) {
      for (int r_c=0; r_c < size; r_c++) {
        printf("%g %g %g\n", step*r_c, step*g_c, step*b_c);
      }
    }
  }
}

As you can see, the size is 3448.50MiB. Using %.6f, and the size would be 3456.00.MiB. Rust's ryu implementation gives me a 4068.00MiB file, but using the original C implementation's f2s() will give you a 4790.25MiB file.

Note that a non-identity LUT may not have as many exact 0.0s and 1.0s.

So, I'm not sure what a "good limit" would be here. Is something like 6GiB a helpful limit really?

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.

2 participants