xy-VSFilter icon indicating copy to clipboard operation
xy-VSFilter copied to clipboard

VSFilter: misc colour changes

Open TheOneric opened this issue 2 years ago • 5 comments

Not actually tested yet, but putting this up so it’s less likely to be forgotten about. I won't have time to work on and test this for a while; if someone else wants they’re welcome to pick this up.

@astiob: is the change in the 3rd commit what you had in mind here?

The idea behind officially supporting BGRA is that it could be used for colour-accurate rendering with CSRI by allowing the CSRI user to perform colour mangling like they’d do with libass. Without an alpha channel this isn't possible without degrading the video’s colours. SInce users can probe supported formats before rendering starts a fallback can be used and a warning emitted when no alpha channel and thus no colour accurate rendering is supported. I think aegisub already uses/expects the alpha channel to have ASS semantics where 0xFF is fully transparent and 0x00 fully opaque, but this should be rechecked and it might also be good to check what alpha semantics asa’s CSRI interface used/expected.

TheOneric avatar Jan 05 '23 01:01 TheOneric

@astiob: is the change in the 3rd commit what you had in mind here?

Probably along those lines. But the commit doesn’t actually change anything when the upstream says the matrix is BT.601.

astiob avatar Jan 22 '23 02:01 astiob

New version:

  • BT.601 is now actually preserved
  • SMPTE240M gets conflated with BT709 again for now, since ColorConvTable doesn’t appear to support SMPTE240M (this should ideally also be fixed) and in fact on current master BT2020 is also missing.
  • now all source files with at least one preexisting CRLF are converted to full CRLF instead of only the files touched in this PR.

TheOneric avatar Jan 22 '23 21:01 TheOneric

Copying Cyberbeing’s comment from another PR for future reference:

(Here's the commit dropping other pixel formats; unfortunately it doesn't elaborate on why exactly)

CSRI: Nothing change for it. But after checking the code, I believe it is not broken (by me). It should work right with ARGB output. The interface appears to accept outputting other color formats too, but I don't think the original implementation works. So I assume the users of CSRI never using any color format other ARGB. It uses an old set of interfaces too. (Performance issues TextSub has also happen to it.)

Looking through my old emails. The above is closest I could find to a stated reason @x-xy-y

TheOneric avatar Feb 05 '23 22:02 TheOneric

One thing to mention when making changing to the color handling code, is to be careful to ensure BT.601 continues to always be used on Auto when YCbCr Matrix: is not present in a script. That is a compatibility behavior for legacy scripts, since VSFilter 2.39 supported BT.601 only.

Cyberbeing avatar Feb 08 '23 06:02 Cyberbeing

btw, how about merging the first two or only the first commit(s) early?

The first one only changes whitespaces to make line endings consistent in each file and should thus be totally safe. Since it touches many files, leaving it unmerged makes future merge conflicts likely.

The second commit seems like a strict improvement to me, since before csri_request_fmt pretended like non-BGR formats were supported, but then during rendering crashed (with assert) or invoked UB and (tried to) return without actually rendering. Thus it seems to me like no one could use those formats anyway. With the commit applied csri_request_fmt already tells potential users that the format is not supported, so format probing to select the best choice is possible again.

TheOneric avatar Feb 08 '23 20:02 TheOneric