Re: [FFmpeg-devel] [RFC] Value analysis with Frama-C's Eva plugin (and an UB fix)
ons 2024-05-15 klockan 23:29 +0200 skrev Michael Niedermayer:
> On Wed, May 15, 2024 at 09:39:43PM +0200, Tomas Härdin wrote:
> > Hi
> >
> > So as I said in the coverity thread it would be good if we could
> > get at
> > least part of the codebase covered using formal tools. To this
> > effect I
> > sat down for an hour just now and gave libavutil/common.h a go with
> > Frama-C's Eva plugin [1;2]. This plugin performs value analysis,
> > which
> > is a much simpler analysis compared to say the weakest predicate
> > (WP)
> > plugin.
> >
> > Going through the functions from top to bottom it only took until
> > av_clipl_int32_c() to find my first UB, a patch for which is
> > attached.
> > Thus my harping on this has born at least some fruit.
> >
> > To run the analysis implemented in this set of patches (all of
> > which
> > I've attached here because I don't want to bother writing six
> > follow-up
> > email), first install frama-c using opam. I'm using 28.0~beta
> > (Nickel).
> > Then run "make verify" in libavutil/ and Eva should tell you that
> > 33%
> > of functions are covered and 100% of statements in those functions
> > are
> > covered, with zero alarms.
> >
> > If the project isn't interested in this then I'll probably continue
> > fiddling with it on my own mostly as exercise. But I suspect it
> > will
> > bear even more fruit in time.
>
> i think this is cool, especially considering
>
>
> > common.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > e451a7cd9a600ece22a6ee85ad7ed0c16349a411 0006-lavu-common.h-Fix-
> > UB-in-av_clipl_int32_c.patch
> > From 8a535878b9f1f87ca20d5e626f2c4c098b1c948e Mon Sep 17 00:00:00
> > 2001
> > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?=
> > Date: Wed, 15 May 2024 21:03:47 +0200
> > Subject: [PATCH 6/7] lavu/common.h: Fix UB in av_clipl_int32_c()
> >
> > ---
> > libavutil/common.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavutil/common.h b/libavutil/common.h
> > index d81131f8ad..0521ebbfc5 100644
> > --- a/libavutil/common.h
> > +++ b/libavutil/common.h
> > @@ -258,8 +258,8 @@ static av_always_inline av_const int16_t
> > av_clip_int16_c(int a)
> > */
> > static av_always_inline av_const int32_t av_clipl_int32_c(int64_t
> > a)
> > {
> > - if ((a+0x8000u) & ~UINT64_C(0x)) return
> > (int32_t)((a>>63) ^ 0x7FFF);
> > - else return
> > (int32_t)a;
> > + if ((a+UINT64_C(0x8000)) & ~UINT64_C(0x)) return
> > (int32_t)((a>>63) ^ 0x7FFF);
> > + else return
> > (int32_t)a;
> > }
>
> it already found something
>
> the av_clipl_int32_c patch LGTM
Yeah I'll push that patch later today or so if there are no objections.
I'll probably keep the rest of them in a fork of my own for now,
especially since I'm still learning more about Eva. For example, the
ACSL contracts I added to av_log2*() might not actually be necessary
/Tomas
___
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] [RFC] Value analysis with Frama-C's Eva plugin (and an UB fix)
tor 2024-05-16 klockan 13:12 +0100 skrev Andrew Sayers: > On Wed, May 15, 2024 at 09:39:43PM +0200, Tomas Härdin wrote: > > Hi > > > > So as I said in the coverity thread it would be good if we could > > get at > > least part of the codebase covered using formal tools. To this > > effect I > > sat down for an hour just now and gave libavutil/common.h a go with > > Frama-C's Eva plugin [1;2]. This plugin performs value analysis, > > which > > is a much simpler analysis compared to say the weakest predicate > > (WP) > > plugin. > > > > Going through the functions from top to bottom it only took until > > av_clipl_int32_c() to find my first UB, a patch for which is > > attached. > > Thus my harping on this has born at least some fruit. > > > > To run the analysis implemented in this set of patches (all of > > which > > I've attached here because I don't want to bother writing six > > follow-up > > email), first install frama-c using opam. I'm using 28.0~beta > > (Nickel). > > Then run "make verify" in libavutil/ and Eva should tell you that > > 33% > > of functions are covered and 100% of statements in those functions > > are > > covered, with zero alarms. > > > > If the project isn't interested in this then I'll probably continue > > fiddling with it on my own mostly as exercise. But I suspect it > > will > > bear even more fruit in time. > > > > /Tomas > > > > [1] https://frama-c.com/ > > [2] https://frama-c.com/fc-plugins/eva.html > > I'm all for automated checks, but in my experience they're only > worthwhile > if two conditions are met: > > * they run automatically on a regular basis They could easily be incorporated into FATE or a post-commit hook > * their output doesn't get boring The output of Frama-C in general tends to be quite chatty. I've asked a couple of time for them to add exit codes, for example returning with zero only if there are no alarms and no unproven proof obligations. With Eva grepping for " 0 alarms generated by the analysis." is one way, but that's also quite ugly /Tomas ___ 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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
The entire patchset passes FATE
/Tomas
From c000b8a5e90883f28ce6c58960227e5825ac20d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?=
Date: Wed, 15 May 2024 21:03:47 +0200
Subject: [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
Found by value analysis
---
libavutil/common.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavutil/common.h b/libavutil/common.h
index 3e4c339893..ac68c0cfff 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -252,8 +252,8 @@ static av_always_inline av_const int16_t av_clip_int16_c(int a)
*/
static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
{
-if ((a+0x8000u) & ~UINT64_C(0x)) return (int32_t)((a>>63) ^ 0x7FFF);
-else return (int32_t)a;
+if ((a+UINT64_C(0x8000)) & ~UINT64_C(0x)) return (int32_t)((a>>63) ^ 0x7FFF);
+else return (int32_t)a;
}
/**
--
2.39.2
___
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/5] lavu/common.h: Fix UB in av_clip_intp2_c()
From 7b18f24c0bedfeebcdfb23ea837cea8d4c35cf30 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?=
Date: Thu, 16 May 2024 16:33:44 +0200
Subject: [PATCH 2/5] lavu/common.h: Fix UB in av_clip_intp2_c()
Found by value analysis
---
libavutil/common.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavutil/common.h b/libavutil/common.h
index ac68c0cfff..715f0a594c 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -264,7 +264,7 @@ static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
*/
static av_always_inline av_const int av_clip_intp2_c(int a, int p)
{
-if (((unsigned)a + (1 << p)) & ~((2 << p) - 1))
+if (((unsigned)a + (1U << p)) & ~((2U << p) - 1))
return (a >> 31) ^ ((1 << p) - 1);
else
return a;
--
2.39.2
___
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 3/5] lavu/common.h: Fix UB in av_clip_uintp2_c()
From f81730f8facc54ef23df79ac8d33075403b4f76f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?=
Date: Thu, 16 May 2024 16:37:58 +0200
Subject: [PATCH 3/5] lavu/common.h: Fix UB in av_clip_uintp2_c()
Found by value analysis
---
libavutil/common.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavutil/common.h b/libavutil/common.h
index 715f0a594c..8a3c4d2fcf 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -278,8 +278,8 @@ static av_always_inline av_const int av_clip_intp2_c(int a, int p)
*/
static av_always_inline av_const unsigned av_clip_uintp2_c(int a, int p)
{
-if (a & ~((1<> 31 & ((1<> 31 & ((1U<___
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 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()
From f9a12089bc98dde0ccc2487d1442ec6ddb7705f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= Date: Thu, 16 May 2024 18:10:58 +0200 Subject: [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c() Found by value analysis --- libavutil/intmath.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavutil/intmath.h b/libavutil/intmath.h index c54d23b7bf..52e11a8d5f 100644 --- a/libavutil/intmath.h +++ b/libavutil/intmath.h @@ -119,7 +119,7 @@ static av_always_inline av_const int ff_ctz_c(int v) 0, 1, 28, 2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17, 4, 8, 31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18, 6, 11, 5, 10, 9 }; -return debruijn_ctz32[(uint32_t)((v & -v) * 0x077CB531U) >> 27]; +return debruijn_ctz32[(uint32_t)((v & -(uint32_t)v) * 0x077CB531U) >> 27]; } #endif @@ -135,7 +135,7 @@ static av_always_inline av_const int ff_ctzll_c(long long v) 63, 52, 6, 26, 37, 40, 33, 47, 61, 45, 43, 21, 23, 58, 17, 10, 51, 25, 36, 32, 60, 20, 57, 16, 50, 31, 19, 15, 30, 14, 13, 12 }; -return debruijn_ctz64[(uint64_t)((v & -v) * 0x022FDD63CC95386DU) >> 58]; +return debruijn_ctz64[(uint64_t)((v & -(uint64_t)v) * 0x022FDD63CC95386DU) >> 58]; } #endif -- 2.39.2 ___ 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 5/5] lavu/mathematics: Return early if either a or b is zero
This doesn't really fix anything, it just makes the value analysis
easier. I don't feel strongly about it.
/Tomas
From cf9c56d7d4d7325d51ba6d99259431be7fca1f67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?=
Date: Mon, 20 May 2024 14:46:01 +0200
Subject: [PATCH 5/5] lavu/mathematics: Return early if either a or b is zero
This removes the need to check b further down
---
libavutil/mathematics.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/libavutil/mathematics.c b/libavutil/mathematics.c
index 61aeb7c029..06f6e61e78 100644
--- a/libavutil/mathematics.c
+++ b/libavutil/mathematics.c
@@ -71,6 +71,9 @@ int64_t av_rescale_rnd(int64_t a, int64_t b, int64_t c, enum AVRounding rnd)
rnd -= AV_ROUND_PASS_MINMAX;
}
+if (a == 0 || b == 0)
+return 0;
+
if (a < 0)
return -(uint64_t)av_rescale_rnd(-FFMAX(a, -INT64_MAX), b, c, rnd ^ ((rnd >> 1) & 1));
@@ -85,7 +88,7 @@ int64_t av_rescale_rnd(int64_t a, int64_t b, int64_t c, enum AVRounding rnd)
else {
int64_t ad = a / c;
int64_t a2 = (a % c * b + r) / c;
-if (ad >= INT32_MAX && b && ad > (INT64_MAX - a2) / b)
+if (ad >= INT32_MAX && ad > (INT64_MAX - a2) / b)
return INT64_MIN;
return ad * b + a2;
}
--
2.39.2
___
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/5] lavu/common.h: Fix UB in av_clip_intp2_c()
tor 2024-05-30 klockan 00:24 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > static av_always_inline av_const int av_clip_intp2_c(int a, int p)
> > {
> > - if (((unsigned)a + (1 << p)) & ~((2 << p) - 1))
> > + if (((unsigned)a + (1U << p)) & ~((2U << p) - 1))
> > return (a >> 31) ^ ((1 << p) - 1);
> > else
> > return a;
>
> This will support p == 30 (but not 31); but the first change is not
> UB
> in this range.
p=31 is most definitely UB before this change. 1<<31 is signed overflow
with 32-bit int. The compiler has therefore been allowed to do whatever
for p=31 up until this point. To me it seems the intent of the code is
preserved
Personally I dislike bithacks because they are difficult to verify. A
good enough compiler will gain peephole optimizations for them sooner
or later anyway
/Tomas
___
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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
tor 2024-05-30 klockan 09:41 +0300 skrev Rémi Denis-Courmont:
> Hi,
>
> Le 30 mai 2024 01:13:14 GMT+03:00, "Tomas Härdin" a
> écrit :
> > The entire patchset passes FATE
>
> Is the version in riscv/intmath.h safe? It looks to me that the GCC
> codegen for not only RV64 but also AArch{32,64} and x86-64 is better
> than this.
I haven't checked. It seems weird to me to have two different C
versions. We shouldn't rely on type punning. The standard compliant way
is to use memcpy()
/Tomas
___
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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
tor 2024-05-30 klockan 00:31 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > */
> > static av_always_inline av_const int32_t av_clipl_int32_c(int64_t
> > a)
> > {
> > - if ((a+0x8000u) & ~UINT64_C(0x)) return
> > (int32_t)((a>>63) ^ 0x7FFF);
> > - else return
> > (int32_t)a;
> > + if ((a+UINT64_C(0x8000)) & ~UINT64_C(0x)) return
> > (int32_t)((a>>63) ^ 0x7FFF);
> > + else return
> > (int32_t)a;
>
> IMO (uint64_t)a + 0x8000 is more readable. (Maybe it would even
> be
> good to use >> 32 instead of ~UINT64_C(0x)?)
It already uses UINT64_C, hence why I used it.
>> 32 would work also. Does it make any difference performance wise?
/Tomas
___
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 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()
tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont: > Can't we just use the compiler built-ins here? AFAIK, they (GCC, > LLVM) use the same algorithm if the CPU doesn't support native CTZ. > And they will pick the right instruction if CPU does have CTZ. > > I get it that maybe it wasn't working so well 20 years ago, but we've > increased compiler version requirements since then. I think we still support MSVC, but maybe we shouldn't? It's possible to cross-compile for Windows either way. /Tomas ___ 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 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()
tor 2024-05-30 klockan 16:06 +0300 skrev Rémi Denis-Courmont: > > > Le 30 mai 2024 12:50:05 GMT+03:00, "Tomas Härdin" a > écrit : > > tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont: > > > Can't we just use the compiler built-ins here? AFAIK, they (GCC, > > > LLVM) use the same algorithm if the CPU doesn't support native > > > CTZ. > > > And they will pick the right instruction if CPU does have CTZ. > > > > > > I get it that maybe it wasn't working so well 20 years ago, but > > > we've > > > increased compiler version requirements since then. > > > > I think we still support MSVC, but maybe we shouldn't? It's > > possible to > > cross-compile for Windows either way. > > I don't get how that prevents using the GCC and Clang builtins (on > GCC and Clang). Does MSVC have builtins for these? Do all compilers we support? /Tomas ___ 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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
tor 2024-05-30 klockan 14:50 +0300 skrev Rémi Denis-Courmont:
>
>
> Le 30 mai 2024 12:40:20 GMT+03:00, "Tomas Härdin" a
> écrit :
> > tor 2024-05-30 klockan 09:41 +0300 skrev Rémi Denis-Courmont:
> > > Hi,
> > >
> > > Le 30 mai 2024 01:13:14 GMT+03:00, "Tomas Härdin"
> > > a
> > > écrit :
> > > > The entire patchset passes FATE
> > >
> > > Is the version in riscv/intmath.h safe? It looks to me that the
> > > GCC
> > > codegen for not only RV64 but also AArch{32,64} and x86-64 is
> > > better
> > > than this.
> >
> > I haven't checked. It seems weird to me to have two different C
> > versions.
>
> The common one ends up horrendously bad on RV, and presumably on MIPS
> and some other RISC ISA.
>
> > We shouldn't rely on type punning.
>
> Because?
>
> We should depend on punning as long as it conforms to the standard.
My mistake, I forgot type punning is allowed in C. It's UB in C++
> > The standard compliant way
> > is to use memcpy()
>
> That's way worse than union in terms of how proactively the compiler
> needs to optimise, and both approaches are as confirming.
A good compiler will do the same thing
Maybe I can get the riscv version covered by Eva as well. That's beyond
the scope of this patchset
/Tomas
___
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 1/2] lavc/speedhqdec: Add AV_CODEC_CAP_SLICE_THREADS
Ping Will push in a couple of days /Tomas ___ 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] lavc/speedhqenc: Require width to be a multiple of 16
Ping This stops us from producing broken output /Tomas ___ 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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
tor 2024-05-30 klockan 17:28 +0300 skrev Rémi Denis-Courmont: > > > Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin" a > écrit : > > > We should depend on punning as long as it conforms to the > > > standard. > > > > My mistake, I forgot type punning is allowed in C. It's UB in C++ > > > > > > The standard compliant way > > > > is to use memcpy() > > > > > > That's way worse than union in terms of how proactively the > > > compiler > > > needs to optimise, and both approaches are as confirming. > > > > A good compiler will do the same thing > > True, and I don't care very much about memcpy vs union, as they both > rely on matching representation. AFAIR, FFmpeg tends to use unions > though. > > > > > Maybe I can get the riscv version covered by Eva as well. That's > > beyond > > the scope of this patchset > > IMHO, this specific patch (and the following one) are beating dead > horses. Sure there may be theoretical UB in the current code, but if > there is a *better* implementation, better switch to that than bike > shedding the fix for the UB. Are you saying that UB is acceptable? You know the compiler is free to assume signed arithmetic doesn't overflow, right? If so then what other UB might we accept? /Tomas ___ 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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
tor 2024-05-30 klockan 12:42 -0300 skrev James Almer: > On 5/30/2024 12:32 PM, Tomas Härdin wrote: > > tor 2024-05-30 klockan 17:28 +0300 skrev Rémi Denis-Courmont: > > > > > > > > > Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin" > > > a > > > écrit : > > > > > We should depend on punning as long as it conforms to the > > > > > standard. > > > > > > > > My mistake, I forgot type punning is allowed in C. It's UB in > > > > C++ > > > > > > > > > > The standard compliant way > > > > > > is to use memcpy() > > > > > > > > > > That's way worse than union in terms of how proactively the > > > > > compiler > > > > > needs to optimise, and both approaches are as confirming. > > > > > > > > A good compiler will do the same thing > > > > > > True, and I don't care very much about memcpy vs union, as they > > > both > > > rely on matching representation. AFAIR, FFmpeg tends to use > > > unions > > > though. > > > > > > > > > > > Maybe I can get the riscv version covered by Eva as well. > > > > That's > > > > beyond > > > > the scope of this patchset > > > > > > IMHO, this specific patch (and the following one) are beating > > > dead > > > horses. Sure there may be theoretical UB in the current code, but > > > if > > > there is a *better* implementation, better switch to that than > > > bike > > > shedding the fix for the UB. > > > > Are you saying that UB is acceptable? You know the compiler is free > > to > > assume signed arithmetic doesn't overflow, right? If so then what > > other > > UB might we accept? > > He did not say that... He said we should switch to a better > implementation rather than trying to fix the existing potentially > buggy one. I have a fix for demonstrable UB and Rémi is problematizing it. It is not a "theoretical" UB - that's not how UB works. Any compiler doing basic value analysis will find it, and is therefore free to do whatever it wants, for example deleting all calls to av_clipl_int32_c(). We could certainly replace some of these functions with intrinsics, but that's not what this patchset is about. I don't know what set of compilers we support. I don't know what intrinsics they support. Am I to be compelled to figure that out, and provide the necessary intrinsics for all of them? This may all seem trivial, and it is, but this patchset is also a test balloon. Line struggle is important. What I see is the stalling of fixes of *known broken code*. That is not encouraging. /Tomas ___ 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 5/5] lavu/mathematics: Return early if either a or b is zero
fre 2024-05-31 klockan 02:22 +0200 skrev Michael Niedermayer: > On Thu, May 30, 2024 at 12:15:27AM +0200, Tomas Härdin wrote: > > This doesn't really fix anything, it just makes the value analysis > > easier. I don't feel strongly about it. > > if it doesnt fix anything and it doesnt make the code faster > for our usecases, then i would say that it shouldnt be in a > production build > > but not feeling strongly about this either I'll hold off on this one for the time being since again it's mostly there to make analysis easier. I have an even more *interesting* change that defaults to the long division version in more cases which makes things slower for the sake of correctness I'll push the other four patches in a day or two since the only real feedback was cosmetic, and we can discuss cosmetics until the cows come home /Tomas ___ 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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
tor 2024-05-30 klockan 20:49 +0300 skrev Rémi Denis-Courmont: > Le torstaina 30. toukokuuta 2024, 19.48.13 EEST Tomas Härdin a écrit > > It is not a "theoretical" UB - that's not how UB works. > > It is a *theoretical* UB if you can not prove that it leads to > misbehaviour in > any *practical* use. In theory, all UB is *potentially* fatal. > Emphasis on > potentially. The issue is that compilers can change without notice > > Any compiler doing > > basic value analysis will find it, and is therefore free to do > > whatever > > it wants, for example deleting all calls to av_clipl_int32_c(). > > That is formally true. But it is also formally true that, by that > same logic, > since there is most certainly some UB instance left elsewhere in the > codebase, > the entirety of libavutil could be elided by the compiler. I mean, part of what I'm doing with my little value analysis experiment is finding these instances of UB throughout lavu and fixing them, so that no compiler will perform what we might consider dubious or unexpected optimizations. It is far more powerful than fuzzing when it comes to discovering bugs I'm looking at rational.c at the moment, and there are definitely some dubious codepaths. There have also been fixes for signed overflow in rational.c already. I wouldn't be surprised if there are more corner cases that can be triggered by some demuxer or decoder that fuzzing hasn't discovered yet Maybe in some glorious future we'll have a version of C where signed overflow is defined behavior /Tomas ___ 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] avcodec/hevc_ps: Fix UB 1 << 31
lör 2024-06-01 klockan 15:13 +0200 skrev Andreas Rheinhardt: > Signed-off-by: Andreas Rheinhardt > --- > libavcodec/hevc_ps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c > index 7b486ce0af..1a459ad054 100644 > --- a/libavcodec/hevc_ps.c > +++ b/libavcodec/hevc_ps.c > @@ -200,7 +200,7 @@ int ff_hevc_decode_short_term_rps(GetBitContext > *gb, AVCodecContext *avctx, > } > > for (unsigned i = 0; i < FF_ARRAY_ELEMS(used); i++) > - rps->used |= used[i] * (1 << i); > + rps->used |= used[i] * (1U << i); Why not just (uint32_t)used[i] << i? /Tomas ___ 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] [RFC] STF 2025
fre 2024-05-17 klockan 15:49 +0200 skrev Michael Niedermayer: > * Paul to work on FFmpeg full time. My idea here is that he can work > on whatever > he likes in FFmpeg (so its not full time employment for specific > work but > simply full time employment for him to work on whatever he likes in > FFmpeg any > way he likes) Paul is the 2nd largest contributor to FFmpeg (git > shortlog -s -n) Didn't Paul loudly "leave" the project less than one year ago? > * Fund administrative / maintainance work (one example is the mailman > upgrade that is needed > with the next OS upgrade on one of our servers (this is not as > trivial as one might > expect). Another example here may be some git related tools if we > find something that > theres a broad consensus about. Sounds reasonable > * Fund maintaince on the bug tracker, try to reproduce bugs, ask > users to provide > reproduceable cases, close bugs still unreproduceable, ... > ATM we have over 2000 "new" bugs that are not even marked as open Reasonable > * Fund professional real live presence on multimedia / FOSS / > buisness related > events. Also reasonable. I could help man a booth at IBC or any other event in Europe > Also we need more cute girls on these events, everything i hear > its 100% male geeks/hackers. Michael please /Tomas ___ 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] [RFC] STF 2025
sön 2024-06-02 klockan 20:01 +0200 skrev Michael Niedermayer: > Hi > > > On Sat, Jun 01, 2024 at 05:19:26PM +0200, Tomas Härdin wrote: > > [...] > > > > * Fund professional real live presence on multimedia / FOSS / > > > buisness related > > > events. > > > > Also reasonable. I could help man a booth at IBC or any other event > > in > > Europe > > Iam strongly in favor of that! Though i have no idea about cost (for > IBC) > which probably requires someone to sponsor a booth if its not free. > Or any details. But i think its probably best if you mail thilo as he > was helping with FFmpeg presence on many european booths Attending is free, so I expect booths cost quite a bit to make up the costs. There's an inquiry form on the IBC website. Can't hurt to ask Hotels aren't cheap as Rémi points out. Last time I attended IBC we had to get a hotel in Harlem. Luckily I know some people in Amsterdam /Tomas ___ 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 v2] avcodec: add farbfeld encoder
This format seems to reproduce some of the same issues as QOI: > The RGB-data should be sRGB for best interoperability and not alpha- > premultiplied. This seems to imply it could be something other than sRGB since it says SHOULD rather than MUST. This probably isn't a huge issue, but it should be clearer on the website. It also doesn't say whether alpha is linear or not. > +int pkt_size = HEADER_SIZE + av_image_get_buffer_size( > +p->format, > +p->width, > +p->height, > +1 > +); Check the return value of av_image_get_buffer_size() before adding HEADER_SIZE to it. There will be a signed overflow (UB) for images of size 16385x16385 (and many others). Aside: av_image_get_buffer_size() will UB for sizes above INT_MAX because the size_t's in sizes[] get accumulated into an int. Besides the UB it also returns incorrect values. /Tomas ___ 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 v2] avcodec: add farbfeld encoder
> Check the return value of av_image_get_buffer_size() before adding > HEADER_SIZE to it. There will be a signed overflow (UB) for images of > size 16385x16385 (and many others). Sorry, I missed the multiplication by h+128 in av_image_check_size2(). So this isn't a problem in this specific case. > Aside: av_image_get_buffer_size() will UB for sizes above INT_MAX > because the size_t's in sizes[] get accumulated into an int. Besides > the UB it also returns incorrect values. This however *is* a problem for planar formats. This doesn't affect this patch however. /Tomass ___ 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] lavc/speedhqenc: Require width to be a multiple of 16
tor 2024-05-30 klockan 16:23 +0200 skrev Tomas Härdin: > Ping > > This stops us from producing broken output Pushed /Tomass ___ 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 1/2] lavc/speedhqdec: Add AV_CODEC_CAP_SLICE_THREADS
tor 2024-05-30 klockan 16:23 +0200 skrev Tomas Härdin: > Ping > > Will push in a couple of days Passes FATE -> pushed /Tomas ___ 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] [RFC] STF 2025
mån 2024-06-03 klockan 08:50 +0200 skrev Thilo Borgmann via ffmpeg- devel: > Am 02.06.24 um 22:14 schrieb Tomas Härdin: > > sön 2024-06-02 klockan 20:01 +0200 skrev Michael Niedermayer: > > > Hi > > > > > > > > > On Sat, Jun 01, 2024 at 05:19:26PM +0200, Tomas Härdin wrote: > > > > > > [...] > > > > > > > > * Fund professional real live presence on multimedia / FOSS / > > > > > buisness related > > > > > events. > > > > > > > > Also reasonable. I could help man a booth at IBC or any other > > > > event > > > > in > > > > Europe > > > > > > Iam strongly in favor of that! Though i have no idea about cost > > > (for > > > IBC) > > > which probably requires someone to sponsor a booth if its not > > > free. > > > Or any details. But i think its probably best if you mail thilo > > > as he > > > was helping with FFmpeg presence on many european booths > > > > Attending is free, so I expect booths cost quite a bit to make up > > the > > costs. There's an inquiry form on the IBC website. Can't hurt to > > ask > > > > Hotels aren't cheap as Rémi points out. Last time I attended IBC we > > had > > to get a hotel in Harlem. Luckily I know some people in Amsterdam > > We have a booth on IBC this year which again gets sponsored so no > costs for FFmpeg. > Some details are still unclear which is why it's not yet announced. Alright, cool /Tomas ___ 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] avcodec/libx265: Don't copy A53 closed captions by default
tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: > The SEI handling of libx265 is buggy and can easily lead > to memory corruption: It reuses certain buffers, but when > reusing them it presumes that it is enough for these buffers > to exist and does not check whether they are actually large > enough to hold what is intended to be stored in them.* > > Our users are exposed to this because forwarding A53 CC data > is enabled by default. Change this to make it disabled > by default. > > "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. Shouldn't users use non-buggy versions of libx26? I've had people ask about CC, and I'm sure many users would be annoyed at them suddenly breaking. I suggest complaining loudly at compile time and/or when loading libx265 instead /Tomas ___ 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] avcodec/libx265: Don't copy A53 closed captions by default
tis 2024-06-11 klockan 10:05 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: > > > The SEI handling of libx265 is buggy and can easily lead > > > to memory corruption: It reuses certain buffers, but when > > > reusing them it presumes that it is enough for these buffers > > > to exist and does not check whether they are actually large > > > enough to hold what is intended to be stored in them.* > > > > > > Our users are exposed to this because forwarding A53 CC data > > > is enabled by default. Change this to make it disabled > > > by default. > > > > > > "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. > > > > Shouldn't users use non-buggy versions of libx26? I've had people > > ask > > about CC, and I'm sure many users would be annoyed at them suddenly > > breaking. I suggest complaining loudly at compile time and/or when > > loading libx265 instead > > Non-buggy versions of libx265? People use what they have because it > exists. What I'm getting at is that this is libx265's responsibility, and the responsibility of packagers not to ship broken versions of it. Does all A53 CCs break with the present libx265 bug or only some? /Tomas ___ 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] avcodec/libx265: Don't copy A53 closed captions by default
tis 2024-06-11 klockan 10:16 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2024-06-11 klockan 10:05 +0200 skrev Andreas Rheinhardt: > > > Tomas Härdin: > > > > tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: > > > > > The SEI handling of libx265 is buggy and can easily lead > > > > > to memory corruption: It reuses certain buffers, but when > > > > > reusing them it presumes that it is enough for these buffers > > > > > to exist and does not check whether they are actually large > > > > > enough to hold what is intended to be stored in them.* > > > > > > > > > > Our users are exposed to this because forwarding A53 CC data > > > > > is enabled by default. Change this to make it disabled > > > > > by default. > > > > > > > > > > "Fixes" tickets #9666, #10411, #11052 and (presumably) > > > > > #10906. > > > > > > > > Shouldn't users use non-buggy versions of libx26? I've had > > > > people > > > > ask > > > > about CC, and I'm sure many users would be annoyed at them > > > > suddenly > > > > breaking. I suggest complaining loudly at compile time and/or > > > > when > > > > loading libx265 instead > > > > > > Non-buggy versions of libx265? People use what they have because > > > it > > > exists. > > > > What I'm getting at is that this is libx265's responsibility, and > > the > > responsibility of packagers not to ship broken versions of it. Does > > all > > A53 CCs break with the present libx265 bug or only some? > > > > 1. There is no version of libx265 with this bug fixed (the bug itself > is > here: > https://bitbucket.org/multicoreware/x265_git/src/8787e87020d77416f0ff0b7f3c97ac8b90332c31/source/encoder/encoder.cpp#lines-1086:1117 > ) Then I expect libx265 to fix it posthaste and push a new release, and for Debian etc to discourage installing versions prior to that as appropriate We can change the default of course (yolo!), but expect non-zero numbers of angry users whose workflows suddenly break /Tomas ___ 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 1/2] lavu/bswap: remove some inline assembler
tis 2024-06-11 klockan 12:38 -0300 skrev James Almer: > On 6/11/2024 10:15 AM, Michael Niedermayer wrote: > > On Fri, Jun 07, 2024 at 09:19:46PM +0300, Rémi Denis-Courmont > > wrote: > > > C code or compiler built-ins are preferable over inline assembler > > > for > > > byte-swaps as it allows for better optimisations (e.g. > > > instruction > > > scheduling) which would otherwise be impossible. > > > > > > As with f64c2e710fa1a7b59753224e717f57c48462076f for x86 and Arm, > > > this removes the inline assembler on GCC (and Clang) since we now > > > require recent enough compiler versions (this indeed seems to > > > work on > > > AArch64). > > > --- > > > libavutil/aarch64/bswap.h | 56 > > > --- > > > libavutil/avr32/bswap.h | 44 -- > > > libavutil/bswap.h | 8 +- > > > libavutil/sh4/bswap.h | 48 > > > - > > > > As you are writing that this preferrable for better optimisations > > Please provide benchmarks (for sh4, avr32) > > This is a ridiculous request, considering nobody has such hardware at > all. Maybe Måns has? He's the one who added the AVR32 code. The SH4 code was added all the way back in 2003 in 0c6bd2ea by someone who goes by BERO. Perhaps we should demand platforms for which we have asm also have FATE instances? /Tomas ___ 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 1/2] lavu/bswap: remove some inline assembler
tis 2024-06-11 klockan 18:10 +0200 skrev Michael Niedermayer: > On Tue, Jun 11, 2024 at 05:50:35PM +0200, Tomas Härdin wrote: > [...] > > Perhaps we should demand platforms for which we have asm also have > > FATE > > instances? > > qemu based fate we have for sh-4: > https://fate.ffmpeg.org/?query=subarch:sh4%2F%2F I think we need actual machines, and actual users that want to run on those machines, else we're just doing mental self-gratification /Tomas ___ 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 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()
Pushed patches 1-4 /Tomas ___ 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] avr32: remove explicit support
sön 2024-06-09 klockan 14:55 +0300 skrev Rémi Denis-Courmont: > The vendor has long since switched to Arm, wit the last product > reaching > their official end-of-life over 11 years ago. Linux support for the > ISA > was dropped 7 years ago. More importantly, this architecture was > never > supported by upstream GCC, and the vendor fork is stuck at version > 4.2, > which FFmpeg no longer supports (as per C11 requirement). > > Presumably, this is still the case given the lack of vendor support. > Indeed all of the code being removed here consisted of inline > assembler > scalar optimisations. A sane C compiler should be able to perform > those > automatically nowadays (with the sole exception of fast CLZ > detection), > but this is moot as this architecture is evidently dead. > --- > configure | 26 + > libavcodec/avr32/mathops.h | 101 -- > libavcodec/mathops.h | 2 - > libavutil/avr32/bswap.h | 44 > libavutil/avr32/intreadwrite.h | 182 --- > -- > libavutil/bswap.h | 2 - > libavutil/intreadwrite.h | 2 - > 7 files changed, 1 insertion(+), 358 deletions(-) > delete mode 100644 libavcodec/avr32/mathops.h > delete mode 100644 libavutil/avr32/bswap.h > delete mode 100644 libavutil/avr32/intreadwrite.h Sounds good to me /Tomas ___ 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] fate/lavf-container: add a hevc in ISOBMFF remux test
mån 2024-06-17 klockan 11:41 -0300 skrev James Almer: > Signed-off-by: James Almer > --- > tests/fate/lavf-container.mak | 2 ++ > tests/ref/lavf-fate/hevc.mp4 | 3 +++ > 2 files changed, 5 insertions(+) > create mode 100644 tests/ref/lavf-fate/hevc.mp4 Looks OK /Tomas ___ 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 v2] avcodec/jpeg2000dec: Add support for placeholder passes, CAP, and CPF markers
It seems this patch combines a lot of things that might be better to
split into separate patches for easier review
> @@ -382,6 +391,9 @@ static int get_siz(Jpeg2000DecoderContext *s)
> } else if (ncomponents == 1 && s->precision == 8) {
> s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> i = 0;
> + } else if (ncomponents == 1 && s->precision == 12) {
> + s->avctx->pix_fmt = AV_PIX_FMT_GRAY16LE;
> + i = 0;
Could we handle 9 <= precision <= 16 while we're at it?
> @@ -408,6 +420,73 @@ static int get_siz(Jpeg2000DecoderContext *s)
> s->avctx->bits_per_raw_sample = s->precision;
> return 0;
> }
> +/* get extended capabilities (CAP) marker segment */
> +static int get_cap(Jpeg2000DecoderContext *s, Jpeg2000CodingStyle
> *c)
> +{
> + uint32_t Pcap;
> + uint16_t Ccap_i[32] = { 0 };
> + uint16_t Ccap_15;
> + uint8_t P;
> +
> + if (bytestream2_get_bytes_left(&s->g) < 6) {
> + av_log(s->avctx, AV_LOG_ERROR, "Insufficient space for
> CAP\n");
> + return AVERROR_INVALIDDATA;
> + }
> +
> + Pcap = bytestream2_get_be32u(&s->g);
> + s->isHT = (Pcap >> (31 - (15 - 1))) & 1;
> + for (int i = 0; i < 32; i++) {
> + if ((Pcap >> (31 - i)) & 1)
> + Ccap_i[i] = bytestream2_get_be16u(&s->g);
> + }
> + Ccap_15 = Ccap_i[14];
> + if (s->isHT == 1) {
> + av_log(s->avctx, AV_LOG_INFO, "This is an HTJ2K codestream.\n");
> + // Bits 14-15
> + switch ((Ccap_15 >> 14) & 0x3) {
Missing indentation
> + case 0x3:
> + s->Ccap15_b14_15 = HTJ2K_MIXED;
> + break;
> + case 0x1:
> + s->Ccap15_b14_15 = HTJ2K_HTDECLARED;
> + break;
> + case 0x0:
> + s->Ccap15_b14_15 = HTJ2K_HTONLY;
> + break;
> + default:
> + av_log(s->avctx, AV_LOG_ERROR, "Unknown CCap
> value.\n");
> + return AVERROR(EINVAL);
> + break;
> + }
> + // Bit 13
> + if ((Ccap_15 >> 13) & 1) {
> + av_log(s->avctx, AV_LOG_ERROR, "MULTIHT set is not
> supported.\n");
> + return AVERROR_PATCHWELCOME;
> + }
> + // Bit 12
> + s->Ccap15_b12 = (Ccap_15 >> 12) & 1;
> + // Bit 11
> + s->Ccap15_b11 = (Ccap_15 >> 11) & 1;
> + // Bit 5
> + s->Ccap15_b05 = (Ccap_15 >> 5) & 1;
> + // Bit 0-4
> + P = Ccap_15 & 0x1F;
> + if (!P)
> + s->HT_MAGB = 8;
> + else if (P < 20)
> + s->HT_MAGB = P + 8;
> + else if (P < 31)
> + s->HT_MAGB = 4 * (P - 19) + 27;
> + else
> + s->HT_MAGB = 74;
> +
> + if (s->HT_MAGB > 31) {
> + av_log(s->avctx, AV_LOG_ERROR, "Available internal
> precision is exceeded (MAGB> 31).\n");
> + return AVERROR_PATCHWELCOME;
> + }
> + }
Weird indentation
> @@ -802,6 +881,15 @@ static int read_crg(Jpeg2000DecoderContext *s,
> int n)
> bytestream2_skip(&s->g, n - 2);
> return 0;
> }
> +
> +static int read_cpf(Jpeg2000DecoderContext *s, int n)
> +{
> + if (bytestream2_get_bytes_left(&s->g) < (n - 2))
> + return AVERROR_INVALIDDATA;
> + bytestream2_skip(&s->g, n - 2);
> + return 0;
> +}
Don't we already have code for skipping markers we don't care about?
> +
> /* Tile-part lengths: see ISO 15444-1:2002, section A.7.1
> * Used to know the number of tile parts and lengths.
> * There may be multiple TLMs in the header.
> @@ -965,6 +1053,10 @@ static int init_tile(Jpeg2000DecoderContext *s,
> int tileno)
> comp->roi_shift = s->roi_shift[compno];
> if (!codsty->init)
> return AVERROR_INVALIDDATA;
> + if (s->isHT && (!s->Ccap15_b05) && (!codsty->transform))
> + av_log(s->avctx, AV_LOG_WARNING, "Transformation = 0
> (lossy DWT) is found in HTREV HT set\n");
> + if (s->isHT && s->Ccap15_b14_15 != (codsty->cblk_style >> 6)
> && s->Ccap15_b14_15 != HTJ2K_HTONLY)
> + av_log(s->avctx, AV_LOG_WARNING, "SPcod/SPcoc value does
> not match bit 14-15 values of Ccap15\n");
Do you have samples demonstrating the need to accept such broken files?
If not then we should probably error out
> @@ -1704,7 +1989,7 @@ static int decode_cblk(const
> Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *cod
> Jpeg2000T1Context *t1, Jpeg2000Cblk *cblk,
> int width, int height, int bandpos, uint8_t
> roi_shift)
> {
> - int passno = cblk->npasses, pass_t = 2, bpno = cblk->nonzerobits
> - 1 + roi_shift;
> + int passno = cblk->npasses, pass_t = 2, bpno = cblk->nonzerobits
> - 1;
Won't this break files with ROI? I see there's some ROI stuff further
down so maybe not
> @@ -2187,22 +2472,42 @@ static int
> jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
> if (!s->tile)
> s->numXtiles = s->numYtiles = 0;
> break;
> + case JPEG2000_CAP:
> + if (!s->ncomponents) {
> + av_log(s->avctx, AV_LOG_WARNING, "CAP marker
Re: [FFmpeg-devel] [PATCH] fate/jpeg2000dec: add support for p0_10.j2k
lör 2024-06-15 klockan 21:47 -0700 skrev p...@sandflow.com: > From: Pierre-Anthony Lemieux > > p0_10.j2k is one of the reference codestreams included in Rec. ITU-T > T.803 | ISO/IEC 15444-4. > --- > tests/fate/jpeg2000.mak | 3 +++ > tests/ref/fate/jpeg2000dec-p0_10 | 6 ++ > 2 files changed, 9 insertions(+) > create mode 100644 tests/ref/fate/jpeg2000dec-p0_10 Sounds good, assuming it decodes correctly If there are more files like this, could we add all of them in one go? /Tomas ___ 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 3/5] avformat/mxfdec: Check container_ul->desc before use
fre 2024-06-07 klockan 02:32 +0200 skrev Michael Niedermayer:
> Fixes: CID1592939 Dereference after null check
>
> Sponsored-by: Sovereign Tech Fund
> Signed-off-by: Michael Niedermayer
> ---
> libavformat/mxfdec.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index e65cec74c23..820b03940aa 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -3031,6 +3031,7 @@ static int
> mxf_parse_structural_metadata(MXFContext *mxf)
> if (container_ul->desc)
> av_dict_set(&st->metadata, "data_type",
> container_ul->desc, 0);
> if (mxf->eia608_extract &&
> + container_ul->desc &&
> !strcmp(container_ul->desc, "vbi_vanc_smpte_436M"))
> {
> st->codecpar->codec_type = AVMEDIA_TYPE_SUBTITLE;
> st->codecpar->codec_id = AV_CODEC_ID_EIA_608;
OK
/Tomas
___
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 4/5] avformat/mxfenc: Remove dead code
fre 2024-06-07 klockan 02:32 +0200 skrev Michael Niedermayer:
> Fixes: CID1524681 Logically dead code
>
> Sponsored-by: Sovereign Tech Fund
> Signed-off-by: Michael Niedermayer
> ---
> libavformat/mxfenc.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index f424858fc4e..b8e7bfe3018 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2606,9 +2606,6 @@ static int mxf_parse_ffv1_frame(AVFormatContext
> *s, AVStream *st, AVPacket *pkt)
> ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
> v = get_ffv1_unsigned_symbol(&c, state);
> av_assert0(v >= 2);
> - if (v > 4) {
> - return 0;
> - }
> if (v > 4) {
> av_log(s, AV_LOG_ERROR, "unsupported ffv1 version %d\n",
> v);
> return 0;
Commit message isn't quite accurate - this rather resurrects the error
print
/Tomas
___
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] [RFC] STF 2025
mån 2024-06-03 klockan 08:50 +0200 skrev Thilo Borgmann via ffmpeg- devel: > Am 02.06.24 um 22:14 schrieb Tomas Härdin: > > sön 2024-06-02 klockan 20:01 +0200 skrev Michael Niedermayer: > > > Hi > > > > > > > > > On Sat, Jun 01, 2024 at 05:19:26PM +0200, Tomas Härdin wrote: > > > > > > [...] > > > > > > > > * Fund professional real live presence on multimedia / FOSS / > > > > > buisness related > > > > > events. > > > > > > > > Also reasonable. I could help man a booth at IBC or any other > > > > event > > > > in > > > > Europe > > > > > > Iam strongly in favor of that! Though i have no idea about cost > > > (for > > > IBC) > > > which probably requires someone to sponsor a booth if its not > > > free. > > > Or any details. But i think its probably best if you mail thilo > > > as he > > > was helping with FFmpeg presence on many european booths > > > > Attending is free, so I expect booths cost quite a bit to make up > > the > > costs. There's an inquiry form on the IBC website. Can't hurt to > > ask > > > > Hotels aren't cheap as Rémi points out. Last time I attended IBC we > > had > > to get a hotel in Harlem. Luckily I know some people in Amsterdam > > We have a booth on IBC this year which again gets sponsored so no > costs for FFmpeg. > Some details are still unclear which is why it's not yet announced. > > @Thomas: Happy you want to attend, I'll keep you updated. Update: I probably won't be able to attend due to a scheduling conflict /Tomas ___ 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 v2] avcodec: add farbfeld encoder
mån 2024-06-03 klockan 19:16 + skrev marcus: > > > > > > > Check the return value of av_image_get_buffer_size() before > > > adding > > > > > HEADER_SIZE to it. There will be a signed overflow (UB) for > > > images of > > > size 16385x16385 (and many others). > > > > > > Sorry, I missed the multiplication by h+128 in > > av_image_check_size2(). > > So this isn't a problem in this specific case. > > > > > Aside: av_image_get_buffer_size() will UB for sizes above INT_MAX > > > because the size_t's in sizes[] get accumulated into an int. > > > Besides > > > the UB it also returns incorrect values. > > > > > > This however is a problem for planar formats. This doesn't affect > > this patch however. > Did you incorrectly format that message? Or did you really mean that > the UB when the size is greater than INT_MAX doesn't affect my patch? It doesn't affect this patch because AV_PIX_FMT_RGBA64BE isn't planar /Tomas ___ 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 v2] avcodec/jpeg2000dec: Add support for placeholder passes, CAP, and CPF markers
ons 2024-06-19 klockan 05:51 + skrev WATANABE Osamu:
> First of all, I appreciate your kind review.
> I'm writing to discuss the changes and would like to hear your
> feedback on these.
>
>
> > On Jun 18, 2024, at 23:20, Tomas Hardin wrote:
> >
> >
> > It seems this patch combines a lot of things that might be better
> > to
> > split into separate patches for easier review
>
> Agree. I will split this patch into several patches.
> For example, the set of patches includes changes:
> - only for HTJ2K (JPEG 2000 Part 15)
> - only for J2K (JPEG 2000 Part 1)
> - for both J2K and HTJ2K.
>
> Do you think it makes sense?
Maybe. Going by the commit message, separate support for placeholder
passes from CAP from CFP handling perhaps?
> > > @@ -382,6 +391,9 @@ static int get_siz(Jpeg2000DecoderContext *s)
> > > } else if (ncomponents == 1 && s->precision == 8) {
> > > s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> > > i = 0;
> > > + } else if (ncomponents == 1 && s->precision == 12) {
> > > + s->avctx->pix_fmt = AV_PIX_FMT_GRAY16LE;
> > > + i = 0;
> >
> > Could we handle 9 <= precision <= 16 while we're at it?
> >
>
> Yes. The native J2K decoder for both lossless and lossy J2K/HTJ2K
> codestreams can handle bpp from 9 to 16. This change is required to
> produce the decoded images for the ISO test codestreams defined in
> Part 4 (Conformance testing).
Are there any test files for the other precisions?
> > > @@ -802,6 +881,15 @@ static int read_crg(Jpeg2000DecoderContext
> > > *s,
> > > int n)
> > > bytestream2_skip(&s->g, n - 2);
> > > return 0;
> > > }
> > > +
> > > +static int read_cpf(Jpeg2000DecoderContext *s, int n)
> > > +{
> > > + if (bytestream2_get_bytes_left(&s->g) < (n - 2))
> > > + return AVERROR_INVALIDDATA;
> > > + bytestream2_skip(&s->g, n - 2);
> > > + return 0;
> > > +}
> >
> > Don't we already have code for skipping markers we don't care
> > about?
> >
>
> The `read_cpf()` function was added for consistency with the
> `read_crg()` function.
> We already have `bytestream2_skip(GetByteContext *g, unsigned int
> size)` that skips `size`
> bytes from the compressed data.
> Do you think it is better to replace those functions (= `read_cpf()`
> and `read_crg()`)
> with `bytestream2_skip()`?
read_crg() performs a sanity check on ncomponents so it's not quite the
same. On the other hand this always checks the length of the marker
unlike the main parsing loop which only does so if
strict_std_compliance >= FF_COMPLIANCE_STRICT. I guess keeping
read_cpf() in the patch is fine and is useful for the future
> > > /* Tile-part lengths: see ISO 15444-1:2002, section A.7.1
> > > * Used to know the number of tile parts and lengths.
> > > * There may be multiple TLMs in the header.
> > > @@ -965,6 +1053,10 @@ static int init_tile(Jpeg2000DecoderContext
> > > *s,
> > > int tileno)
> > > comp->roi_shift = s->roi_shift[compno];
> > > if (!codsty->init)
> > > return AVERROR_INVALIDDATA;
> > > + if (s->isHT && (!s->Ccap15_b05) && (!codsty->transform))
> > > + av_log(s->avctx, AV_LOG_WARNING, "Transformation = 0
> > > (lossy DWT) is found in HTREV HT set\n");
> > > + if (s->isHT && s->Ccap15_b14_15 != (codsty->cblk_style
> > > >> 6)
> > > && s->Ccap15_b14_15 != HTJ2K_HTONLY)
> > > + av_log(s->avctx, AV_LOG_WARNING, "SPcod/SPcoc value
> > > does
> > > not match bit 14-15 values of Ccap15\n");
> >
> > Do you have samples demonstrating the need to accept such broken
> > files?
> > If not then we should probably error out
>
> Does `error out` mean that
> - Should we exit decoding here?
> - or should we replace AV_LOG_WARNING with AV_LOG_ERROR?
Returning with AVERROR_INVALIDDATA
> > > @@ -2187,22 +2472,42 @@ static int
> > > jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
> > > if (!s->tile)
> > > s->numXtiles = s->numYtiles = 0;
> > > break;
> > > + case JPEG2000_CAP:
> > > + if (!s->ncomponents) {
> > > + av_log(s->avctx, AV_LOG_WARNING, "CAP marker
> > > segment
> > > shall come after SIZ\n");
> >
> > SHALL -> we should be able to safely reject. Similarly with the
> > other
> > errors. Unless we know of an encoder that produces broken files
> > then
> > there's no reason to be lenient. And if such a broken encoder
> > exists we
> > could try to get it fixed
>
> Does `safely reject` mean that we should replace AV_LOG_WARNING with
> AV_LOG_ERROR? or we should stop decoding here?
Returning AVERROR_INVALIDDATA since the input is invalid per the spec.
Else we invite having to deal with incredibly broken encoders.
> > > @@ -112,6 +112,13 @@ typedef struct Jpeg2000DecoderContext {
> > > Jpeg2000Tile *tile;
> > > Jpeg2000DSPContext dsp;
> > >
> > > + uint8_t isHT; // HTJ2K?
> >
> > Isn't it possible to mix Part 1 and HT in the same file? I
Re: [FFmpeg-devel] [PATCH] fate/jpeg2000dec: add support for p0_10.j2k
tis 2024-06-18 klockan 07:59 -0700 skrev Pierre-Anthony Lemieux: > On Tue, Jun 18, 2024 at 7:25 AM Tomas Härdin wrote: > > > > lör 2024-06-15 klockan 21:47 -0700 skrev p...@sandflow.com: > > > From: Pierre-Anthony Lemieux > > > > > > p0_10.j2k is one of the reference codestreams included in Rec. > > > ITU-T > > > T.803 | ISO/IEC 15444-4. > > > --- > > > tests/fate/jpeg2000.mak | 3 +++ > > > tests/ref/fate/jpeg2000dec-p0_10 | 6 ++ > > > 2 files changed, 9 insertions(+) > > > create mode 100644 tests/ref/fate/jpeg2000dec-p0_10 > > > > Sounds good, assuming it decodes correctly > > > > If there are more files like this, could we add all of them in one > > go? > > I expect significantly more files to be added once the "Add support > for placeholder passes, CAP, and CPF markers" patch is merged. In the > meantime, I do not see a downside to updating FATE since it addresses > a specific bug in trac. Alright, fair enough /Tomas ___ 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 3/3] avformat/mxfdec: don't use sizeof(AVMasteringDisplayMetadata)
ons 2024-06-19 klockan 15:24 -0300 skrev James Almer:
> It's not part of the libavutil ABI.
>
> Signed-off-by: James Almer
> ---
> libavformat/mxfdec.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 852fb7e056..a5863445ab 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -227,6 +227,7 @@ typedef struct MXFDescriptor {
> UID color_trc_ul;
> UID color_space_ul;
> AVMasteringDisplayMetadata *mastering;
> + size_t mastering_size;
> AVContentLightMetadata *coll;
> size_t coll_size;
> } MXFDescriptor;
> @@ -1424,7 +1425,7 @@ static int mxf_read_generic_descriptor(void
> *arg, AVIOContext *pb, int tag, int
> }
> if (IS_KLV_KEY(uid, mxf_mastering_display_prefix)) {
> if (!descriptor->mastering) {
> - descriptor->mastering =
> av_mastering_display_metadata_alloc();
> + descriptor->mastering =
> av_mastering_display_metadata_alloc_size(&descriptor-
> >mastering_size);
> if (!descriptor->mastering)
> return AVERROR(ENOMEM);
> }
> @@ -2955,7 +2956,7 @@ static int
> mxf_parse_structural_metadata(MXFContext *mxf)
> if (descriptor->mastering) {
> if (!av_packet_side_data_add(&st->codecpar-
> >coded_side_data, &st->codecpar->nb_coded_side_data,
>
> AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
> - (uint8_t *)descriptor-
> >mastering, sizeof(*descriptor->mastering), 0)) {
> + (uint8_t *)descriptor-
> >mastering, descriptor->mastering_size, 0)) {
> ret = AVERROR(ENOMEM);
> goto fail_and_free;
> }
Looks OK
/Tomas
___
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] ffmpeg: Carry streamid as metadata key 'id'
mån 2024-07-01 klockan 16:51 +0200 skrev Anton Khirnov: > Quoting Tomas Härdin (2024-04-16 16:12:13) > > tis 2024-04-16 klockan 09:52 -0300 skrev James Almer: > > > On Tue, Apr 16, 2024 at 9:38 AM Anton Khirnov > > > wrote: > > > > > > > Quoting Tomas Härdin (2024-04-12 11:40:47) > > > > > This idea could be extended to other fields not presently > > > > > considered to > > > > > be metadata, that would be handy to treat as such. > > > > > > > > > > I use the key "id" because ffprobe outputs id= for streamid. > > > > > Another > > > > > option could be to collect these types of metadata that go > > > > > into > > > > > AVStream fields under a namespace like FFMPEG: or AVSTREAM: > > > > > or > > > > > something, then delete all of them using > > > > > AV_DICT_IGNORE_SUFFIX > > > > > near the > > > > > end of of_open() since they're for internal ffmpeg use. > > > > > > > > > > The FATE change is just because av_dict() changes the order > > > > > of > > > > > things > > > > > when elements are deleted. > > > > > > > > > > /Tomas > > > > > > > > > > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 > > > > > 00:00:00 > > > > > 2001 > > > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= > > > > > Date: Fri, 12 Apr 2024 10:34:12 +0200 > > > > > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id' > > > > > > > > > > This allows using -map_metadata and -metadata to copy/set > > > > > streamids > > > > (PIDs). > > > > > > > > I dislike this patch, metadata is the wrong place for such > > > > information. > > > > Seems like a matter of taste to me, but maybe I'm missing something > > It's not just a matter of taste - it's happened several times already > that people (ab)used metadata to carry structured information and > then > it turned out it was a bad idea and we had to change it to a real > API. > > Metadata really should only be used for unstructured user-facing > information like title/author/etc.. It's a terrible mechanism for > other > usecases, because it's an implicit API hidden from the compiler, with > no > type information, stability guarantees, deprecation mechanisms, etc. > Not > to mention it forces users to parse and serialize strings, which is a > massive pain and a constant source of bugs, especially in C. > > > In the very common case where users want to copy PIDs from inputs > > to > > outputs, implementing -map_streamid seems a bit silly. Consider > > also > > the case where the user wants to copy codec and bitrate from some > > source stream, such as when filtering audio. It would be nice if > > such > > operations were handled by a common mechanism. Call it -map_params > > perhaps > > How long until ffmpeg CLI options are turing complete? *cough* lavu/eval.* *cough* But yeah there's probably no way to make everyone happy with something like this. In practice plenty of users maintain their own forks to cover their own obscure use-cases /Tomas ___ 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/9] avcodec/zmbvenc: Remove redundant pixel format check
tis 2021-09-28 klockan 16:41 +0200 skrev Andreas Rheinhardt: > ff_encode_preinit() already checked the pixel format via > AVCodec.pix_fmts. > > Signed-off-by: Andreas Rheinhardt > --- > libavcodec/zmbvenc.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c > index b431476241..8efdbc963e 100644 > --- a/libavcodec/zmbvenc.c > +++ b/libavcodec/zmbvenc.c > @@ -347,9 +347,6 @@ static av_cold int encode_init(AVCodecContext > *avctx) > c->fmt = ZMBV_FMT_32BPP; > c->bypp = 4; > break; > - default: > - av_log(avctx, AV_LOG_INFO, "unsupported pixel format\n"); > - return AVERROR(EINVAL); Sounds OK /Tomas ___ 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] avformat/mxf: support MCA audio information
mån 2021-10-11 klockan 18:32 +0200 skrev Marc-Antoine Arnaud: > --- > libavformat/mxf.h | 1 + > libavformat/mxfdec.c | 276 > ++- > 2 files changed, 271 insertions(+), 6 deletions(-) Did we reach a consensus on this? While I think signalling ffmpeg to create a remap filter is a good idea, I don't know whether we have any mechanism for that at the moment. If we don't then perhaps that shouldn't hold up this patch? /Tomas ___ 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] avformat/mxf: support MCA audio information
mån 2021-10-18 klockan 17:06 +0200 skrev Marc-Antoine Arnaud:
>
> +static int mxf_audio_remapping(int* channel_ordering, uint8_t* data,
> int size, int sample_size, int channels)
> +{
> + int sample_offset = channels * sample_size;
> + int number_of_samples = size / sample_offset;
> + uint8_t* tmp = av_malloc(sample_offset);
You could avoid this allocation using
uint8_t tmp[FF_SANE_NB_CHANNELS*4];
since mxfdec only handles up to 32-bit PCM. Maybe *8 if someone down
the line decides to add support for 64-bit PCM. Not that I understand
why anyone would ever use that.. Don't think SMPTE defines a UL for it.
> + uint8_t* data_ptr = data;
you can just use data
> +
> + if (!tmp)
> + return AVERROR(ENOMEM);
> +
> + for (int sample = 0; sample < number_of_samples; ++sample) {
> + memcpy(tmp, data_ptr, sample_offset);
> +
> + for (int channel = 0; channel < channels; ++channel) {
> + for (int sample_index = 0; sample_index < sample_size;
> ++sample_index) {
> + data_ptr[sample_size * channel_ordering[channel] +
> sample_index] = tmp[sample_size * channel + sample_index];
> + }
why not memcpy()? Should get inlined by any decent compiler I think
/Tomas
___
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] avformat/mxf: support MCA audio information
ons 2021-10-27 klockan 15:50 +0200 skrev Marc-Antoine Arnaud:
> +if(channel_ordering_ptr->service_type !=
> AV_AUDIO_SERVICE_TYPE_NB) {
> +ast = (enum
> AVAudioServiceType*)av_stream_new_side_data(st,
> AV_PKT_DATA_AUDIO_SERVICE_TYPE, sizeof(*ast));
This needs a check for ast == NULL
> +static int mxf_audio_remapping(int* channel_ordering, uint8_t* data,
> int size, int sample_size, int channels)
> +{
> +int sample_offset = channels * sample_size;
> +int number_of_samples = size / sample_offset;
> +uint8_t tmp[FF_SANE_NB_CHANNELS * 4];
> +uint8_t* data_ptr = data;
> +
> +if (!tmp)
> +return AVERROR(ENOMEM);
> +
> +for (int sample = 0; sample < number_of_samples; ++sample) {
> +memcpy(tmp, data_ptr, sample_offset);
> +
> +for (int channel = 0; channel < channels; ++channel) {
> +for (int sample_index = 0; sample_index < sample_size;
> ++sample_index) {
> +data_ptr[sample_size * channel_ordering[channel] +
> sample_index] = tmp[sample_size * channel + sample_index];
> +}
What I meant with my previous comment on this is that the innermost
loop can be replaced with memcpy(), making the code simpler.
/Tomas
___
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] mfxenc add jpeg2000 frame field interlacing support
Looks like you messed up the subject. You need two newlines between the
title of the patch and the description.
This patch also mixes a lot of different changes. Please split it up.
The bigger a patch is the more rounds of review it tends to have to go
through.
> + { 0x840B,
> {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x0B,0x0
> 0,0x00,0x00}}, /* 8+3n bytes : Array of picture components where each
> component comprises 3 bytes named Ssizi, XRSizi, YRSizi The array of
> 3-byte groups is preceded by the array header comprising a 4-byte
> value of the number of components followed by a 4-byte value of �3�.
> */
some kind of encoding problem in the comment
> @@ -843,7 +886,28 @@ static void mxf_write_track(AVFormatContext *s,
> AVStream *st, MXFPackage *packag
>
> mxf_write_metadata_key(pb, 0x013b00);
> PRINT_KEY(s, "track key", pb->buf_ptr - 16);
> - klv_encode_ber_length(pb, 80);
> +
> + if (st->codecpar) {
> + static const char * pcTrackName_Video = "Picture" ;
> + static const char * pcTrackName_Audio = "Sound" ;
> + static const char * pcTrackName_Anc = "Ancillary" ;
static is redundant here
> + if ( st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO )
> + {
> + //TrackName Video
> + klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Video));
> + mxf_write_local_tag_utf16(s, 0x4802 ,
> pcTrackName_Video);
> + } else if ( st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO )
> {
> + //TrackName Audio
> + klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Audio));
> + mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Audio);
> + } else {
> + //TrackName Ancillary
> + klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Anc));
> + mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Anc);
> + }
> + } else {
> + klv_encode_ber_length(pb, 80);
> + }
Is this hunk really necessary?
> @@ -1327,6 +1420,182 @@ static int64_t
> mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
> return pos;
> }
>
> +static int64_t mxf_write_jpeg2000_common(AVFormatContext *s,
> AVStream *st, const UID key)
> +{
> + MXFStreamContext *sc = st->priv_data;
> + MXFContext *mxf = s->priv_data;
> + AVIOContext *pb = s->pb;
> + int stored_width = st->codecpar->width;
> + int stored_height = st->codecpar->height;
> + int display_height;
> + int f1, f2;
> + UID transfer_ul = {0};
> + int64_t pos = mxf_write_generic_desc(s, st, key);
> + get_trc(transfer_ul, st->codecpar->color_trc);
Return value of get_trc() is not checked. Not sure if invalid values
can ever get in here.
> +
> + mxf_write_local_tag(s, 4, 0x3203);
> + avio_wb32(pb, stored_width);
> + mxf_write_local_tag(s, 4, 0x3202);
> + avio_wb32(pb, stored_height);
> +
> + //Sampled width
> + mxf_write_local_tag(s, 4, 0x3205);
> + avio_wb32(pb, st->codecpar->width);
> +
> + //Samples height
> + mxf_write_local_tag(s, 4, 0x3204);
> + avio_wb32(pb, st->codecpar->height);
Why use stored_* and codecpar->*? Just use codecpar->* in both places.
> +
> + //Sampled X Offset
> + mxf_write_local_tag(s, 4, 0x3206);
> + avio_wb32(pb, 0);
> +
> + //Sampled Y Offset
> + mxf_write_local_tag(s, 4, 0x3207);
> + avio_wb32(pb, 0);
> +
> + mxf_write_local_tag(s, 4, 0x3209);
> + avio_wb32(pb, st->codecpar->width);
> +
> + if (st->codecpar->height == 608) // PAL + VBI
> + display_height = 576;
> + else if (st->codecpar->height == 512) // NTSC + VBI
> + display_height = 486;
> + else
> + display_height = st->codecpar->height;
> +
> + mxf_write_local_tag(s, 4, 0x3208);
> + avio_wb32(pb, display_height);
> +
> + // display X offset
> + mxf_write_local_tag(s, 4, 0x320A);
> + avio_wb32(pb, 0);
> +
> + //display Y offset
> + mxf_write_local_tag(s, 4, 0x320B);
> + avio_wb32(pb, (st->codecpar->height - display_height));
Would be better if we could get this information via metadata instead
of adding hacks to the muxer
> + // // Padding Bits
> + // mxf_write_local_tag(s, 2, 0x3307);
> + // avio_wb16(pb, 0);
Stray dead code
> + // video line map
> + {
> + int _field_height = (mxf->mxf_j2kinterlace) ? (2*st-
> >codecpar->height) : st->codecpar->height;
> +
> + if (st->codecpar->sample_aspect_ratio.num && st->codecpar-
> >sample_aspect_ratio.den) {
> + AVRational _ratio = av_mul_q(st->codecpar-
> >sample_aspect_ratio,
> + av_make_q(st->codecpar->width, st-
> >codecpar->height));
> + sc->aspect_ratio = _ratio;
> +
> + switch (_field_height) {
> + case 576: f1 = 23; f2 = st->codecpar->codec_id ==
> AV_CODEC_ID_DVVIDEO ? 335 : 336; break;
> + case 608: f1 = 7; f2 = 320; break;
>
Re: [FFmpeg-devel] [PATCH] avfilter: add wpsnr video filter
tor 2021-10-28 klockan 21:09 +0200 skrev Paul B Mahol:
>
> +FRAMESYNC_DEFINE_CLASS(wpsnr, WPSNRContext, fs);
> +
> +#define COMPUTE_HX(type, stype, depth) \
> +static void compute_hx##depth(const uint8_t *ssrc, \
> + int linesize, \
> + int w, int h, \
> + uint16_t *dstp, \
> + int dst_linesize) \
> +{ \
> + const type *src = (const type *)ssrc; \
> + stype *dst = (stype *)dstp; \
> + \
> + linesize /= (depth / 8); \
Can linesize ever be odd? Probably not, so this should be fine.
> +static double get_hx(const uint8_t *src, int linesize, int w, int h)
> +{
> + int64_t sum = 0;
> +
> + for (int y = 0; y < h; y++) {
> + for (int x = 0; x < w; x++) {
> + sum += 12 * src[x] -
> + 2 * (src[x-1] + src[x+1] +
> + src[x + linesize] +
> + src[x - linesize]) -
> + 1 * (src[x - 1 - linesize] +
> + src[x + 1 - linesize] +
> + src[x - 1 + linesize] +
> + src[x + 1 + linesize]);
> + }
> +
> + src += linesize;
> + }
> +
> + return fabs(sum * 0.25);
> +}
> +
> +static double get_hx16(const uint8_t *ssrc, int linesize, int w, int
> h)
> +{
> + const uint16_t *src = (const uint16_t *)ssrc;
This is not -fstrict-aliasing safe
> + int64_t sum = 0;
> +
> + linesize /= 2;
> +
> + for (int y = 0; y < h; y++) {
> + for (int x = 0; x < w; x++) {
> + sum += 12 * src[x] -
> + 2 * (src[x-1] + src[x+1] +
> + src[x + linesize] +
> + src[x - linesize]) -
> + 1 * (src[x - 1 - linesize] +
> + src[x + 1 - linesize] +
> + src[x - 1 + linesize] +
> + src[x + 1 + linesize]);
> + }
> +
> + src += linesize;
> + }
> +
> + return fabs(sum * 0.25);
> +}
Why not use the same kind of templatization for these as compute_hx*?
> +
> +static double get_sd(const uint8_t *ref, int ref_linesize,
> + const uint8_t *main, int main_linesize,
> + int w, int h)
> +{
> + int64_t sum = 0;
> +
> + for (int y = 0; y < h; y++) {
> + for (int x = 0; x < w; x++)
> + sum += pow_2(ref[x] - main[x]);
> + ref += ref_linesize;
> + main += main_linesize;
> + }
> +
> + return sum;
> +}
> +
> +static double get_sd16(const uint8_t *rref, int ref_linesize,
> + const uint8_t *mmain, int main_linesize,
> + int w, int h)
> +{
> + const uint16_t *ref = (const uint16_t *)rref;
> + const uint16_t *main = (const uint16_t *)mmain;
> + int64_t sum = 0;
> +
> + ref_linesize /= 2;
> + main_linesize /= 2;
> +
> + for (int y = 0; y < h; y++) {
> + for (int x = 0; x < w; x++)
> + sum += pow_2(ref[x] - main[x]);
> + ref += ref_linesize;
> + main += main_linesize;
> + }
> +
> + return sum;
> +}
Same here, and for more functions in the patch it seems so I'm not
going to bother repeating myself any more
> +static void set_meta(AVDictionary **metadata, const char *key, char
> comp, float d)
> +{
> + char value[128];
> + snprintf(value, sizeof(value), "%f", d);
> + if (comp) {
> + char key2[128];
> + snprintf(key2, sizeof(key2), "%s%c", key, comp);
> + av_dict_set(metadata, key2, value, 0);
> + } else {
> + av_dict_set(metadata, key, value, 0);
> + }
> +}
We should probably add av_dict_set_* for int, double etc at some point
/Tomas
___
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] avfilter: add wpsnr video filter
fre 2021-10-29 klockan 15:33 +0200 skrev Paul B Mahol:
> On Fri, Oct 29, 2021 at 3:12 PM Tomas Härdin
> wrote:
> >
> > > +static double get_hx(const uint8_t *src, int linesize, int w,
> > > int h)
> > > +{
> > > + int64_t sum = 0;
> > > +
> > > + for (int y = 0; y < h; y++) {
> > > + for (int x = 0; x < w; x++) {
> > > + sum += 12 * src[x] -
> > > + 2 * (src[x-1] + src[x+1] +
> > > + src[x + linesize] +
> > > + src[x - linesize]) -
> > > + 1 * (src[x - 1 - linesize] +
> > > + src[x + 1 - linesize] +
> > > + src[x - 1 + linesize] +
> > > + src[x + 1 + linesize]);
> > > + }
> > > +
> > > + src += linesize;
> > > + }
> > > +
> > > + return fabs(sum * 0.25);
> > > +}
> > > +
> > > +static double get_hx16(const uint8_t *ssrc, int linesize, int w,
> > > int
> > > h)
> > > +{
> > > + const uint16_t *src = (const uint16_t *)ssrc;
> >
> > This is not -fstrict-aliasing safe
> >
>
> How so? I get no warnings at all, and similar is used everywhere
> else.
Then those places should be fixed. We have macros like AV_RB16() for a
reason. That gcc doesn't warn about this doesn't mean it isn't free to
assume ssrc and src points to different non-overlapping regions of
memory.
We could disable strict aliasing, but that will likely hurt performance
of other parts of the code.
/Tomas
___
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] avfilter: add wpsnr video filter
fre 2021-10-29 klockan 17:00 +0200 skrev Paul B Mahol:
> On Fri, Oct 29, 2021 at 4:46 PM Tomas Härdin
> wrote:
>
> > fre 2021-10-29 klockan 15:33 +0200 skrev Paul B Mahol:
> > > On Fri, Oct 29, 2021 at 3:12 PM Tomas Härdin
> > > wrote:
> > > >
> > > > > +static double get_hx(const uint8_t *src, int linesize, int
> > > > > w,
> > > > > int h)
> > > > > +{
> > > > > + int64_t sum = 0;
> > > > > +
> > > > > + for (int y = 0; y < h; y++) {
> > > > > + for (int x = 0; x < w; x++) {
> > > > > + sum += 12 * src[x] -
> > > > > + 2 * (src[x-1] + src[x+1] +
> > > > > + src[x + linesize] +
> > > > > + src[x - linesize]) -
> > > > > + 1 * (src[x - 1 - linesize] +
> > > > > + src[x + 1 - linesize] +
> > > > > + src[x - 1 + linesize] +
> > > > > + src[x + 1 + linesize]);
> > > > > + }
> > > > > +
> > > > > + src += linesize;
> > > > > + }
> > > > > +
> > > > > + return fabs(sum * 0.25);
> > > > > +}
> > > > > +
> > > > > +static double get_hx16(const uint8_t *ssrc, int linesize,
> > > > > int w,
> > > > > int
> > > > > h)
> > > > > +{
> > > > > + const uint16_t *src = (const uint16_t *)ssrc;
> > > >
> > > > This is not -fstrict-aliasing safe
> > > >
> > >
> > > How so? I get no warnings at all, and similar is used everywhere
> > > else.
> >
> > Then those places should be fixed. We have macros like AV_RB16()
> > for a
> > reason. That gcc doesn't warn about this doesn't mean it isn't free
> > to
> > assume ssrc and src points to different non-overlapping regions of
> > memory.
> >
> >
> That is sub optimal and unacceptable solution.
Did you do measurements to come to this conclusion?
> Review remark ignored.
Undefined behavior is *not* acceptable. If you want this file
specifically to be compiled with strict aliasing disabled then you must
at the very least change the build system accordingly
Type punning via union seems to be defined as of C99, so that should
also be acceptable
/Tomas
___
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] avfilter: add wpsnr video filter
fre 2021-10-29 klockan 19:17 +0200 skrev Paul B Mahol:
> On Fri, Oct 29, 2021 at 6:59 PM Tomas Härdin
> wrote:
>
> > fre 2021-10-29 klockan 17:00 +0200 skrev Paul B Mahol:
> > > On Fri, Oct 29, 2021 at 4:46 PM Tomas Härdin
> > > wrote:
> > >
> > > > fre 2021-10-29 klockan 15:33 +0200 skrev Paul B Mahol:
> > > > > On Fri, Oct 29, 2021 at 3:12 PM Tomas Härdin <
> > > > > tjop...@acc.umu.se>
> > > > > wrote:
> > > > > >
> > > > > > > +static double get_hx(const uint8_t *src, int linesize,
> > > > > > > int
> > > > > > > w,
> > > > > > > int h)
> > > > > > > +{
> > > > > > > + int64_t sum = 0;
> > > > > > > +
> > > > > > > + for (int y = 0; y < h; y++) {
> > > > > > > + for (int x = 0; x < w; x++) {
> > > > > > > + sum += 12 * src[x] -
> > > > > > > + 2 * (src[x-1] + src[x+1] +
> > > > > > > + src[x + linesize] +
> > > > > > > + src[x - linesize]) -
> > > > > > > + 1 * (src[x - 1 - linesize] +
> > > > > > > + src[x + 1 - linesize] +
> > > > > > > + src[x - 1 + linesize] +
> > > > > > > + src[x + 1 + linesize]);
> > > > > > > + }
> > > > > > > +
> > > > > > > + src += linesize;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return fabs(sum * 0.25);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static double get_hx16(const uint8_t *ssrc, int
> > > > > > > linesize,
> > > > > > > int w,
> > > > > > > int
> > > > > > > h)
> > > > > > > +{
> > > > > > > + const uint16_t *src = (const uint16_t *)ssrc;
> > > > > >
> > > > > > This is not -fstrict-aliasing safe
> > > > > >
> > > > >
> > > > > How so? I get no warnings at all, and similar is used
> > > > > everywhere
> > > > > else.
> > > >
> > > > Then those places should be fixed. We have macros like
> > > > AV_RB16()
> > > > for a
> > > > reason. That gcc doesn't warn about this doesn't mean it isn't
> > > > free
> > > > to
> > > > assume ssrc and src points to different non-overlapping regions
> > > > of
> > > > memory.
> > > >
> > > >
> > > That is sub optimal and unacceptable solution.
> >
> > Did you do measurements to come to this conclusion?
> >
> > > Review remark ignored.
> >
> > Undefined behavior is *not* acceptable. If you want this file
> > specifically to be compiled with strict aliasing disabled then you
> > must
> > at the very least change the build system accordingly
> >
>
> Cite source that can prove this.
The gcc manpage
> Compilers are silent about this.
Irrelevant
> There are hundredths if not thousands of such examples across
> codebase.
This is not a valid excuse. We need less UB in the codebase, not more.
UBs beget CVEs.
/Tomas
___
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] avfilter: add wpsnr video filter
fre 2021-10-29 klockan 19:43 +0200 skrev Paul B Mahol:
> On Fri, Oct 29, 2021 at 7:26 PM Tomas Härdin
> wrote:
>
> > fre 2021-10-29 klockan 19:17 +0200 skrev Paul B Mahol:
> > > On Fri, Oct 29, 2021 at 6:59 PM Tomas Härdin
> > > wrote:
> > >
> > > > fre 2021-10-29 klockan 17:00 +0200 skrev Paul B Mahol:
> > > > > On Fri, Oct 29, 2021 at 4:46 PM Tomas Härdin <
> > > > > tjop...@acc.umu.se>
> > > > > wrote:
> > > > >
> > > > > > fre 2021-10-29 klockan 15:33 +0200 skrev Paul B Mahol:
> > > > > > > On Fri, Oct 29, 2021 at 3:12 PM Tomas Härdin <
> > > > > > > tjop...@acc.umu.se>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > +static double get_hx(const uint8_t *src, int
> > > > > > > > > linesize,
> > > > > > > > > int
> > > > > > > > > w,
> > > > > > > > > int h)
> > > > > > > > > +{
> > > > > > > > > + int64_t sum = 0;
> > > > > > > > > +
> > > > > > > > > + for (int y = 0; y < h; y++) {
> > > > > > > > > + for (int x = 0; x < w; x++) {
> > > > > > > > > + sum += 12 * src[x] -
> > > > > > > > > + 2 * (src[x-1] + src[x+1] +
> > > > > > > > > + src[x + linesize] +
> > > > > > > > > + src[x - linesize]) -
> > > > > > > > > + 1 * (src[x - 1 - linesize] +
> > > > > > > > > + src[x + 1 - linesize] +
> > > > > > > > > + src[x - 1 + linesize] +
> > > > > > > > > + src[x + 1 + linesize]);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + src += linesize;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + return fabs(sum * 0.25);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static double get_hx16(const uint8_t *ssrc, int
> > > > > > > > > linesize,
> > > > > > > > > int w,
> > > > > > > > > int
> > > > > > > > > h)
> > > > > > > > > +{
> > > > > > > > > + const uint16_t *src = (const uint16_t *)ssrc;
> > > > > > > >
> > > > > > > > This is not -fstrict-aliasing safe
> > > > > > > >
> > > > > > >
> > > > > > > How so? I get no warnings at all, and similar is used
> > > > > > > everywhere
> > > > > > > else.
> > > > > >
> > > > > > Then those places should be fixed. We have macros like
> > > > > > AV_RB16()
> > > > > > for a
> > > > > > reason. That gcc doesn't warn about this doesn't mean it
> > > > > > isn't
> > > > > > free
> > > > > > to
> > > > > > assume ssrc and src points to different non-overlapping
> > > > > > regions
> > > > > > of
> > > > > > memory.
> > > > > >
> > > > > >
> > > > > That is sub optimal and unacceptable solution.
> > > >
> > > > Did you do measurements to come to this conclusion?
> > > >
> > > > > Review remark ignored.
> > > >
> > > > Undefined behavior is *not* acceptable. If you want this file
> > > > specifically to be compiled with strict aliasing disabled then
> > > > you
> > > > must
> > > > at the very least change the build system accordingly
> > > >
> > >
> > > Cite source that can prove this.
> >
> > The gcc manpage
> >
>
> citations needed.
>
>
> >
> > > Compilers are silent about this.
> >
> > Irrelevant
> >
>
>
> citations needed.
>
>
> >
> > > There are hundredths if not thousands of such examples across
> > > codebase.
> >
> > This is not a valid excuse. We need less UB in the codebase, not
> > more.
> > UBs beget CVEs.
> >
>
> citations needed.
Childish behavior doesn't make you right, Paul.
/Tomas
___
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] avfilter: add wpsnr video filter
fre 2021-10-29 klockan 21:17 -0400 skrev Ronald S. Bultje: > Hi Thomas, > > On Fri, Oct 29, 2021 at 9:12 AM Tomas Härdin > wrote: > > > tor 2021-10-28 klockan 21:09 +0200 skrev Paul B Mahol: > > > + const uint16_t *src = (const uint16_t *)ssrc; > > > > This is not -fstrict-aliasing safe > > > > I don't believe that is correct. It's true we're not allowed to cast > between two clashing types to access the same pointer (memory > location), > but the C standard would appear to make an exception for byte types. > > Quoted from https://stackoverflow.com/a/99010/4726410 because I'm too > lazy > to dig through the standard: > > "The types that C 2011 6.5 7 allows an lvalue to access are: > - a type compatible with the effective type of the object, > - a qualified version of a type compatible with the effective type of > the > object, > - a type that is the signed or unsigned type corresponding to the > effective > type of the object, > - a type that is the signed or unsigned type corresponding to a > qualified > version of the effective type of the object, > - an aggregate or union type that includes one of the aforementioned > types > among its members (including, recursively, a member of a subaggregate > or > contained union), or > - a character type." > > uint8_t is a variant of the character (aka byte) type, and so the > cast > would not seem to violate strict aliasing rules. Maybe we should upgrade to C11 then? This gives us access to more useful language features. Type-generic expressions look very useful /Tomas ___ 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] avfilter: add wpsnr video filter
lör 2021-10-30 klockan 10:28 -0400 skrev Ronald S. Bultje: > Hi, > > On Sat, Oct 30, 2021 at 4:57 AM Tomas Härdin > wrote: > > > Maybe we should upgrade to C11 then? This gives us access to more > > useful language features. Type-generic expressions look very useful > > > > https://stackoverflow.com/a/7005988 (same thread, further down) > appears to > suggest the exact same literal wording exists in C99. No upgrade is > necessary. Ah. Well disregard my ramblings then (: /Tomas ___ 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] avformat/mxf: support MCA audio information
> + if(channel_ordering_ptr->service_type !=
> AV_AUDIO_SERVICE_TYPE_NB) {
> + ast = (enum
> AVAudioServiceType*)av_stream_new_side_data(st,
> AV_PKT_DATA_AUDIO_SERVICE_TYPE, sizeof(*ast));
ast == NULL still needs handling here
I don't see anything else that needs fixing, except maybe splitting the
actual reordering into a separate patch as (IIRC) Marton wants
/Tomas
___
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] avformat/mxf: support MCA audio information
sön 2021-11-07 klockan 12:32 +0100 skrev Marton Balint:
>
> > +
> > + while (channel_ordering_ptr->uid[0]) {
> > + if (IS_KLV_KEY(channel_ordering_ptr->uid,
> > mca_sub_descriptor->mca_label_dictionary_id)) {
>
> You should check if current_channel < desciptor->channels here, and
> if
> not, then warn the user and break out of the loop. Otherwise
> current_channel can grow out of array limits.
>
> It should also be checked that channel_ordering_ptr->index <
> descriptor->channels, and if not, then similarly, warn the user and
> break
> out.
>
> Maybe a hard failure (returning AVERROR_INVALIDDATA) is not
> necessary, to
> allow some slightly invalid metadata?
We should be as strict as we can get away with. Else we encourage
laxness in other implementations.
/Tomas
___
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 01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed
tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt: > The mxf and mxf_opatom muxer expect H.264 in Annex B format. > > Signed-off-by: Andreas Rheinhardt > --- > The check here is taken from mpegtsenc. You didn't think to make both muxers share code instead of copy- pasting? /Tomas ___ 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 02/17] fate/mxf: Add ProRes remux test
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt: > Also covers writing mastering display metadata. So you're merging the D-10 user comments test and ProRes remuxing? What about remuxing D-10? /Tomas ___ 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 04/17] avformat/mxfenc: Use smaller types to make struct smaller
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt
> ---
> libavformat/mxfenc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index d1c4d43a50..3b6604d0d6 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2203,9 +2203,9 @@ static int mxf_parse_dv_frame(AVFormatContext
> *s, AVStream *st, AVPacket *pkt)
> static const struct {
> UID uid;
> int frame_size;
> - int profile;
> + uint8_t profile;
> uint8_t interlaced;
> - int intra_only; // 1 or 0 when there are separate UIDs for Long
> GOP and Intra, -1 when Intra/LGOP detection can be ignored
> + int8_t intra_only; // 1 or 0 when there are separate UIDs for
Looks OK, and should work as intended since they're at the end of the
struct.
/Tomas
___
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 05/17] avformat/mxfenc: Remove redundant check
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> None of the muxers here has the AVFMT_NOSTREAMS flag set,
> so it is checked generically that there are streams.
Didn't know about AVFMT_NOSTREAMS
>
> Signed-off-by: Andreas Rheinhardt
> ---
> libavformat/mxfenc.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 3b6604d0d6..fd9e2c4c48 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2507,9 +2507,6 @@ static int mxf_write_header(AVFormatContext *s)
> uint8_t present[FF_ARRAY_ELEMS(mxf_essence_container_uls)] =
> {0};
> int64_t timestamp = 0;
>
> - if (!s->nb_streams)
> - return -1;
Looks OK
/Tomas
___
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 06/17] avformat/mxfenc: Make init function out of write_header
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt: > The MXF muxers only write the header after they have received > a packet; the actual write_header function does not write anything. > So make an init function out of it. New API being put to good use. Patch looks OK We could write *some* output if we really wanted to, like the header partition key and klv fill. Not very useful however. /Tomas ___ 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 07/17] avformat/mxfenc: Error out when receiving invalid data
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt: > (Unless the packet has a size of zero, the packet will run afoul > of the cbr_index check a few lines below.) > > Signed-off-by: Andreas Rheinhardt > --- > libavformat/mxfenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c > index a6535eb43f..c20ba9bfca 100644 > --- a/libavformat/mxfenc.c > +++ b/libavformat/mxfenc.c > @@ -2151,7 +2151,7 @@ static int mxf_parse_dv_frame(AVFormatContext > *s, AVStream *st, AVPacket *pkt) > > // Check for minimal frame size > if (pkt->size < 12) > - return -1; > + return 0; Might be nicer to have negative return value mean error, just like everywhere else, and change the code further down accordingly. -1 is a crappy return value for mxf_write_packet() I think. But then the other parsing functions should change at the same time. So let's not hold this patch up with that. Looks OK for now. /Tomas ___ 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 08/17] avformat/mxfenc: Improve returned error codes
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt: > Signed-off-by: Andreas Rheinhardt > --- > libavformat/mxfenc.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) Aha, this one does exactly what I just suggested with return values. D'oh! Looks OK of course /Tomas ___ 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 09/17] avformat/mxfenc: Avoid allocation for timecode track
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt
> ---
> libavformat/mxfenc.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index cf63340313..aa9857fcff 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -421,6 +421,7 @@ typedef struct MXFContext {
> int track_instance_count; // used to generate MXFTrack uuids
> int cbr_index; ///< use a constant bitrate index
> uint8_t unused_tags[MXF_NUM_TAGS]; ///< local tags that we know
> will not be used
> + MXFStreamContext timecode_track_priv;
> } MXFContext;
>
> static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType
> type, int value)
> @@ -2712,9 +2713,7 @@ static int mxf_init(AVFormatContext *s)
> mxf->timecode_track = av_mallocz(sizeof(*mxf->timecode_track));
> if (!mxf->timecode_track)
> return AVERROR(ENOMEM);
> - mxf->timecode_track->priv_data =
> av_mallocz(sizeof(MXFStreamContext));
> - if (!mxf->timecode_track->priv_data)
> - return AVERROR(ENOMEM);
> + mxf->timecode_track->priv_data = &mxf->timecode_track_priv;
> mxf->timecode_track->index = -1;
>
> return 0;
> @@ -3087,10 +3086,7 @@ static void mxf_deinit(AVFormatContext *s)
>
> av_freep(&mxf->index_entries);
> av_freep(&mxf->body_partition_offset);
> - if (mxf->timecode_track) {
> - av_freep(&mxf->timecode_track->priv_data);
> - av_freep(&mxf->timecode_track);
> - }
> + av_freep(&mxf->timecode_track);
> }
>
> static int mxf_interleave_get_packet(AVFormatContext *s, AVPacket
> *out, int flush)
Looks OK. We never have more than one timecode track.
/Tomas
___
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 10/17] avformat/mxfdec: Simplify data->hex string conversion
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt
> ---
> libavformat/mxfdec.c | 13 +++--
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index af9d33f796..4191e82474 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1984,22 +1984,15 @@ static int mxf_uid_to_str(UID uid, char
> **str)
>
> static int mxf_umid_to_str(UID ul, UID uid, char **str)
> {
> - int i;
> char *p;
> p = *str = av_mallocz(sizeof(UID) * 4 + 2 + 1);
> if (!p)
> return AVERROR(ENOMEM);
> snprintf(p, 2 + 1, "0x");
Could use strncpy() while you're at it.
/Tomas
___
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 11/17] avformat/mxfenc: Store locally whether DNXHD profile is interlaced
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> It is just a flag per supported CID. So there is no reason to use
> an avpriv function for this purpose.
This is data duplication. Honestly these ULs should probably live in
dnxhddata.c.
>
> Signed-off-by: Andreas Rheinhardt
> ---
> libavformat/mxfenc.c | 47 ++
> --
> 1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index aa9857fcff..326ec6a7d6 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2036,29 +2036,30 @@ static int
> mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket *pk
> }
>
> static const struct {
> - int cid;
> + uint16_t cid;
> + uint8_t interlaced;
> UID codec_ul;
> } mxf_dnxhd_codec_uls[] = {
Not sure if the narrowing of types here does any good. You might need
to put cid and interlaced after codec_ul. On the other hand UID is
uint8_t[] so perhaps it works out.
/Tomas
___
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 11/17] avformat/mxfenc: Store locally whether DNXHD profile is interlaced
tis 2021-11-09 klockan 22:48 +0100 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> > > It is just a flag per supported CID. So there is no reason to use
> > > an avpriv function for this purpose.
> >
> > This is data duplication. Honestly these ULs should probably live
> > in
> > dnxhddata.c.
> >
>
> But aren't these ULs mxf-specific? So the right place for them is
> here
> in libavformat.
> And the amount of data duplicated is trivial; furthermore adding a
> new
> DNXHD profile then and now requires modifications in two tables, so I
> don't see a maintenance burden either.
Right, this improves lavc/lavf separation. I suppose that's OK. Still
not the biggest fan of having this kind of data in more than one place,
but on the other hand it's constants..
> > >
> > > Signed-off-by: Andreas Rheinhardt
> > >
> > > ---
> > > libavformat/mxfenc.c | 47 ++
> > >
> > > --
> > > 1 file changed, 23 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index aa9857fcff..326ec6a7d6 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -2036,29 +2036,30 @@ static int
> > > mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *pk
> > > }
> > >
> > > static const struct {
> > > - int cid;
> > > + uint16_t cid;
> > > + uint8_t interlaced;
> > > UID codec_ul;
> > > } mxf_dnxhd_codec_uls[] = {
> >
> > Not sure if the narrowing of types here does any good. You might
> > need
> > to put cid and interlaced after codec_ul. On the other hand UID is
> > uint8_t[] so perhaps it works out.
> >
> The narrowing is irrelevant, as all cid values in use fit into an
> uint16_t.
> Why would I need to put it at the end? Do you worry about padding
> between interlaced and codec_ul? In this case it doesn't matter: It
> would just be a matter of whether the padding is in the middle or at
> the
> end of the structure.
I don't actually care. I just thought it was curious given patch 04
/Tomas
___
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 01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed
tis 2021-11-09 klockan 22:07 +0100 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt: > > > The mxf and mxf_opatom muxer expect H.264 in Annex B format. > > > > > > Signed-off-by: Andreas Rheinhardt > > > > > > --- > > > The check here is taken from mpegtsenc. > > > > You didn't think to make both muxers share code instead of copy- > > pasting? > > > > Well, I can share it. The problem is just that I didn't really know > where it belongs: mux.c? utils.c? A new file? A new file probably, say libavformat/annexb.c Do we ever need to be able to do this kind of stuff in lavc? In that case it could maybe live there. But that again increases coupling between the two libs.. /Tomas ___ 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 13/17] avformat/mxfenc: Remove redundant DNXHD frame size checks
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> The actual frame_size is no longer used since commit
> 3d38e45eb85c7a2420cb48a9cd45625c28644b2e; and the check for
> "< 0" is equivalent to the CID being valid. But this is already
> ensured by mxf_dnxhd_codec_uls containing this CID.
>
> Signed-off-by: Andreas Rheinhardt
> ---
> libavformat/mxfenc.c | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 326ec6a7d6..83f9a778fe 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2066,7 +2066,7 @@ static int
> mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt
> {
> MXFContext *mxf = s->priv_data;
> MXFStreamContext *sc = st->priv_data;
> - int i, cid, frame_size = 0;
> + int i, cid;
>
> if (mxf->header_written)
> return 1;
> @@ -2094,12 +2094,6 @@ static int
> mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt
> if (!sc->component_depth)
> return 0;
>
> - if ((frame_size = avpriv_dnxhd_get_frame_size(cid)) ==
> DNXHD_VARIABLE) {
> - frame_size = avpriv_dnxhd_get_hr_frame_size(cid, st-
> >codecpar->width, st->codecpar->height);
> - }
> - if (frame_size < 0)
> - return 0;
Looks simple enough. Might want Baptiste to chime in given that he
authored 3d38e45.
/Tomas
___
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 03/17] fate/mxf: Add tests for H.264 remuxing
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt: > These tests exhibit two bugs: Instead of using the in-band extradata > the demuxer makes up some extradata designed for AVC intra tracks > that lack in-band extradata; these files are nevertheless decodable > because of the in-band extradata. Furthermore, the frame reordering > is lost. > > Signed-off-by: Andreas Rheinhardt > --- > tests/fate/mxf.mak | 22 ++- > tests/ref/fate/mxf-remux-h264 | 37 ++ > tests/ref/fate/mxf-remux-xavc | 71 > +++ > 3 files changed, 128 insertions(+), 2 deletions(-) > create mode 100644 tests/ref/fate/mxf-remux-h264 > create mode 100644 tests/ref/fate/mxf-remux-xavc > > diff --git a/tests/fate/mxf.mak b/tests/fate/mxf.mak > index f96f4a429b..58a697cd86 100644 > --- a/tests/fate/mxf.mak > +++ b/tests/fate/mxf.mak > @@ -42,6 +42,21 @@ FATE_MXF_REMUX_PROBE-$(call ALLYES, PRORES_DECODER > MXF_MUXER) \ > += fate-mxf-remux-applehdr10 > fate-mxf-remux-applehdr10: CMD = transcode mxf > $(TARGET_SAMPLES)/mxf/Meridian-Apple_ProResProxy-HDR10.mxf mxf "-map > 0 -c copy" "-c copy -t 0.3" "" "-show_entries > format_tags:stream_side_data_list:stream=index,codec_name,codec_tag:s > tream_tags" > > +# Tests muxing H.264, in particular automatic insertion of > h264_mp4toannexb. > +# FIXME: The timestamps of the demuxed file are not properly > reordered. > +# Furthermore the extradata is wrong: It is one of the AVC intra > SPS/PPS; > +# decoding only works due to in-band extradata. Is this a problem with the samples or the code? Or both? /Tomas ___ 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 01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed
ons 2021-11-10 klockan 14:21 +0100 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2021-11-09 klockan 22:07 +0100 skrev Andreas Rheinhardt: > > > Tomas Härdin: > > > > tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt: > > > > > The mxf and mxf_opatom muxer expect H.264 in Annex B format. > > > > > > > > > > Signed-off-by: Andreas Rheinhardt > > > > > > > > > > --- > > > > > The check here is taken from mpegtsenc. > > > > > > > > You didn't think to make both muxers share code instead of > > > > copy- > > > > pasting? > > > > > > > > > > Well, I can share it. The problem is just that I didn't really > > > know > > > where it belongs: mux.c? utils.c? A new file? > > > > A new file probably, say libavformat/annexb.c > > > > Do we ever need to be able to do this kind of stuff in lavc? In > > that > > case it could maybe live there. But that again increases coupling > > between the two libs.. > > > > lavc does not have AVStreams or AVFormatContexts, so sharing code > would > be absolutely impossible anyway. But one can share ideas: Using a bsf > to > prepare data for a decoder is also done for certain decoders; see > AVCodec.bsfs. These bitstream filters are applied unconditionally and > are therefore supposed to detect themselves whether they should do > something or whether the data already has the desired form. I am > unsure > whether this is a better approach; doing the same in lavf would add a > problem that lavc does not have: There would be a copy of every > non-refcounted packet when using av_write_frame(). Just noting here that it might be that we shouldn't even insert a BSF. See my comment on patch 03. /Tomas ___ 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 03/17] fate/mxf: Add tests for H.264 remuxing
ons 2021-11-10 klockan 14:52 +0100 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt: > > > These tests exhibit two bugs: Instead of using the in-band > > > extradata > > > the demuxer makes up some extradata designed for AVC intra tracks > > > that lack in-band extradata; these files are nevertheless > > > decodable > > > because of the in-band extradata. Furthermore, the frame > > > reordering > > > is lost. > > > > > > Signed-off-by: Andreas Rheinhardt > > > > > > --- > > > tests/fate/mxf.mak | 22 ++- > > > tests/ref/fate/mxf-remux-h264 | 37 ++ > > > tests/ref/fate/mxf-remux-xavc | 71 > > > +++ > > > 3 files changed, 128 insertions(+), 2 deletions(-) > > > create mode 100644 tests/ref/fate/mxf-remux-h264 > > > create mode 100644 tests/ref/fate/mxf-remux-xavc > > > > > > diff --git a/tests/fate/mxf.mak b/tests/fate/mxf.mak > > > index f96f4a429b..58a697cd86 100644 > > > --- a/tests/fate/mxf.mak > > > +++ b/tests/fate/mxf.mak > > > @@ -42,6 +42,21 @@ FATE_MXF_REMUX_PROBE-$(call ALLYES, > > > PRORES_DECODER > > > MXF_MUXER) \ > > > += fate-mxf-remux-applehdr10 > > > fate-mxf-remux-applehdr10: CMD = transcode mxf > > > $(TARGET_SAMPLES)/mxf/Meridian-Apple_ProResProxy-HDR10.mxf mxf "- > > > map > > > 0 -c copy" "-c copy -t 0.3" "" "-show_entries > > > format_tags:stream_side_data_list:stream=index,codec_name,codec_t > > > ag:s > > > tream_tags" > > > > > > +# Tests muxing H.264, in particular automatic insertion of > > > h264_mp4toannexb. > > > +# FIXME: The timestamps of the demuxed file are not properly > > > reordered. > > > +# Furthermore the extradata is wrong: It is one of the AVC intra > > > SPS/PPS; > > > +# decoding only works due to in-band extradata. > > > > Is this a problem with the samples or the code? Or both? > > > > The extradata issue is due to a bug in the demuxer: It always adds > extradata for H.264 instead of letting extract_extradata_bsf extract > it > from the bitstream. See these lines here: > https://github.com/FFmpeg/FFmpeg/blob/447cf537746cd9969674ebbd60411b6093603c59/libavformat/mxfdec.c#L2711-L2718 > The solution for this is of course to detect whether we are dealing > with > AVC intra data and only adding the extradata in this case. What is > the > right way to check for this? Is it the presence of the H.264 ul in > mxf_intra_only_picture_essence_coding_uls? (Another way would be to > check whether the first packet has extradata, but this is probably > more > involved (we would basically have to reimplement/reuse a part of > avformat_find_stream_info() (the part that deals with > extract_extradata) > ourselves).) > I don't know whether the timestamps are due to a bug in the muxer or > the > demuxer, but the timestamps the muxer receives are properly > reordered. Answers to questions like these are in the official SMPTE mapping documents. We should not invent our own hacks. Typically a UL is used to signal this, yes. For AVC-Intra, PPS and SPS seem to be implicit, hence ff_generate_avci_extradata(). For other AVC/H.264 mappings it might be that there's a metadata KLV that stores the extradata, or extradata are to be parsed from the essence. Again, the mapping document should specify this. Do *not* add hacks that try to autodetect this stuff. That just encourages even more hacks and deviations from the standards. Hacks beget hacks. At some point we might have to go through mxfdec.c and limit already existing hacks to only apply to files written by specific MXF encoders. /Tomas ___ 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] avformat/mxfenc: fix DNxHD GC ULs
tis 2021-11-30 klockan 10:22 +0100 skrev Nicolas Gaullier:
Fix GC container ul.
Fix GC element type both for the generic case and for OPAtom.
Thanks to Philip de Nier
for checking the values, especially for OPAtom.
---
libavformat/mxfenc.c | 8 ++--
tests/ref/lavf/mxf_opatom | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index fcd9afda2a..38de3d1ab5 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -32,6 +32,7 @@
* SMPTE 379M MXF Generic Container
* SMPTE 381M Mapping MPEG Streams into the MXF Generic Container
* SMPTE 422M Mapping JPEG 2000 Codestreams into the MXF Generic
Container
+ * SMPTE ST2019-4 Mapping VC-3 Coding Units into the MXF Generic
Container
* SMPTE RP210: SMPTE Metadata Dictionary
* SMPTE RP224: Registry of SMPTE Universal Labels
*/
@@ -181,8 +182,8 @@ static const MXFContainerEssenceEntry
mxf_essence_container_uls[] = {
{
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x02,0x00,0
x00,0x00 },
mxf_write_cdci_desc },
// DNxHD
- { {
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x11,0
x01,0x00 },
- {
0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x15,0x01,0
x05,0x00 },
+ { {
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x0D,0x01,0x03,0x01,0x02,0x11,0
x01,0x00 },
+ {
0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x15,0x01,0
x0C,0x00 },
{
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x02,0x71,0x01,0
x00,0x00 },
Please add a reference to the relevant SMPTE document in the comment,
or perhaps at the list of references at the start of the file
/Tomas
___
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] avformat/mxfenc: fix static building
tis 2023-09-19 klockan 17:12 +0200 skrev Michael Riedl: > MXF muxer requires rangecoder otherwise static linking fails. > > Signed-off-by: Michael Riedl Pushed /Tomas ___ 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 1/2] avformat/mxfdec: Check klv offset
ons 2023-10-18 klockan 02:49 +0200 skrev Michael Niedermayer:
> Fixes: Assertion klv_offset >= mxf->run_in failed at
> libavformat/mxfdec.c:736
> Fixes: 62936/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 5778404366221312.fuzz
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer
> ---
> libavformat/mxfdec.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 68939091e6..f2ec508b72 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -458,12 +458,15 @@ static int mxf_read_sync(AVIOContext *pb, const
> uint8_t *key, unsigned size)
> return i == size;
> }
>
> -static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
> +static int klv_read_packet(MXFContext *mxf, KLVPacket *klv,
> AVIOContext *pb)
> {
> int64_t length, pos;
> if (!mxf_read_sync(pb, mxf_klv_key, 4))
> return AVERROR_INVALIDDATA;
> klv->offset = avio_tell(pb) - 4;
> + if (klv->offset < mxf->run_in)
One stray space in there which of course can be fixed when pushing
Looks OK
/Tomas
___
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] avcodec/mlp*: improvements
> if (c) {
> e[0] = 1 << 14;
> e[1] = 0 << 14;
> e[2] = v[1];
> e[3] = v[0];
> } else {
> e[0] = v[0];
> e[1] = v[1];
> e[2] = 0 << 14;
> e[3] = 1 << 14;
> }
>
> if (invert2x2(e, d)) {
> sum = UINT64_MAX;
> goto next;
> }
>
You can make use of the properties of e to simplify calculating the
inverse. The determinant is always v[0]<<14, so you can just do if
(!v[0]) continue; and skip the determinant check altogether.
> if (d[i] != av_clip_intp2(d[i], 15)) {
d[i] < INT16_MIN || d[i] > INT16_MAX is more clear and probably faster
> +lt = ((lm * e[0]) >> 14) + ((rm * e[1]) >> 14);
> +rt = ((lm * e[2]) >> 14) + ((rm * e[3]) >> 14);
Result is implementation-defined. Use division by (1<<14). Also add
then divide. The intermediate result is 49 bits so fits easily in 64
bits.
You could also simplify this calculation by again making use of the
properties of e.
> if (c)
> v += FFABS(rt);
> else
> v += FFABS(lt);
> sum += v;
> if (sum > best_sum)
> goto next;
Seems like this reduces to solving a linear program.
> if lt * d[0]) >> 14) + ((rt * d[1]) >> 14))
> != lm) {
> sum = UINT64_MAX;
> goto next;
> }
>
> if lt * d[2]) >> 14) + ((rt * d[3]) >> 14))
> != rm) {
> sum = UINT64_MAX;
> goto next;
> }
Looks like a massive hack. I'd prefer to formally verify that the
arithmetic works out. Also again you can make use of the properties of
e, or inv(e) as it were.
/Tomas
___
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] avcodec/mlp*: improvements
On Wed, 2023-10-25 at 21:00 +0200, Paul B Mahol wrote:
> On Wed, Oct 25, 2023 at 8:39 PM Tomas Härdin wrote:
>
> >
> > > if (c) {
> > > e[0] = 1 << 14;
> > > e[1] = 0 << 14;
> > > e[2] = v[1];
> > > e[3] = v[0];
> > > } else {
> > > e[0] = v[0];
> > > e[1] = v[1];
> > > e[2] = 0 << 14;
> > > e[3] = 1 << 14;
> > > }
> > >
> > > if (invert2x2(e, d)) {
> > > sum = UINT64_MAX;
> > > goto next;
> > > }
> > >
> >
> > You can make use of the properties of e to simplify calculating the
> > inverse. The determinant is always v[0]<<14, so you can just do if
> > (!v[0]) continue; and skip the determinant check altogether.
> >
>
> Even for real 2x2 matrix case? (Once one of rows is not 1, 0) ?
> May added such cases later.
You can just work the math out on paper. Inverse of
1 0
v[1] v[0]
is
1 0
-v[1]/v[0] 1/v[0]
not accounting for shifts.
Also RE: my other comments, you are right. I didn't take into account
that MLP is lossless and that there may be off-by-one errors.
And as I said on IRC you can formulate this as a least squares problem,
then solve it using a linear system solve. This patch seems finds a
solution that minimizes L1 rather than L2 though. Not sure what the
implications of that are compressionwise. What happens if you replace
FFABS() with a square for scoring?
/Tomas
___
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 1/4] avcodec/jpeg2000dec: Check image offset
On Thu, 2023-10-05 at 00:59 +0200, Michael Niedermayer wrote:
> Fixes: left shift of negative value -538967841
> Fixes: 62447/clusterfuzz-testcase-minimized-
> ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-6427134337613824
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/jpeg2000dec.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index eda959e558d..691cfbd8915 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -238,6 +238,11 @@ static int get_siz(Jpeg2000DecoderContext *s)
> return AVERROR_INVALIDDATA;
> }
>
> + if (s->image_offset_x >= s->width || s->image_offset_y >= s-
> >height) {
> + av_log(s->avctx, AV_LOG_ERROR, "image offsets outside
> image");
> + return AVERROR_INVALIDDATA;
> + }
Probably OK
/Tomas
___
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 v2] avformat: introduce AVStreamGroup
>
> +enum AVStreamGroupParamsType {
> + AV_STREAM_GROUP_PARAMS_NONE,
Maybe AV_STREAM_GROUP_PARAMS_NUM on the end?
/Tomas
___
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 1/3] lavc/jpeg2000dsp: make coefficients extern
lör 2023-10-28 klockan 22:04 +0300 skrev Rémi Denis-Courmont: > This is so that they can be loaded from assembler, rather than > duplicated. Is loading these constants via immediates slower than the indirect load that the patchset does? /Tomas ___ 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] avcodec/mlp*: improvements
ons 2023-10-25 klockan 21:59 +0200 skrev Paul B Mahol:
> On Wed, Oct 25, 2023 at 9:03 PM Tomas Härdin wrote:
>
> > On Wed, 2023-10-25 at 21:00 +0200, Paul B Mahol wrote:
> > > On Wed, Oct 25, 2023 at 8:39 PM Tomas Härdin
> > > wrote:
> > >
> > > >
> > > > > if (c) {
> > > > > e[0] = 1 << 14;
> > > > > e[1] = 0 << 14;
> > > > > e[2] = v[1];
> > > > > e[3] = v[0];
> > > > > } else {
> > > > > e[0] = v[0];
> > > > > e[1] = v[1];
> > > > > e[2] = 0 << 14;
> > > > > e[3] = 1 << 14;
> > > > > }
> > > > >
> > > > > if (invert2x2(e, d)) {
> > > > > sum = UINT64_MAX;
> > > > > goto next;
> > > > > }
> > > > >
> > > >
> > > > You can make use of the properties of e to simplify calculating
> > > > the
> > > > inverse. The determinant is always v[0]<<14, so you can just do
> > > > if
> > > > (!v[0]) continue; and skip the determinant check altogether.
> > > >
> > >
> > > Even for real 2x2 matrix case? (Once one of rows is not 1, 0) ?
> > > May added such cases later.
> >
> > You can just work the math out on paper. Inverse of
> >
> > 1 0
> > v[1] v[0]
> >
> > is
> >
> > 1 0
> > -v[1]/v[0] 1/v[0]
> >
> > not accounting for shifts.
> >
>
> But I want to add real 2x2 matrix with no 0 cell, with:
>
> a, b
> c, d
>
> later. (even though gains are small, as encoded files use it rarely)
If this is possible within MLP then yes, do that. It is not clear from
what you've told me so far and from my brief reading of the code how
capable the format is.
> > Also RE: my other comments, you are right. I didn't take into
> > account
> > that MLP is lossless and that there may be off-by-one errors.
> >
> > And as I said on IRC you can formulate this as a least squares
> > problem,
> > then solve it using a linear system solve. This patch seems finds a
> > solution that minimizes L1 rather than L2 though. Not sure what the
> > implications of that are compressionwise. What happens if you
> > replace
> > FFABS() with a square for scoring?
> >
>
> It reduces size usually by less then 0.002 %
>
> Linear system solver gives vectors to create equations for both
> channels at
> same time?
L2 minimization allows using ordinary least squarse. As I said on IRC,
the rub lies in formulating the problem properly. Minimizing L1 is much
harder, since it involves solving a linear program. Of course for
practical purposes we don't need an exact solution.
Looking a bit more at the code, what is important is the decoding
coefficients, the d matrix. The encoder is free to choose d and the
encoded residuals so long as it decodes correctly. The decoder is
specified on d, not e.
Currently only one matrix is used (count=1 in estimate_coeff). With two
matrices something akin to a lifting scheme can be performed. This
means almost any 2x2 transform should be possible to perform (modulo
bitexactness concerns).
What I mean by lifting scheme here is that any 2x2 matrix A can be
decomposed into the product of two or more matrices on the form that e
has. I think.
We could potentially do something like alternating transforms on this
form:
l += k1*r;
r += k2*l;
l += k3*r;
r += k4*l;
This can always be inverted provided the intermediate results don't go
out of range, or in the event that they do go out of range, the decoder
is sufficiently well specified so that encoder and decoder don't go out
of sync. Compare how YCoCg-R is specified and fits in 3*8 bits. In fact
the WP article on YCoCg perhaps gets the point across better:
https://en.wikipedia.org/wiki/YCoCg
it in turn links this stackoverflow post which makes the same point:
https://stackoverflow.com/questions/1058/lossless-rgb-to-ycbcr-transformation/12146329#12146329
I believe any transformed found by PCA can be converted into an
equivalent lifting scheme, and it will always be lossless provided
modulo is specified correctly in the codec. I have no idea if it is.
/Tomas
___
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 v2] avformat: introduce AVStreamGroup
ons 2023-10-25 klockan 16:40 -0300 skrev James Almer:
> On 10/25/2023 4:25 PM, Tomas Härdin wrote:
> > >
> > > +enum AVStreamGroupParamsType {
> > > + AV_STREAM_GROUP_PARAMS_NONE,
> >
> > Maybe AV_STREAM_GROUP_PARAMS_NUM on the end?
>
> For what purpose? Usually, when we add those values they are not part
> of
> the API and exist for some internal iteration.
Perhaps more importantly, it would be a symbol that would have to
become versioned. So, objection retracted :)
/Tomas
___
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] MAINTAINERS: add myself as a mediacodec and videotoolbox maintainer
tis 2023-11-07 klockan 11:19 +0800 skrev Zhao Zhili: > From: Zhao Zhili > > Signed-off-by: Zhao Zhili > --- > MAINTAINERS | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index b66c3d09a6..3430e1722b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -268,11 +268,11 @@ Codecs: > Hardware acceleration: > dxva2* Hendrik Leppkes, Laurent > Aimar, Steve Lhomme > d3d11va* Steve Lhomme > - mediacodec* Matthieu Bouron, Aman Gupta > + mediacodec* Matthieu Bouron, Aman Gupta, > Zhao Zhili > vaapi* Haihao Xiang > vaapi_encode* Mark Thompson, Haihao Xiang > vdpau* Philip Langdale, Carl Eugen > Hoyos > - videotoolbox* Rick Kern, Aman Gupta > + videotoolbox* Rick Kern, Aman Gupta, Zhao > Zhili Sounds good to me /Tomas ___ 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] [ANNOUNCE] Repeat vote: GA voters list updates
lör 2023-11-11 klockan 10:42 +0100 skrev Jean-Baptiste Kempf: > > You have been told, now, several times, that a list of email is a > collection of Private Information, according to a GDPR and that you > are a process of this PI, and therefore liable to the law. Everyone with a copy of the git repository already has this information, and it has been willingly given by all contributors. Is further processing of already extant PI really not permissible according to the GDPR? Do we have to seek permission from all contributors to publish our git repositories containing PI that they have already given implicit permission to publish? That said, this part strikes me as far more relevant wrt the GDPR: > I patched the voting system to log all authorized voters in the > future to prevent this situation in the future. /Tomas ___ 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 v2 0/6] WebRTC sub-second live streaming support
This patchset is missing tests. I know that we for some reason don't really have tests for protocols, but I feel the issue is worthwhile to bring up. I've worked a bit with WebRTC recently and it's fiddly, so it'd be nice to have some automated thing that keeps track of which WebRTC implementations this works with. Or maybe this is better handled with an external project, testing which implementations interoperate? /Tomas ___ 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 v2 0/6] WebRTC sub-second live streaming support
tis 2023-11-14 klockan 13:59 +0100 skrev Michael Riedl: > Another option is an external project for WebRTC testing, but the > challenge is > keeping it maintained and compatible with changes in implementations. Perhaps there are funds to raise for such an effort? I can imagine Google et al have an interest in this. > External services might update their software, causing issues with > tests. We > would need a plan for dealing with that. Just making sure various free software implementations interoperate would be more than enough work, and provides some confidence. Surveying what features are supported by each implementation would also be of interest I imagine. For example a client of mine uses aiortc which is lacking FEC and rate control. A local company that sometimes contributes to FFmpeg is also involved in various streaming stuff (though not WebRTC so far, I think). > For paid services like Millicast, we need to figure out who pays for > the tests. > Understanding the costs is essential if we want to use paid services > for > testing. > > I'd like to hear your thoughts on these points and how you would > propose to > proceed with adding tests for protocols like WebRTC. The way I've been testing this stuff so far is by hand via Firefox and Chromium. Both can be automated, though frontend isn't really my area. As it stands at the moment, we let users be guinea pigs for our protocols. While not great, this at least means we get bug reports. But there's no way to ensure old bugs don't resurface. /Tomas ___ 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 v2 2/6] libavformat/sdp: remove whitespaces in fmtp
tis 2023-11-07 klockan 15:12 +0100 skrev Michael Riedl: > Whitespaces after semicolon breaks some servers Which servers? If the spec allows whitespace then the onus is on them to fix their implementations. /Tomas ___ 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] [ANNOUNCE] Repeat vote: GA voters list updates
tis 2023-11-14 klockan 13:49 +0100 skrev Thilo Borgmann via ffmpeg- devel: > Am 14.11.23 um 10:28 schrieb Tomas Härdin: > > lör 2023-11-11 klockan 10:42 +0100 skrev Jean-Baptiste Kempf: > > > > > > You have been told, now, several times, that a list of email is a > > > collection of Private Information, according to a GDPR and that > > > you > > > are a process of this PI, and therefore liable to the law. > > > > Everyone with a copy of the git repository already has this > > information, and it has been willingly given by all contributors. > > Is > > further processing of already extant PI really not permissible > > according to the GDPR? Do we have to seek permission from all > > contributors to publish our git repositories containing PI that > > they > > have already given implicit permission to publish? > > +1 that it is no problem to reshare public information. > A problem would arise if we connect this information with other > information - new unknown information or an unknown link between the > two. Makes sense to me. > > That said, this part strikes me as far more relevant wrt the GDPR: > > > > > I patched the voting system to log all authorized voters in the > > > future to prevent this situation in the future. > > The logfile before: > > Sun Nov 5 11:04:32 2023 127.0.0.1 Sending mail to a voter for poll > E_af2d343f39602862 > Sun Nov 5 11:04:32 2023 127.0.0.1 Sending mail to a voter for poll > E_af2d343f39602862 > > The logfile afterwards: > > Sun Nov 5 11:04:32 2023 127.0.0.1 Sending mail to a voter for poll > E_af2d343f39602862 someth...@somewhere.com > Sun Nov 5 11:04:32 2023 127.0.0.1 Sending mail to a voter for poll > E_af2d343f39602862 somethinge...@somewhereelse.com > > The other outputs are unchanged. > This does not break any privacy about the ballots - it is still > unknown what ballots voted if at all neither what that ballot > eventually voted for. Ballot secrecy cannot actually be guaranteed. While I'm not versed in the specifics of CIVS, I doubt it has solved problems like evil sysadmin. Ballots should be public IMO, secret voting is cowardice. That way duplicate ballots are easily filtered out ex-post and scrutable to all. /Tomas ___ 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 v2 2/6] libavformat/sdp: remove whitespaces in fmtp
tis 2023-11-14 klockan 18:51 +0200 skrev Rémi Denis-Courmont: > Le tiistaina 14. marraskuuta 2023, 17.47.07 EET Tomas Härdin a écrit > : > > tis 2023-11-07 klockan 15:12 +0100 skrev Michael Riedl: > > > Whitespaces after semicolon breaks some servers > > > > Which servers? If the spec allows whitespace then the onus is on > > them > > to fix their implementations. > > I couldn't find any note to the effect that white spaces are allowed > to separate > format parameters in RFC4566, but maybe I didn't look hard enough. > > I think that most implementations just happen to ignore white-spaces, > either > *because* some broken implementations like FFmpeg do send them, or > just by > implementation accident. It wouldn't be the first time FFmpeg did something wrong Anyway I'm not formally against this patch, especially since it's not something I maintain :) /Tomas ___ 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 v2 2/6] libavformat/sdp: remove whitespaces in fmtp
tis 2023-11-14 klockan 17:14 +0100 skrev Kieran Kunhya: > On Tue, 14 Nov 2023 at 16:47, Tomas Härdin wrote: > > > tis 2023-11-07 klockan 15:12 +0100 skrev Michael Riedl: > > > Whitespaces after semicolon breaks some servers > > > > Which servers? If the spec allows whitespace then the onus is on > > them > > to fix their implementations. > > > > /Tomas > > > > Poor Tomas, you are not versed in SDP witchcraft where a single > character > breaks dozens of devices but fixes dozens of others. I have in fact had some contact with SDP, much to my chagrin. This is also why we should be very strict with it, and be very clear what the spec says, and/or have the spec changed to reflect reality. With MXF, being strict has already paid dividends. /Tomas ___ 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] [ANNOUNCE] Repeat vote: GA voters list updates
tis 2023-11-14 klockan 18:46 +0200 skrev Rémi Denis-Courmont: > Le tiistaina 14. marraskuuta 2023, 17.56.24 EET Tomas Härdin a écrit > : > > Ballots should be public IMO, secret voting is cowardice. > > The French (XIXth century) Empire used notoriously public ballots, > and the > results were skewed to say the least. There is a good reason why > ballots are > supposed to be confidential. > > And I don't think FFmpeg is immune to the same sort of issues. > Consider the > case of developers with any kind of corporate affiliation or > financial > dependency. What's to prevent their boss from telling them how to > vote if the > ballots are public? Secret ballots do not prevent this. The only method we know that works is voting in person with pen and paper. It's honestly surprising how much trust is given to e-voting by people of which I assume a majority have CS degrees. /Tomas ___ 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] [POC][PATCHSET] Add qrencodesrc source
tor 2023-11-30 klockan 01:49 +0100 skrev Stefano Sabatini: > This is meant to introduce functionality to handle QR codes. Why? /Tomas ___ 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] [POC][PATCHSET] Add qrencodesrc source
tor 2023-11-30 klockan 15:39 + skrev Cosmin Stejerean via ffmpeg- devel: > > > On Nov 30, 2023, at 03:07, Tomas Härdin wrote: > > > > tor 2023-11-30 klockan 01:49 +0100 skrev Stefano Sabatini: > > > This is meant to introduce functionality to handle QR codes. > > > > Why? > > > > The why seems to be answered below the section you quoted in the > original email > > > QR codes are robust to lossy coding, therefore it should be > > possible to use them to compare a generated video with a reference > > one (in case > they cannot be compared frame-by-frame). Is the intent to generate per-frame QR codes? I guess that has some merit. Otherwise it seems pointless if it's just for generating a single QR code which could just as well be done with an external program and then overlaying the resulting image. /Tomas ___ 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] News: Removal of libndi
tis 2019-03-26 klockan 09:45 +0100 skrev Tobias Rapp: > On 25.03.2019 18:02, Jean-Baptiste Kempf wrote: > > On Mon, 25 Mar 2019, at 08:32, Tobias Rapp wrote: > > > > Most of those hardware libraries are glorified ioctls around > > > > the driver and shipped with the drivers. > > > > And I see this with nVidia, Intel MFX and Decklink (lots of > > > > "acquire C++ interface, set param" there, release the C++ > > > > interface). > > > > > > > > Matrox seems to do something else, though, introducing a > > > > special library for FFmpeg consumption, and I doubt that feels > > > > like a driver... > > > > > > The GPL is mentioned a lot in this thread. Maybe it would make > > > sense to > > > distinguish the two cases where FFmpeg is compiled with --enable- > > > gpl and > > > without it -- as the LGPL applies in that case. > > > > That does not change a thing, sorry. > > The section 6 of the LGPLv2.1 is similar to the section 3 of the > > GPL, and mentions exactly the same limitations and exceptions for > > major components of the OS. > > > > The fact that you can combine the library with a 3rd party library > > inside your program does not allow you to ship non-LGPL-compatible > > code inside the library. (The library must be changeable + > > redistributable by the user). > > > > I know that means that you can do more or less the same feature, > > but that means the architecture must be different. > > I thought that section 7 would allow to combine a 3rd party library with > a LGPL library to create a new library but now when reading it again I > stumble over the word "side-by-side" which indicates that the two parts > should not interact with each other. You can include LGPL code in a proprietary library if you provide the object files for the proprietary parts, such that you can modify the LGPL part and still link together a functioning library. I don't think I've ever seen that done however /Tomas ___ 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]lavc/bmp: Avoid a heap buffer overwrite for 1bpp
tis 2019-03-26 klockan 13:38 +0100 skrev Carl Eugen Hoyos: > Hi! > > Attached patch intends to fix a buffer overwrite reported today. Funny, I was looking at this code a few weeks ago as a good candidate for some static analysis/formal verification > ptr[avctx->width - (avctx->width & 7) + j] = buf[avctx->width >> 3] >> (7 - > j) & 1; An extra pair of parenthesis around the right-hand side would be prudent: (buf[avctx->width >> 3] >> (7 - j)) & 1 /Tomas ___ 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 1/2] libavcodec/zmbv: change 24-bit decoder channel order, from RGB24 to BGR24
tis 2019-03-26 klockan 22:13 + skrev Matthew Fearnley: > This brings the channel order in line with that used in 32-bit mode (BGR0). > > 24-bit decoding is disabled by default (#ifdef ZMBV_ENABLE_24BPP), and no > prior encoders or sample videos are known to exist for this bit depth, so > I consider this change in implementation is unlikely to affect anyone. > > The decision has been made in agreement with the DOSBox Development Team > > (dosbox.c...@gmail.com), specifically with harekiet, who wrote the original > codec. I can confirm this > Additional minor fix: use PTRDIFF_SPECIFIER for `src - c->decomp_buf`. > Other bit depths saw this change in ced0d6c14d, but this instance was > missed, presumably because of the #ifdef block. I think it'd be best to split this off into its own patch, even if it's trivial /Tomas ___ 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/2] libavcodec/zmbvenc: add support for 24-bit encoding, using pix_fmt BGR24.
tis 2019-03-26 klockan 22:13 + skrev Matthew Fearnley: > Support is #ifdef'd out at this stage, using ZMBV_ENABLE_24BPP (like in > the zmbv.c decoder) > --- > libavcodec/zmbvenc.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c > index 98029de5f6..48871758e0 100644 > --- a/libavcodec/zmbvenc.c > +++ b/libavcodec/zmbvenc.c > @@ -340,6 +340,12 @@ static av_cold int encode_init(AVCodecContext *avctx) > c->fmt = ZMBV_FMT_16BPP; > c->bypp = 2; > break; > +#ifdef ZMBV_ENABLE_24BPP > +case AV_PIX_FMT_BGR24: > +c->fmt = ZMBV_FMT_24BPP; > +c->bypp = 3; > +break; > +#endif //ZMBV_ENABLE_24BPP Should be fine, since our encoder is byte-oriented rather than word- oriented like the DosBox encoder /Tomas ___ 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".
