Re: [FFmpeg-devel] [PATCH 2/3] avcodec/ass: accurately preserve colours
> -Original Message- > From: ffmpeg-devel On Behalf Of > Oneric > Sent: Sunday, November 13, 2022 8:22 PM > To: FFmpeg development discussions and patches de...@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/bn0p223mb0358900908a4b2a6a37c3296ba...@bn0p223mb0358.namp223.prod.outlook.com/ PS: Thanks for your patch, will look at it later softworkz ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
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. Regards Oneric signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] avcodec/ass: accurately preserve colours
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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/3] avcodec/ass: accurately preserve colours
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(+)
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_O{{R30
delta 10
RcmZ1|*(*7rVdE5D9sm~71J3{e
diff --git a/tests/ref/fate/sub-cc b/tests/ref/fate/sub-cc
index
13f393cc86b9f797be24f37a07aafc7272c22e59..516d26af9ab09613f939d004826e44c80b3b1054
100644
GIT binary patch
delta 29
kcmX@kcAITNgHWV%l5>%QZ(>PNW`#=4VC0HTfx{Qv*}
delta 10
Rcmcc3cARZO!^SDcnE)Fg1kC^d
diff --git a/tests/ref/fate/sub-cc-realtime b/tests/ref/fate/sub-cc-realtime
index
169361f540e4dff43a132df395e55a19ff73..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
index
be28084887a9b7fb80ae4ff3369cf7192a978919..a97d29f70ba1d3984887eacb6e833abb8a406902
100644
GIT binary patch
delta 29
kcmdnUew2MegHWV%l5>%QZ(>PNW`#yVqW0G!DRJ^%m!
delta 10
RcmX@gzL9-G!^SCw%m5g91P%ZI
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
index
b574dda54dfdee55a07f13c808ee3baa6e100798..32086d9365399ae3a968f9f61c6e3fa83a35ff65
100644
GIT binary patch
delta 29
kcmey)+s!wjK`7EW$+<|uH?gEBv%*TjFF!AJV_hX10HB5npa1{>
delta 10
RcmeC?`_4O|VdIo?HUJr21aJTV
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`#=3_r0I1jtPXGV_
delta 10
Rcmey)RcO!^SE1SpXeE1s?za
diff --git a/tests/ref/fate/sub-movtext b/tests/ref/fate/sub-movtext
index
