diff mbox series

[FFmpeg-devel,2/3] avcodec/ass: accurately preserve colours

Message ID 20221113190704.15518-3-oneric@oneric.de
State New
Headers show
Series Some small ASS conversion fixes | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Oneric Nov. 13, 2022, 7:07 p.m. UTC 
Colour values used in ASS files without a "YCbCr Matrix" header set to
"None" are subject to colour mangling, due to how ASS was historically
conceived. A more in-depth description can be found in the documetation
inside libass' public ass_types.h header. The important part is, if this
header is not set to "None", the final output colours can deviate from
the literal value specified in the file. When converting from non-ASS
formats we do not want any colour shift to happen, so let's set the
appropiate header.

NB: ffmpeg's subtitle filter, does not follow libass' documentation
regarding colour mangling, thus hiding the bug. Anything based on
VSFilter, XySubFilter or e.g. mpv do and might show the issue.
(Of course native ASS subs, which _do_ rely on colour mangling won't
 work properly with the subtitle filter, but this can be fixed another
 time)
---
Link to libass’ docs regarding colour mangling:
  https://github.com/libass/libass/blob/3df19c2e809b16c9cf7c925fa3bb573e2e6f4fdd/libass/ass_types.h#L152-L232
---
 libavcodec/ass.c                 |   1 +
 tests/ref/fate/sub-aqtitle       | Bin 3213 -> 3233 bytes
 tests/ref/fate/sub-cc            | Bin 839 -> 859 bytes
 tests/ref/fate/sub-cc-realtime   | Bin 1524 -> 1544 bytes
 tests/ref/fate/sub-cc-scte20     | Bin 945 -> 965 bytes
 tests/ref/fate/sub-charenc       | Bin 6008 -> 6028 bytes
 tests/ref/fate/sub-jacosub       | Bin 1783 -> 1803 bytes
 tests/ref/fate/sub-microdvd      | Bin 1375 -> 1395 bytes
 tests/ref/fate/sub-movtext       | Bin 783 -> 803 bytes
 tests/ref/fate/sub-mpl2          | Bin 870 -> 890 bytes
 tests/ref/fate/sub-mpsub         | Bin 2517 -> 2537 bytes
 tests/ref/fate/sub-mpsub-frames  | Bin 736 -> 756 bytes
 tests/ref/fate/sub-pjs           | Bin 860 -> 880 bytes
 tests/ref/fate/sub-realtext      | Bin 939 -> 959 bytes
 tests/ref/fate/sub-sami          | Bin 1981 -> 2001 bytes
 tests/ref/fate/sub-sami2         | Bin 9970 -> 9990 bytes
 tests/ref/fate/sub-scc           |   1 +
 tests/ref/fate/sub-srt           | Bin 6301 -> 6321 bytes
 tests/ref/fate/sub-srt-badsyntax | Bin 1561 -> 1581 bytes
 tests/ref/fate/sub-stl           | Bin 2241 -> 2261 bytes
 tests/ref/fate/sub-subviewer     | Bin 780 -> 800 bytes
 tests/ref/fate/sub-subviewer1    | Bin 1494 -> 1514 bytes
 tests/ref/fate/sub-vplayer       | Bin 742 -> 762 bytes
 tests/ref/fate/sub-webvtt        | Bin 1985 -> 2005 bytes
 tests/ref/fate/sub-webvtt2       | Bin 1648 -> 1668 bytes
 25 files changed, 2 insertions(+)

index 13f393cc86b9f797be24f37a07aafc7272c22e59..516d26af9ab09613f939d004826e44c80b3b1054 100644
GIT binary patch
delta 29
kcmX@kcAITNgHWV%l5>%QZ(>PNW`&i4Uw&Td#=4VC0HTfx{Qv*}

delta 10
Rcmcc3cARZO!^SDcnE)Fg1kC^d

index be28084887a9b7fb80ae4ff3369cf7192a978919..a97d29f70ba1d3984887eacb6e833abb8a406902 100644
GIT binary patch
delta 29
kcmdnUew2MegHWV%l5>%QZ(>PNW`&i4Uw&Td#yVqW0G!DRJ^%m!

delta 10
RcmX@gzL9-G!^SCw%m5g91P%ZI

index b574dda54dfdee55a07f13c808ee3baa6e100798..32086d9365399ae3a968f9f61c6e3fa83a35ff65 100644
GIT binary patch
delta 29
kcmey)+s!wjK`7EW$+<|uH?gEBv%*TjFF!AJV_hX10HB5npa1{>

delta 10
RcmeC?`_4O|VdIo?HUJr21aJTV

index 6e2d2e35dbd94d6dded683b59436645805723372..973e9f16457906c4bf9bd95d7053b2c4e405e68a 100644
GIT binary patch
delta 29
kcmeBYTg*10K`7EW$+<|uH?gEBv%*TjFF!AJW8Fk10F~YfRR910

delta 10
RcmZ3?*3UMfVdIp3CIA+|1SJ3f

index 1a8e757585a91121cb27f360b352fecc34340d8b..4c3d37fa42206d262e221b1d0b81312289329143 100644
GIT binary patch
delta 29
kcmcaA{8D&AgHWV%l5>%QZ(>PNW`&i4Uw&Td#=3A$0H=Qn$N&HU

delta 10
RcmaDUd{uZt!^SD0oB$jT1egE-

index 378190a3eb0aa64fd983bc10dd1caa512ba6c92e..a32720514305771ae40b9182a9c469e098b6b1aa 100644
GIT binary patch
delta 29
kcmcb^_JM6egHWV%l5>%QZ(>PNW`&i4Uw&Td#=3h<0H;t3JOBUy

delta 10
Rcmeysc86_3!^SChm;fBa1q=WH

index dbd1cc310d670d6a2874af6d669b8e0d43c3b7cd..3f194bdd0253eff13af8df85876f656e9fca8658 100644
GIT binary patch
delta 29
kcmdnXf02JegHWV%l5>%QZ(>PNW`&i4Uw&Td#yWd;0HHMsb^rhX

delta 10
Rcmcb}zn6bP!^SDL>;M_<1V#V=

index 62cbf6fa4a..ff667eb4ec 100644
index 61f472a84b9aac22954215b60df7acdecd05fddc..27bdd48c404370d047697c06b710e44aaa371b74 100644
GIT binary patch
delta 29
kcmbQqvzBK<gHWV%l5>%QZ(>PNW`&i4Uw&Td#=6<80GVkDf&c&j

delta 10
RcmZ3>Gm~dR!^SBySpgU81X2J1

index c68a6442be223b8c98cef6e3a67b5a176cd434c9..e910e154ba6203eca72f994d2f3cfd40d50f5ebc 100644
GIT binary patch
delta 29
kcmeBSTfjD<K`7EW$+<|uH?gEBv%*TjFF!AJV_hE;0F^KbOaK4?

delta 10
RcmZ3$*26ZTVdIn@CIA+M1RMYW

index d83db9e09fce65590dd76ed3fac7fa6cdf146f60..ef9cb2d9e5eec004f9d1ae70112bf4fee6a0bbfb 100644
GIT binary patch
delta 29
kcmaFH`ipf!gHWV%l5>%QZ(>PNW`&i4Uw&Td#<~n90H|IH*Z=?k

delta 10
Rcmeyx`iyl#!^SCTOaL5w1gHQ2

index 357b8178ea1cf224ad47dcf78b24f1948ece6665..56afed8944071d465eba2a8c0308af88863e54b6 100644
GIT binary patch
delta 29
kcmeys)51HUK`7EW$+<|uH?gEBv%*TjFF!AJW8EiK0H3)F2mk;8

delta 10
RcmZqS{lGJ!VdIn!tN<Cn1lRxo

Comments

Nicolas George Nov. 13, 2022, 7:12 p.m. UTC | #1 
Oneric (12022-11-13):
>  tests/ref/fate/sub-aqtitle       | Bin 3213 -> 3233 bytes
>  tests/ref/fate/sub-cc            | Bin 839 -> 859 bytes
>  tests/ref/fate/sub-cc-realtime   | Bin 1524 -> 1544 bytes
>  tests/ref/fate/sub-cc-scte20     | Bin 945 -> 965 bytes
>  tests/ref/fate/sub-charenc       | Bin 6008 -> 6028 bytes
>  tests/ref/fate/sub-jacosub       | Bin 1783 -> 1803 bytes
>  tests/ref/fate/sub-microdvd      | Bin 1375 -> 1395 bytes
>  tests/ref/fate/sub-movtext       | Bin 783 -> 803 bytes
>  tests/ref/fate/sub-mpl2          | Bin 870 -> 890 bytes
>  tests/ref/fate/sub-mpsub         | Bin 2517 -> 2537 bytes
>  tests/ref/fate/sub-mpsub-frames  | Bin 736 -> 756 bytes
>  tests/ref/fate/sub-pjs           | Bin 860 -> 880 bytes
>  tests/ref/fate/sub-realtext      | Bin 939 -> 959 bytes
>  tests/ref/fate/sub-sami          | Bin 1981 -> 2001 bytes
>  tests/ref/fate/sub-sami2         | Bin 9970 -> 9990 bytes
>  tests/ref/fate/sub-srt           | Bin 6301 -> 6321 bytes
>  tests/ref/fate/sub-srt-badsyntax | Bin 1561 -> 1581 bytes
>  tests/ref/fate/sub-stl           | Bin 2241 -> 2261 bytes
>  tests/ref/fate/sub-subviewer     | Bin 780 -> 800 bytes
>  tests/ref/fate/sub-subviewer1    | Bin 1494 -> 1514 bytes
>  tests/ref/fate/sub-vplayer       | Bin 742 -> 762 bytes
>  tests/ref/fate/sub-webvtt        | Bin 1985 -> 2005 bytes
>  tests/ref/fate/sub-webvtt2       | Bin 1648 -> 1668 bytes

These are text files. Please fix this so that we can review the patch.

Regards,
Oneric Nov. 13, 2022, 7:21 p.m. UTC | #2 
Thanks for taking a look!

On Sun, Nov 13, 2022 at 20:12:46 +0100, Nicolas George wrote:
> Oneric (12022-11-13):
> >  [...]
> >  tests/ref/fate/sub-webvtt2       | Bin 1648 -> 1668 bytes
> 
> These are text files. Please fix this so that we can review the patch.

As explained in the cover letter, those files are intentionally using CRLF
line endings and patchwork bugs (or at least used to bug) out if it
receives patches with CRLF line endings. The last few times this always
caused some debate, so I used binary diffs to workaround it this time.
It just adds the "YCbCr Matrix: None" line in every file and changes
the Encoding field to "1" instead of "0".

If you prefer and believe the patchwork bugs won’t cause confusion,
I can of course immediately send a v2 with plain text diffs.

Regards

Oneric
Soft Works Nov. 13, 2022, 7:39 p.m. UTC | #3 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Oneric
> Sent: Sunday, November 13, 2022 8:22 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/3] avcodec/ass: accurately
> preserve colours
> 
> Thanks for taking a look!
> 
> On Sun, Nov 13, 2022 at 20:12:46 +0100, Nicolas George wrote:
> > Oneric (12022-11-13):
> > >  [...]
> > >  tests/ref/fate/sub-webvtt2       | Bin 1648 -> 1668 bytes
> >
> > These are text files. Please fix this so that we can review the
> patch.
> 
> As explained in the cover letter, those files are intentionally using
> CRLF
> line endings and patchwork bugs (or at least used to bug) out if it
> receives patches with CRLF line endings. The last few times this
> always
> caused some debate, so I used binary diffs to workaround it this
> time.
> It just adds the "YCbCr Matrix: None" line in every file and changes
> the Encoding field to "1" instead of "0".
> 
> If you prefer and believe the patchwork bugs won’t cause confusion,
> I can of course immediately send a v2 with plain text diffs.

IMO, this is not necessary, because those might be better visible
but nobody would be able to properly apply them.

I would even go one step further and specify those files which
are causing trouble in a .gitattributes file, because this will 
allow this (creating patches for those files as binary) to be 
handled automatically by git.

The .gitattributes solution worked successfully as tested here:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/BN0P223MB0358900908A4B2A6A37C3296BA769@BN0P223MB0358.NAMP223.PROD.OUTLOOK.COM/


PS: Thanks for your patch, will look at it later

softworkz
diff mbox series

Patch

diff --git a/libavcodec/ass.c b/libavcodec/ass.c
index fdf55f36ca..d2ea4c62c3 100644
--- a/libavcodec/ass.c
+++ b/libavcodec/ass.c
@@ -41,6 +41,7 @@  int ff_ass_subtitle_header_full(AVCodecContext *avctx,
              "PlayResX: %d\r\n"
              "PlayResY: %d\r\n"
              "ScaledBorderAndShadow: yes\r\n"
+             "YCbCr Matrix: None\r\n"
              "\r\n"
              "[V4+ Styles]\r\n"
 
diff --git a/tests/ref/fate/sub-aqtitle b/tests/ref/fate/sub-aqtitle
index af0c06d7c2809e2bfe8859f2f53cafd3c1808889..ae5edcd9abe039d33dd3e2da496fd991dcbe8490 100644
GIT binary patch
delta 29
kcmeB`TqrrAK`7EW$+<|uH?gEBv%*TjFF!AJV;w&a0GM_O{{R30

delta 10
RcmZ1|*(*7rVdE5D9sm~71J3{e

diff --git a/tests/ref/fate/sub-cc b/tests/ref/fate/sub-cc
diff --git a/tests/ref/fate/sub-cc-realtime b/tests/ref/fate/sub-cc-realtime
index 169361f540e4dff43a132df1111395e55a19ff73..98dfef55019719911d6c5d9faa0c057cc324f227 100644
GIT binary patch
delta 29
kcmeyu-N7@VK`7EW$+<|uH?gEBv%*TjFF!AJV_g|50H13Lk^lez

delta 10
RcmeC+`NBP+VdIn%Rsb0Q1Y!UH

diff --git a/tests/ref/fate/sub-cc-scte20 b/tests/ref/fate/sub-cc-scte20
diff --git a/tests/ref/fate/sub-charenc b/tests/ref/fate/sub-charenc
index 4efacb073d14b43002b58ab0b4aa55e9d34de029..339137ae0b5485c4c954f859316cf8b413b01505 100644
GIT binary patch
delta 29
kcmeyN*P}n7K`7EW$+<|uH?gEBv%*TjFF!AJW8E(?0I0?bbpQYW

delta 10
RcmeCt|DiXbVdIn^VgMYG1w{Y=

diff --git a/tests/ref/fate/sub-jacosub b/tests/ref/fate/sub-jacosub
diff --git a/tests/ref/fate/sub-microdvd b/tests/ref/fate/sub-microdvd
index 4ddb254c698245958b12e3ab0e988ef6359592a8..11440c28243f00cf99f07ff91ab2eb1c23dc2cc1 100644
GIT binary patch
delta 29
kcmcc5^_gozgHWV%l5>%QZ(>PNW`&i4Uw&Td#=3_r0I1jtPXGV_

delta 10
Rcmey&b)RcO!^SE1SpXeE1s?za

diff --git a/tests/ref/fate/sub-movtext b/tests/ref/fate/sub-movtext
diff --git a/tests/ref/fate/sub-mpl2 b/tests/ref/fate/sub-mpl2
index f78cf6849530c0015fe58a6cc7cb1584d6c4c4ce..d740fbc365460187e6b4edcf3faa00bdb5638196 100644
GIT binary patch
delta 29
kcmaFH_KR&ogHWV%l5>%QZ(>PNW`&i4Uw&Td#<~|w0I8G<S^xk5

delta 10
Rcmeyx_Ka;p!^SDkm;fD&1u6gl

diff --git a/tests/ref/fate/sub-mpsub b/tests/ref/fate/sub-mpsub
diff --git a/tests/ref/fate/sub-mpsub-frames b/tests/ref/fate/sub-mpsub-frames
index abd52ad277212d34a93b31b2114345e91f3ea1dc..4f69e68948d66530152459e31f60883b528532f5 100644
GIT binary patch
delta 29
kcmaFB`h|5ugHWV%l5>%QZ(>PNW`&i4Uw&Td#=0aX0H*>9#sB~S

delta 10
Rcmeyu`hayp!^SBIOaL4N1eO2*

diff --git a/tests/ref/fate/sub-pjs b/tests/ref/fate/sub-pjs
diff --git a/tests/ref/fate/sub-realtext b/tests/ref/fate/sub-realtext
index 04b1664f8968cc550ce6f695f3bd68544ce3c0cb..d80db64dafcc51c0536b968eb0416444bbdb2a21 100644
GIT binary patch
delta 29
kcmZ3@zMp+UgHWV%l5>%QZ(>PNW`&i4Uw&Td#yVYQ0Gn+JEC2ui

delta 10
RcmdnbzM6eP!^SDv%m5ex1N;C0

diff --git a/tests/ref/fate/sub-sami b/tests/ref/fate/sub-sami
diff --git a/tests/ref/fate/sub-sami2 b/tests/ref/fate/sub-sami2
index dbec842d2b0181ebc0a7e8eefbd2991273255efb..bab07e4c0709cb420c1c248782e95bbf015b844f 100644
GIT binary patch
delta 29
kcmez5+vYc+K`7EW$+<|uH?gEBv%*TjFF!AJV_k_F0IdWIF#rGn

delta 10
RcmZqk`{X;JVdIn{H2@tM1pxp6

diff --git a/tests/ref/fate/sub-scc b/tests/ref/fate/sub-scc
--- a/tests/ref/fate/sub-scc
+++ b/tests/ref/fate/sub-scc
@@ -4,6 +4,7 @@  ScriptType: v4.00+
 PlayResX: 384
 PlayResY: 288
 ScaledBorderAndShadow: yes
+YCbCr Matrix: None
 
 [V4+ Styles]
 Format: Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding
diff --git a/tests/ref/fate/sub-srt b/tests/ref/fate/sub-srt
index b4eed235ce01f1f85bb26fd49fcdc123467c4740..a6ed4f31df47709db51272dff3bd889cb9ffc317 100644
GIT binary patch
delta 29
kcmbPhxY2MzgHWV%l5>%QZ(>PNW`&i4Uw&Td#yWWk0HHDpYXATM

delta 10
RcmdmJIM;AO!^SDH5&#*V1Umo#

diff --git a/tests/ref/fate/sub-srt-badsyntax b/tests/ref/fate/sub-srt-badsyntax
diff --git a/tests/ref/fate/sub-stl b/tests/ref/fate/sub-stl
index 0f326c21730c959e9428cd96a5eca21014b5c888..3e847a68beb5d6cfca08ea4612027d72eda9958a 100644
GIT binary patch
delta 29
kcmX>ocvWyhgHWV%l5>%QZ(>PNW`&i4Uw&Td#yV#X0HTTthX4Qo

delta 10
RcmcaAcu;Uc!^SC&8~_?#1Xln6

diff --git a/tests/ref/fate/sub-subviewer b/tests/ref/fate/sub-subviewer
diff --git a/tests/ref/fate/sub-subviewer1 b/tests/ref/fate/sub-subviewer1
index e88729ad5ec990202f9d23f96288a36eb5a845bb..2d253288e1db9f2c2061512b6c85ebaefc5c51b0 100644
GIT binary patch
delta 29
kcmcb{{fc`+gHWV%l5>%QZ(>PNW`&i4Uw&Td#<~br0Hy^Bw*UYD

delta 10
RcmaFGeT{oU!^SCLtN<II1cv|s

diff --git a/tests/ref/fate/sub-vplayer b/tests/ref/fate/sub-vplayer
diff --git a/tests/ref/fate/sub-webvtt b/tests/ref/fate/sub-webvtt
index dea535b19b20e00c4249950af9b511a2e1a617f9..091cfb5d3f73c97c82146d67bb0447f3fd8bebe7 100644
GIT binary patch
delta 29
kcmX@ef0chigHWV%l5>%QZ(>PNW`&i4Uw&Td#yV$q0HPfVf&c&j

delta 10
Rcmcc0e~^Dd!^SC&>;M`)1X2J1