search

Jaded-Encoding-Thaumaturgy / vs-scale

VapourSynth (de)scaling functions
MIT License
10 stars 4 forks source link

Linear light scalers can cause color shift on the output clip #90

Closed shssoichiro closed 3 months ago

shssoichiro commented 4 months ago

Steps to reproduce:

clip = vstools.initialize_clip(
    clip,
    matrix=vstools.Matrix.SMPTE170M,
    primaries=vstools.Primaries.ST170M,
    transfer=vstools.Transfer.BT709,
)
clip = vsscale.SSIM().scale(clip, 848, 480)

In the clip that is returned, the Matrix will have been changed to BT.709, resulting in incorrect colors in the output. This occurred with a few combinations of colorimetry settings, though not all combinations.

I tested MergedFSRCNNX and noted that the issue is not present with that scaler.

shssoichiro commented 4 months ago

It seems this is more complicated than I originally thought.

If I resize to a size that is much smaller than the original, then even if I attempt to manually fix the matrix with e.g.:

matrix = vstools.Matrix.from_video(clip)
clip = vsscale.SSIM().scale(clip, 848, 480)
clip = clip.std.SetFrameProp(prop="_Matrix", intval=matrix)

The color shifting persists.

However, if I resize to a resolution that is normally in BT.709, e.g.:

clip = vsscale.SSIM().scale(clip, 1280, 720)

(assuming the original is 1920x1080) Then the matrix changes to BT.709, but there is no color shift, i.e. it is being fully converted to the new matrix properly. So this may be some sort of conversion issue when resizing below 1024x576.

Setsugennoao commented 4 months ago

Can you print the props after initialize clip? By default that will not override existing props unless you force it, so maybe the clip is marked bt709 in the metadata.

shssoichiro commented 4 months ago

I believe I've traced down the issue, and confirmed no color shift compared to Bicubic or Merged FSRCNNX.

See https://github.com/Jaded-Encoding-Thaumaturgy/vs-kernels/pull/24

Setsugennoao commented 4 months ago

Oh lol OFC, you're not specifying the curve

Setsugennoao commented 4 months ago

Just noticed it, i'm dumb

shssoichiro commented 4 months ago

Should I be? I've been assuming it's optional. Though if that's the case the change I posted might need an update to work correctly if the curve is set.

Edit: Actually it looks like _curve there is always set from Transfer.from_video(self.clip)

Setsugennoao commented 4 months ago

Yeah the function assumes you're specifying it. If we make this optional we gotta make it None and then retrieve it from the video without heuristic I'd rather it fail if it's not specified in curve and props

shssoichiro commented 4 months ago

I'm a bit lost on where should be specifying curve then. Right now LinearLightProcessing seems to assume the curve based on the Transfer of the clip, and then the matrix is assumed from that, which may be incorrect. It doesn't look like we can pass a curve param into the SSIM scaler (it throws an error), and if we could it doesn't look like it would do anything with it.

Setsugennoao commented 4 months ago

You totally can SSIM(curve=Transfer.xxx) Or SSIM.scale(clip, xx, xx, curve=Transfer.xxx)

shssoichiro commented 4 months ago

Both of those result in the following traceback (all plugins are on latest git version):

  File "/usr/lib/python3.12/site-packages/vsengine/vpy.py", line 219, in _execute
    runpy.run_path(str(script), module.__dict__, module.__name__)
  File "<frozen runpy>", line 286, in run_path
  File "<frozen runpy>", line 98, in _run_module_code
  File "/usr/lib/python3.12/site-packages/vspreview/core/vsenv.py", line 36, in _monkey_runpy_func
    glob_dict = orig_runpy_run_code(*args, **kwargs)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen runpy>", line 88, in _run_code
  File "/home/soichiro/Downloads/adv10bit.vpy", line 25, in <module>
    clip = vsscale.SSIM(curve=vstools.Transfer.BT709).scale(clip, 848, 480)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 180, in _wrapper
    return self.function(obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 314, in __call__
    return self.__get__(None, self)(*args, **kwargs)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 309, in _wrapper
    return this.function(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vskernels/kernels/complex.py", line 102, in func
    ll.linear = operation(ll.linear, width, height, shift, **kwargs)  # type: ignore
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vsscale/scale.py", line 127, in _linear_scale
    l1 = self.scaler.scale(clip, width, height, shift, **(kwargs | self.kwargs))
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 180, in _wrapper
    return self.function(obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 314, in __call__
    return self.__get__(None, self)(*args, **kwargs)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 309, in _wrapper
    return this.function(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vskernels/kernels/complex.py", line 249, in scale
    return super().scale(
           ^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 180, in _wrapper
    return self.function(obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 314, in __call__
    return self.__get__(None, self)(*args, **kwargs)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 309, in _wrapper
    return this.function(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vskernels/kernels/complex.py", line 97, in func
    return operation(clip, width, height, shift, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 180, in _wrapper
    return self.function(obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 314, in __call__
    return self.__get__(None, self)(*args, **kwargs)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 309, in _wrapper
    return this.function(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vskernels/kernels/complex.py", line 228, in scale
    clip = Scaler.scale(self, clip, width, height, shift, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 180, in _wrapper
    return self.function(obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 314, in __call__
    return self.__get__(None, self)(*args, **kwargs)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/stgpytools/types/utils.py", line 309, in _wrapper
    return this.function(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vskernels/kernels/abstract.py", line 230, in scale
    return self.scale_function(clip, **_norm_props_enums(self.get_scale_args(clip, shift, width, height, **kwargs)))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/vstools/utils/vs_proxy.py", line 210, in __call__
    return proxy_utils.get_vs_function(self)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "src/cython/vapoursynth.pyx", line 3069, in vapoursynth.Function.__call__
vapoursynth.Error: Bicubic: Function does not take argument(s) named curve
shssoichiro commented 4 months ago

Huh... I wonder if this is a Vapoursynth R68 error? I don't have an instance with R66 to test against at the moment.

Setsugennoao commented 4 months ago

Huh no you're right, I forgot i updated it Will have to check it out tomorrow

shssoichiro commented 4 months ago

Have you had an opportunity to investigate this?

As an aside, I think a fix that automates detection of the curve correctly would be better than requiring the user to specify, unless there's a solid reason to require user intervention here. This seems like it should be able to be automated e.g. this.