diff mbox series

[FFmpeg-devel,1/5] avfilter/vf_ciescope: Handle black as very dark neutral gray

Message ID 20210606134757.18919-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/5] avfilter/vf_ciescope: Handle black as very dark neutral gray
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer June 6, 2021, 1:47 p.m. UTC
Fixes: floating point division by 0
Fixes: undefined behavior in handling NaN
Fixes: Ticket 8268

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavfilter/vf_ciescope.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Michael Niedermayer June 6, 2021, 3:34 p.m. UTC | #1
On Sun, Jun 06, 2021 at 04:50:41PM +0300, Valerii Zapodovnikov wrote:
> I am sorry, what???

If that is intended as a review comment, please compose
a english sentance which represents what you are trying to convey.
That way others know what you mean.

The goal of review is to improve code

Anyway to elaborate on the rather trivial change
the code before the change has undefined behavior, that is a bug
in C per definition of what a bug is pretty much.
the change tried to replace the used value with one thats close
and not undefined.
we could skip black pixels or also use x=y=0 but later is
discontinous on all pathes with non negative color leading to black
so it felt a bit strange. But probably x=y=0 is a more strictly correct
choice, so ill change it to that.

thx

[...]
Valerii Zapodovnikov June 6, 2021, 3:47 p.m. UTC | #2
вс, 6 июн. 2021 г., 18:35 Michael Niedermayer <michael@niedermayer.cc>:

> On Sun, Jun 06, 2021 at 04:50:41PM +0300, Valerii Zapodovnikov wrote:
> > I am sorry, what???
>
> If that is intended as a review comment, please compose
> a english sentance which represents what you are trying to convey.
> That way others know what you mean.
>
> The goal of review is to improve code
>
> Anyway to elaborate on the rather trivial change
> the code before the change has undefined behavior, that is a bug
> in C per definition of what a bug is pretty much.
> the change tried to replace the used value with one thats close
> and not undefined.
> we could skip black pixels or also use x=y=0 but later is
> discontinous on all pathes with non negative color leading to black
> so it felt a bit strange. But probably x=y=0 is a more strictly correct
> choice, so ill change it to that.
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Many that live deserve death. And some that die deserve life. Can you give
> it to them? Then do not be too eager to deal out death in judgement. For
> even the very wise cannot see all ends. -- Gandalf
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

Nice, please change to that. And actually first link in google says
https://stackoverflow.com/questions/42926763/ that it was defined in Annex
F of ISO/IEC 9899 and gcc can do it, while clang cannot. I am not sure how
one catches such NAN exceptions though.

Moreover clang also wants that
https://bugs.llvm.org/show_bug.cgi?id=19535#c13 and
https://bugs.llvm.org/show_bug.cgi?id=17005, the last one even says and I
quote: "The most visible issue caused by this is apparently that using
-fsanitize=undefined on programs that make use of floating point division
by 0.0 (which is perfectly legal udner IEEE floats) will print "runtime
error: division by zero".

(Also see: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=720935)

It's also scary to think that your program might run slightly incorrectly
in corner cases under clang, while it's correct under gcc (at least with
-std=c99, gcc intentionally breaks floating point semantics with
-std=gnu99)."

Another bug in compiler, nothing to worry about IMHO.

>
diff mbox series

Patch

diff --git a/libavfilter/vf_ciescope.c b/libavfilter/vf_ciescope.c
index b3b906f517..f8fe554c06 100644
--- a/libavfilter/vf_ciescope.c
+++ b/libavfilter/vf_ciescope.c
@@ -844,6 +844,9 @@  rgb_to_xy(double rc,
 {
     double sum;
 
+    if (rc == 0 && gc == 0 && bc == 0)
+        rc = gc = bc = 0.1/65536;
+
     *x = m[0][0] * rc + m[0][1] * gc + m[0][2] * bc;
     *y = m[1][0] * rc + m[1][1] * gc + m[1][2] * bc;
     *z = m[2][0] * rc + m[2][1] * gc + m[2][2] * bc;