[FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[Andreas Rheinhardt]

Useful to let the compiler and static analyzers know that
something is unreachable without adding an av_assert
(which would be either dead for the compiler or add runtime
overhead) for this.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I can add more macros if it is desired to differentiate between
ASSERT_LEVEL == 1 and ASSERT_LEVEL > 1.

 doc/APIchanges       |  3 +++
 libavutil/avassert.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/doc/APIchanges b/doc/APIchanges
index 60f056b863..5a3ae37999 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
 
 API changes, most recent first:
 
+2024-05-24 - xxxxxxxxxx - lavu 59.xx.100 - avassert.h
+  Add av_unreachable and av_assume() macros.
+
 2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
   Add av_channel_layout_ambisonic_order().
 
diff --git a/libavutil/avassert.h b/libavutil/avassert.h
index 1895fb7551..41e29c7687 100644
--- a/libavutil/avassert.h
+++ b/libavutil/avassert.h
@@ -31,6 +31,7 @@
 #ifdef HAVE_AV_CONFIG_H
 #   include "config.h"
 #endif
+#include "attributes.h"
 #include "log.h"
 #include "macros.h"
 
@@ -68,6 +69,38 @@
 #define av_assert2_fpu() ((void)0)
 #endif
 
+/**
+ * Asserts that are used as compiler optimization hints depending
+ * upon ASSERT_LEVEL and NBDEBUG.
+ *
+ * Undefined behaviour occurs if execution reaches a point marked
+ * with av_unreachable or if a condition used with av_assume()
+ * is false.
+ *
+ * The condition used with av_assume() should not have side-effects
+ * and should be visible to the compiler.
+ */
+#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 0 || !defined(HAVE_AV_CONFIG_H) && !defined(NDEBUG)
+#define av_unreachable  av_assert0(0)
+#define av_assume(cond) av_assert0(cond)
+#elif AV_GCC_VERSION_AT_LEAST(4, 5) || AV_HAS_BUILTIN(__builtin_unreachable)
+#define av_unreachable __builtin_unreachable()
+#if AV_HAS_BUILTIN(__builtin_assume)
+#define av_assume(cond) __builtin_assume(cond)
+#else
+#define av_assume(cond) do {      \
+    if (!(cond))                  \
+        __builtin_unreachable();  \
+} while (0)
+#endif
+#elif  defined(_MSC_VER)
+#define av_unreachable  __assume(0)
+#define av_assume(cond) __assume(cond)
+#else
+#define av_unreachable
+#define av_assume(cond)
+#endif
+
 /**
  * Assert that floating point operations can be executed.
  *
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 02/12] avcodec/amrwbdec: Mark default switch as unreachable

[Andreas Rheinhardt]

Alternative fix for Coverity issue #1473499
instead of a3bb269db92601e2dc0e99352468d02f7b26c7c2.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/amrwbdec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/amrwbdec.c b/libavcodec/amrwbdec.c
index 21a730b835..de1e661f2f 100644
--- a/libavcodec/amrwbdec.c
+++ b/libavcodec/amrwbdec.c
@@ -556,7 +556,9 @@ static void decode_fixed_vector(float *fixed_vector, const uint16_t *pulse_hi,
                            ((int) pulse_hi[i] << 11), 4, 1);
         break;
     default:
-        av_assert2(0);
+        /* Everything >= MODE_SIM is impossible: MODE_SIM is patchwelcome,
+         * > MODE_SIM is invalid. */
+        av_unreachable;
     }
 
     memset(fixed_vector, 0, sizeof(float) * AMRWB_SFR_SIZE);
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 03/12] avcodec/proresenc_anatoliy: Mark impossible case as unreachable

[Andreas Rheinhardt]

Should fix Coverity issue 1440385.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Alternative to https://ffmpeg.org/pipermail/ffmpeg-devel/2024-May/327293.html

 libavcodec/proresenc_anatoliy.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/libavcodec/proresenc_anatoliy.c b/libavcodec/proresenc_anatoliy.c
index 2fb96e9cf5..7b7618af9b 100644
--- a/libavcodec/proresenc_anatoliy.c
+++ b/libavcodec/proresenc_anatoliy.c
@@ -845,18 +845,25 @@ static av_cold int prores_encode_init(AVCodecContext *avctx)
     }
 
     if (avctx->profile == AV_PROFILE_UNKNOWN) {
-        if (avctx->pix_fmt == AV_PIX_FMT_YUV422P10) {
+        switch (avctx->pix_fmt) {
+        case AV_PIX_FMT_YUV422P10:
             avctx->profile = AV_PROFILE_PRORES_STANDARD;
             av_log(avctx, AV_LOG_INFO,
                 "encoding with ProRes standard (apcn) profile\n");
-        } else if (avctx->pix_fmt == AV_PIX_FMT_YUV444P10) {
+            break;
+        case AV_PIX_FMT_YUV444P10:
             avctx->profile = AV_PROFILE_PRORES_4444;
             av_log(avctx, AV_LOG_INFO,
                    "encoding with ProRes 4444 (ap4h) profile\n");
-        } else if (avctx->pix_fmt == AV_PIX_FMT_YUVA444P10) {
+            break;
+        case AV_PIX_FMT_YUVA444P10:
             avctx->profile = AV_PROFILE_PRORES_4444;
             av_log(avctx, AV_LOG_INFO,
                    "encoding with ProRes 4444+ (ap4h) profile\n");
+            break;
+        default:
+            /* Already checked via AVCodec.pix_fmts. */
+            av_unreachable;
         }
     } else if (avctx->profile < AV_PROFILE_PRORES_PROXY
             || avctx->profile > AV_PROFILE_PRORES_XQ) {
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 04/12] all: Use put_bytes_output() instead of put_bits_ptr - pb->buf

[Andreas Rheinhardt]

Avoids accessing internals of PutBitContext.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ljpegenc.c           | 2 +-
 libavcodec/proresenc_anatoliy.c | 2 +-
 libavcodec/wmaenc.c             | 2 +-
 libavformat/mpegenc.c           | 4 ++--
 libavformat/swfenc.c            | 6 +++---
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/libavcodec/ljpegenc.c b/libavcodec/ljpegenc.c
index 46546e2160..3443951af5 100644
--- a/libavcodec/ljpegenc.c
+++ b/libavcodec/ljpegenc.c
@@ -252,7 +252,7 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
     ff_mjpeg_encode_picture_trailer(&pb, header_bits);
 
     flush_put_bits(&pb);
-    pkt->size   = put_bits_ptr(&pb) - pb.buf;
+    pkt->size   = put_bytes_output(&pb);
     *got_packet = 1;
 
     return 0;
diff --git a/libavcodec/proresenc_anatoliy.c b/libavcodec/proresenc_anatoliy.c
index 7b7618af9b..6ddb799297 100644
--- a/libavcodec/proresenc_anatoliy.c
+++ b/libavcodec/proresenc_anatoliy.c
@@ -382,7 +382,7 @@ static int encode_slice_plane(int16_t *blocks, int mb_count, uint8_t *buf, unsig
     encode_acs(&pb, blocks, blocks_per_slice, qmat, scan);
 
     flush_put_bits(&pb);
-    return put_bits_ptr(&pb) - pb.buf;
+    return put_bytes_output(&pb);
 }
 
 static av_always_inline unsigned encode_slice_data(AVCodecContext *avctx,
diff --git a/libavcodec/wmaenc.c b/libavcodec/wmaenc.c
index 6949f08fb6..eaf0498ea2 100644
--- a/libavcodec/wmaenc.c
+++ b/libavcodec/wmaenc.c
@@ -425,7 +425,7 @@ static int encode_superframe(AVCodecContext *avctx, AVPacket *avpkt,
         put_bits(&s->pb, 8, 'N');
 
     flush_put_bits(&s->pb);
-    av_assert0(put_bits_ptr(&s->pb) - s->pb.buf == avctx->block_align);
+    av_assert0(put_bytes_output(&s->pb) == avctx->block_align);
 
     if (frame->pts != AV_NOPTS_VALUE)
         avpkt->pts = frame->pts - ff_samples_to_time_base(avctx, avctx->initial_padding);
diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
index 6b6763c30f..068bc9d71e 100644
--- a/libavformat/mpegenc.c
+++ b/libavformat/mpegenc.c
@@ -124,7 +124,7 @@ static int put_pack_header(AVFormatContext *ctx, uint8_t *buf,
         put_bits(&pb, 3, 0); /* stuffing length */
     }
     flush_put_bits(&pb);
-    return put_bits_ptr(&pb) - pb.buf;
+    return put_bytes_output(&pb);
 }
 
 static int put_system_header(AVFormatContext *ctx, uint8_t *buf,
@@ -269,7 +269,7 @@ static int put_system_header(AVFormatContext *ctx, uint8_t *buf,
     }
 
     flush_put_bits(&pb);
-    size = put_bits_ptr(&pb) - pb.buf;
+    size = put_bytes_output(&pb);
     /* patch packet size */
     AV_WB16(buf + 4, size - 6);
 
diff --git a/libavformat/swfenc.c b/libavformat/swfenc.c
index d106e16d19..fc883b8023 100644
--- a/libavformat/swfenc.c
+++ b/libavformat/swfenc.c
@@ -124,7 +124,7 @@ static void put_swf_rect(AVIOContext *pb,
     put_bits(&p, nbits, ymax & mask);
 
     flush_put_bits(&p);
-    avio_write(pb, buf, put_bits_ptr(&p) - p.buf);
+    avio_write(pb, buf, put_bytes_output(&p));
 }
 
 static void put_swf_line_edge(PutBitContext *pb, int dx, int dy)
@@ -189,7 +189,7 @@ static void put_swf_matrix(AVIOContext *pb,
     put_bits(&p, nbits, ty);
 
     flush_put_bits(&p);
-    avio_write(pb, buf, put_bits_ptr(&p) - p.buf);
+    avio_write(pb, buf, put_bytes_output(&p));
 }
 
 static int swf_write_header(AVFormatContext *s)
@@ -323,7 +323,7 @@ static int swf_write_header(AVFormatContext *s)
         put_bits(&p, 5, 0);
 
         flush_put_bits(&p);
-        avio_write(pb, buf1, put_bits_ptr(&p) - p.buf);
+        avio_write(pb, buf1, put_bytes_output(&p));
 
         put_swf_end_tag(s);
     }
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 05/12] avcodec/mpeg4videodec: Mark impossible switch case as unreachable

[Andreas Rheinhardt]

Alternative to 8fc649b931a3cbc3a2dd9b50b75a9261a2fb4b49.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpeg4videodec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index df1e22207d..724ca202f5 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -24,6 +24,7 @@
 
 #include "config_components.h"
 
+#include "libavutil/avassert.h"
 #include "libavutil/internal.h"
 #include "libavutil/opt.h"
 #include "libavutil/thread.h"
@@ -598,7 +599,10 @@ static int mpeg4_decode_sprite_trajectory(Mpeg4DecContext *ctx, GetBitContext *g
         ctx->sprite_shift[1]  = alpha + beta + rho - min_ab + 2;
         break;
     default:
-        av_assert0(0);
+        /* num_sprite_warping_points outside of 0..3 results in an error
+         * in which num_sprite_warping_points is reset to zero. */
+        av_unreachable;
+        break;
     }
     /* try to simplify the situation */
     if (sprite_delta[0][0] == a << ctx->sprite_shift[0] &&
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 06/12] avcodec/pcm-dvdenc: Mark unreachable default cases as unreachable

[Andreas Rheinhardt]

Fixes a Clang warning when asserts are disabled:
"variable 'quant' is used uninitialized whenever switch default is taken
[-Wsometimes-uninitialized]"

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/pcm-dvdenc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libavcodec/pcm-dvdenc.c b/libavcodec/pcm-dvdenc.c
index 1e7ee644f6..c3e853ed6a 100644
--- a/libavcodec/pcm-dvdenc.c
+++ b/libavcodec/pcm-dvdenc.c
@@ -45,7 +45,9 @@ static av_cold int pcm_dvd_encode_init(AVCodecContext *avctx)
         freq = 1;
         break;
     default:
-        av_assert1(0);
+        /* Already checked via AVCodec.supported_samplerates. */
+        av_unreachable;
+        break;
     }
 
     switch (avctx->sample_fmt) {
@@ -58,7 +60,9 @@ static av_cold int pcm_dvd_encode_init(AVCodecContext *avctx)
         quant = 2;
         break;
     default:
-        av_assert1(0);
+        /* Already checked via AVCodec.sample_fmts. */
+        av_unreachable;
+        break;
     }
 
     avctx->bits_per_coded_sample = 16 + quant * 4;
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 07/12] avcodec/vlc: Make code more readable with av_unreachable

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/vlc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c
index ee09d96fd6..f869c19650 100644
--- a/libavcodec/vlc.c
+++ b/libavcodec/vlc.c
@@ -49,10 +49,11 @@
         v = *(const uint16_t *)ptr;                         \
         break;                                              \
     case 4:                                                 \
-    default:                                                \
-        av_assert1(size == 4);                              \
         v = *(const uint32_t *)ptr;                         \
         break;                                              \
+    default:                                                \
+        av_unreachable;                                     \
+        break;                                              \
     }                                                       \
 }
 
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 08/12] avcodec/utvideoenc: Remove always-false pixel format check

[Andreas Rheinhardt]

Mark it as unreachable instead.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/utvideoenc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavcodec/utvideoenc.c b/libavcodec/utvideoenc.c
index 59e198458b..f347a33e6e 100644
--- a/libavcodec/utvideoenc.c
+++ b/libavcodec/utvideoenc.c
@@ -143,9 +143,9 @@ static av_cold int utvideo_encode_init(AVCodecContext *avctx)
         original_format  = UTVIDEO_444;
         break;
     default:
-        av_log(avctx, AV_LOG_ERROR, "Unknown pixel format: %d\n",
-               avctx->pix_fmt);
-        return AVERROR_INVALIDDATA;
+        /* Already checked via AVCodec.pix_fmts. */
+        av_unreachable;
+        break;
     }
 
     ff_bswapdsp_init(&c->bdsp);
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 09/12] avcodec/dolby_e_parse: Use av_unreachable instead of av_assert0(0)

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/dolby_e_parse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/dolby_e_parse.c b/libavcodec/dolby_e_parse.c
index ffedcd99a4..62d38ab3a3 100644
--- a/libavcodec/dolby_e_parse.c
+++ b/libavcodec/dolby_e_parse.c
@@ -88,7 +88,8 @@ int ff_dolby_e_convert_input(DBEContext *s, int nb_words, int key)
             AV_WB24(dst, AV_RB24(src) ^ key);
         break;
     default:
-        av_assert0(0);
+        av_unreachable;
+        break;
     }
 
     return init_get_bits(&s->gb, s->buffer, nb_words * s->word_bits);
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 10/12] avcodec/put_bits: Allow to mark places where PutBitContext is flushed

[Andreas Rheinhardt]

This will allow the compiler to optimize the "is the cache full?"
branches away from some put_bits().

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/put_bits.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
index 4561dc131a..79bad1c214 100644
--- a/libavcodec/put_bits.h
+++ b/libavcodec/put_bits.h
@@ -74,6 +74,16 @@ static inline void init_put_bits(PutBitContext *s, uint8_t *buffer,
     s->bit_buf      = 0;
 }
 
+/**
+ * Inform the compiler that a PutBitContext is flushed (i.e. if it has just
+ * been initialized or flushed). Undefined behaviour occurs if this is used
+ * with a PutBitContext for which this is not true.
+ */
+static inline void put_bits_assume_flushed(const PutBitContext *s)
+{
+    av_assume(s->bit_left == BUF_BITS);
+}
+
 /**
  * @return the total number of bits written to the bitstream.
  */
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 11/12] avcodec/e?ac3enc: Inform compiler about PutBitContext being blank

[Andreas Rheinhardt]

This turned out to be very beneficial: For GCC 13, the codesize
of ac3_output_frame_header went down from 4522B to 1247B and
from 10762B to 9298B for eac3_output_frame_header. For Clang 17,
the numbers went down from 3923B to 2477B and from 8338B to 6548B
(always with -O3).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ac3enc.c  | 2 ++
 libavcodec/eac3enc.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c
index 3649289865..a1783577c5 100644
--- a/libavcodec/ac3enc.c
+++ b/libavcodec/ac3enc.c
@@ -1638,6 +1638,8 @@ static void ac3_output_frame_header(AC3EncodeContext *s, PutBitContext *pb)
 {
     AC3EncOptions *opt = &s->options;
 
+    put_bits_assume_flushed(pb);
+
     put_bits(pb, 16, 0x0b77);   /* frame header */
     put_bits(pb, 16, 0);        /* crc1: will be filled later */
     put_bits(pb, 2,  s->bit_alloc.sr_code);
diff --git a/libavcodec/eac3enc.c b/libavcodec/eac3enc.c
index 8ef3e7e773..1c522dae2e 100644
--- a/libavcodec/eac3enc.c
+++ b/libavcodec/eac3enc.c
@@ -135,6 +135,8 @@ static void eac3_output_frame_header(AC3EncodeContext *s, PutBitContext *pb)
     int blk, ch;
     AC3EncOptions *opt = &s->options;
 
+    put_bits_assume_flushed(pb);
+
     put_bits(pb, 16, 0x0b77);                   /* sync word */
 
     /* BSI header */
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 12/12] avcodec/speedhqenc: Use av_unreachable for unreachable condition

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/speedhqenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/speedhqenc.c b/libavcodec/speedhqenc.c
index 5b4ff4c139..29d603c729 100644
--- a/libavcodec/speedhqenc.c
+++ b/libavcodec/speedhqenc.c
@@ -128,7 +128,8 @@ av_cold int ff_speedhq_encode_init(MpegEncContext *s)
         s->avctx->codec_tag = MKTAG('S','H','Q','4');
         break;
     default:
-        av_assert0(0);
+        /* Already checked via AVCodec.pix_fmts. */
+        av_unreachable;
     }
 
     return 0;
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 13/16] avcodec/wmaenc: Move transient PutBitContext from Context to stack

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/wma.h    |  2 --
 libavcodec/wmaenc.c | 58 ++++++++++++++++++++++-----------------------
 2 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/libavcodec/wma.h b/libavcodec/wma.h
index 3d0d872ea3..a909f5baa7 100644
--- a/libavcodec/wma.h
+++ b/libavcodec/wma.h
@@ -28,7 +28,6 @@
 
 #include "avcodec.h"
 #include "get_bits.h"
-#include "put_bits.h"
 
 /* size of blocks */
 #define BLOCK_MIN_BITS 7
@@ -68,7 +67,6 @@ typedef struct CoefVLCTable {
 typedef struct WMACodecContext {
     AVCodecContext *avctx;
     GetBitContext gb;
-    PutBitContext pb;
     int version;                            ///< 1 = 0x160 (WMAV1), 2 = 0x161 (WMAV2)
     int use_bit_reservoir;
     int use_variable_block_len;
diff --git a/libavcodec/wmaenc.c b/libavcodec/wmaenc.c
index eaf0498ea2..7240c0895c 100644
--- a/libavcodec/wmaenc.c
+++ b/libavcodec/wmaenc.c
@@ -28,6 +28,7 @@
 #include "avcodec.h"
 #include "codec_internal.h"
 #include "encode.h"
+#include "put_bits.h"
 #include "wma.h"
 #include "libavutil/avassert.h"
 
@@ -161,7 +162,7 @@ static void init_exp(WMACodecContext *s, int ch, const int *exp_param)
     s->max_exponent[ch] = max_scale;
 }
 
-static void encode_exp_vlc(WMACodecContext *s, int ch, const int *exp_param)
+static void encode_exp_vlc(WMACodecContext *s, PutBitContext *pb, int ch, const int *exp_param)
 {
     int last_exp;
     const uint16_t *ptr;
@@ -173,7 +174,7 @@ static void encode_exp_vlc(WMACodecContext *s, int ch, const int *exp_param)
     if (s->version == 1) {
         last_exp = *exp_param++;
         av_assert0(last_exp - 10 >= 0 && last_exp - 10 < 32);
-        put_bits(&s->pb, 5, last_exp - 10);
+        put_bits(pb, 5, last_exp - 10);
         q += *ptr++;
     } else
         last_exp = 36;
@@ -181,7 +182,7 @@ static void encode_exp_vlc(WMACodecContext *s, int ch, const int *exp_param)
         int exp  = *exp_param++;
         int code = exp - last_exp + 60;
         av_assert1(code >= 0 && code < 120);
-        put_bits(&s->pb, ff_aac_scalefactor_bits[code],
+        put_bits(pb, ff_aac_scalefactor_bits[code],
                  ff_aac_scalefactor_code[code]);
         /* XXX: use a table */
         q       += *ptr++;
@@ -189,7 +190,8 @@ static void encode_exp_vlc(WMACodecContext *s, int ch, const int *exp_param)
     }
 }
 
-static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
+static int encode_block(WMACodecContext *s, PutBitContext *pb,
+                        float (*src_coefs)[BLOCK_MAX_SIZE],
                         int total_gain)
 {
     int channels = s->avctx->ch_layout.nb_channels;
@@ -230,7 +232,7 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
     }
 
     if (channels == 2)
-        put_bits(&s->pb, 1, !!s->ms_stereo);
+        put_bits(pb, 1, !!s->ms_stereo);
 
     for (ch = 0; ch < channels; ch++) {
         // FIXME only set channel_coded when needed, instead of always
@@ -269,7 +271,7 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
     v = 0;
     for (ch = 0; ch < channels; ch++) {
         int a = s->channel_coded[ch];
-        put_bits(&s->pb, 1, a);
+        put_bits(pb, 1, a);
         v |= a;
     }
 
@@ -277,8 +279,8 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
         return 1;
 
     for (v = total_gain - 1; v >= 127; v -= 127)
-        put_bits(&s->pb, 7, 127);
-    put_bits(&s->pb, 7, v);
+        put_bits(pb, 7, 127);
+    put_bits(pb, 7, v);
 
     coef_nb_bits = ff_wma_total_gain_to_bits(total_gain);
 
@@ -288,7 +290,7 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
                 int i, n;
                 n = s->exponent_high_sizes[bsize];
                 for (i = 0; i < n; i++) {
-                    put_bits(&s->pb, 1, s->high_band_coded[ch][i] = 0);
+                    put_bits(pb, 1, s->high_band_coded[ch][i] = 0);
                     if (0)
                         nb_coefs[ch] -= s->exponent_high_bands[bsize][i];
                 }
@@ -298,13 +300,13 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
 
     parse_exponents = 1;
     if (s->block_len_bits != s->frame_len_bits)
-        put_bits(&s->pb, 1, parse_exponents);
+        put_bits(pb, 1, parse_exponents);
 
     if (parse_exponents) {
         for (ch = 0; ch < channels; ch++) {
             if (s->channel_coded[ch]) {
                 if (s->use_exp_vlc) {
-                    encode_exp_vlc(s, ch, fixed_exp);
+                    encode_exp_vlc(s, pb, ch, fixed_exp);
                 } else {
                     av_assert0(0); // FIXME not implemented
 //                    encode_exp_lsp(s, ch);
@@ -333,28 +335,28 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
                             code = run + s->int_table[tindex][abs_level - 1];
 
                     av_assert2(code < s->coef_vlcs[tindex]->n);
-                    put_bits(&s->pb, s->coef_vlcs[tindex]->huffbits[code],
+                    put_bits(pb, s->coef_vlcs[tindex]->huffbits[code],
                              s->coef_vlcs[tindex]->huffcodes[code]);
 
                     if (code == 0) {
                         if (1 << coef_nb_bits <= abs_level)
                             return -1;
 
-                        put_bits(&s->pb, coef_nb_bits, abs_level);
-                        put_bits(&s->pb, s->frame_len_bits, run);
+                        put_bits(pb, coef_nb_bits, abs_level);
+                        put_bits(pb, s->frame_len_bits, run);
                     }
                     // FIXME the sign is flipped somewhere
-                    put_bits(&s->pb, 1, level < 0);
+                    put_bits(pb, 1, level < 0);
                     run = 0;
                 } else
                     run++;
             }
             if (run)
-                put_bits(&s->pb, s->coef_vlcs[tindex]->huffbits[1],
+                put_bits(pb, s->coef_vlcs[tindex]->huffbits[1],
                          s->coef_vlcs[tindex]->huffcodes[1]);
         }
         if (s->version == 1 && channels >= 2)
-            align_put_bits(&s->pb);
+            align_put_bits(pb);
     }
     return 0;
 }
@@ -362,23 +364,24 @@ static int encode_block(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
 static int encode_frame(WMACodecContext *s, float (*src_coefs)[BLOCK_MAX_SIZE],
                         uint8_t *buf, int buf_size, int total_gain)
 {
-    init_put_bits(&s->pb, buf, buf_size);
+    PutBitContext pb;
+    init_put_bits(&pb, buf, buf_size);
 
     if (s->use_bit_reservoir)
         av_assert0(0); // FIXME not implemented
-    else if (encode_block(s, src_coefs, total_gain) < 0)
+    else if (encode_block(s, &pb, src_coefs, total_gain) < 0)
         return INT_MAX;
 
-    align_put_bits(&s->pb);
+    flush_put_bits(&pb);
 
-    return put_bits_count(&s->pb) / 8 - s->avctx->block_align;
+    return put_bytes_output(&pb) - s->avctx->block_align;
 }
 
 static int encode_superframe(AVCodecContext *avctx, AVPacket *avpkt,
                              const AVFrame *frame, int *got_packet_ptr)
 {
     WMACodecContext *s = avctx->priv_data;
-    int i, total_gain, ret, error;
+    int i, total_gain, ret, error, remaining;
 
     s->block_len_bits = s->frame_len_bits; // required by non variable block len
     s->block_len      = 1 << s->block_len_bits;
@@ -418,14 +421,9 @@ static int encode_superframe(AVCodecContext *avctx, AVPacket *avpkt,
         avpkt->size = 0;
         return AVERROR(EINVAL);
     }
-    av_assert0((put_bits_count(&s->pb) & 7) == 0);
-    i = avctx->block_align - put_bytes_count(&s->pb, 0);
-    av_assert0(i>=0);
-    while(i--)
-        put_bits(&s->pb, 8, 'N');
-
-    flush_put_bits(&s->pb);
-    av_assert0(put_bytes_output(&s->pb) == avctx->block_align);
+    remaining = -error;
+    // Add padding
+    memset(avpkt->data + avctx->block_align - remaining, 'N', remaining);
 
     if (frame->pts != AV_NOPTS_VALUE)
         avpkt->pts = frame->pts - ff_samples_to_time_base(avctx, avctx->initial_padding);
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 14/16] avcodec/wma: Mark ff_wma_end() as av_cold

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/wma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/wma.c b/libavcodec/wma.c
index da9c914b57..257184c0d8 100644
--- a/libavcodec/wma.c
+++ b/libavcodec/wma.c
@@ -364,7 +364,7 @@ int ff_wma_total_gain_to_bits(int total_gain)
         return  9;
 }
 
-int ff_wma_end(AVCodecContext *avctx)
+av_cold int ff_wma_end(AVCodecContext *avctx)
 {
     WMACodecContext *s = avctx->priv_data;
     int i;
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 15/16] avcodec/wmaenc: Use av_unreachable to mark unreachable codepath

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/wmaenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/wmaenc.c b/libavcodec/wmaenc.c
index 7240c0895c..b4650d42b3 100644
--- a/libavcodec/wmaenc.c
+++ b/libavcodec/wmaenc.c
@@ -80,7 +80,7 @@ static av_cold int encode_init(AVCodecContext *avctx)
         AV_WL32(extradata, flags1);
         AV_WL16(extradata + 4, flags2);
     } else {
-        av_assert0(0);
+        av_unreachable;
     }
     avctx->extradata          = extradata;
     s->use_exp_vlc            = flags2 & 0x0001;
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 16/16] avcodec/wmaenc: Don't unnecessarily reset AVPacket.size

[Andreas Rheinhardt]

The packet is unreferenced generically lateron anyway.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/wmaenc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavcodec/wmaenc.c b/libavcodec/wmaenc.c
index b4650d42b3..73ce0876c9 100644
--- a/libavcodec/wmaenc.c
+++ b/libavcodec/wmaenc.c
@@ -418,7 +418,6 @@ static int encode_superframe(AVCodecContext *avctx, AVPacket *avpkt,
         error = encode_frame(s, s->coefs, avpkt->data, avpkt->size, total_gain++);
     if (error > 0) {
         av_log(avctx, AV_LOG_ERROR, "Invalid input data or requested bitrate too low, cannot encode\n");
-        avpkt->size = 0;
         return AVERROR(EINVAL);
     }
     remaining = -error;
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2948 bytes --]

Hi

On Fri, May 24, 2024 at 11:58:21PM +0200, Andreas Rheinhardt wrote:
> Useful to let the compiler and static analyzers know that
> something is unreachable without adding an av_assert
> (which would be either dead for the compiler or add runtime
> overhead) for this.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> I can add more macros if it is desired to differentiate between
> ASSERT_LEVEL == 1 and ASSERT_LEVEL > 1.
> 
>  doc/APIchanges       |  3 +++
>  libavutil/avassert.h | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 60f056b863..5a3ae37999 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>  
>  API changes, most recent first:
>  
> +2024-05-24 - xxxxxxxxxx - lavu 59.xx.100 - avassert.h
> +  Add av_unreachable and av_assume() macros.
> +
>  2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
>    Add av_channel_layout_ambisonic_order().
>  
> diff --git a/libavutil/avassert.h b/libavutil/avassert.h
> index 1895fb7551..41e29c7687 100644
> --- a/libavutil/avassert.h
> +++ b/libavutil/avassert.h
> @@ -31,6 +31,7 @@
>  #ifdef HAVE_AV_CONFIG_H
>  #   include "config.h"
>  #endif
> +#include "attributes.h"
>  #include "log.h"
>  #include "macros.h"
>  
> @@ -68,6 +69,38 @@
>  #define av_assert2_fpu() ((void)0)
>  #endif
>  
> +/**
> + * Asserts that are used as compiler optimization hints depending
> + * upon ASSERT_LEVEL and NBDEBUG.
> + *
> + * Undefined behaviour occurs if execution reaches a point marked
> + * with av_unreachable or if a condition used with av_assume()
> + * is false.
> + *
> + * The condition used with av_assume() should not have side-effects
> + * and should be visible to the compiler.
> + */

this feels wrong

We have 3 assert functions

one for security relevant code or other things we always want to check and not play around

one for speed relevant code where we dont want to check in production code. But may want
to do checks if we are debuging.

and one for the cases between


What is an assert ? Its a statement about a condition that is true unless the code
is broken. Its never correct to use an assert to check for a condition that is known
to be false for some input.
So a assert really already is either

A. Check, print, abort
or
B. undefined if false

But if an assert already is "undefined if false" then what you add is not
usefull, just add the compiler specific "assume" code to the disabled asserts

This would also keep the API simpler

thx

[..]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[Andreas Rheinhardt]

Michael Niedermayer:
> Hi
> 
> On Fri, May 24, 2024 at 11:58:21PM +0200, Andreas Rheinhardt wrote:
>> Useful to let the compiler and static analyzers know that
>> something is unreachable without adding an av_assert
>> (which would be either dead for the compiler or add runtime
>> overhead) for this.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> I can add more macros if it is desired to differentiate between
>> ASSERT_LEVEL == 1 and ASSERT_LEVEL > 1.
>>
>>  doc/APIchanges       |  3 +++
>>  libavutil/avassert.h | 33 +++++++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 60f056b863..5a3ae37999 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>>  
>>  API changes, most recent first:
>>  
>> +2024-05-24 - xxxxxxxxxx - lavu 59.xx.100 - avassert.h
>> +  Add av_unreachable and av_assume() macros.
>> +
>>  2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
>>    Add av_channel_layout_ambisonic_order().
>>  
>> diff --git a/libavutil/avassert.h b/libavutil/avassert.h
>> index 1895fb7551..41e29c7687 100644
>> --- a/libavutil/avassert.h
>> +++ b/libavutil/avassert.h
>> @@ -31,6 +31,7 @@
>>  #ifdef HAVE_AV_CONFIG_H
>>  #   include "config.h"
>>  #endif
>> +#include "attributes.h"
>>  #include "log.h"
>>  #include "macros.h"
>>  
>> @@ -68,6 +69,38 @@
>>  #define av_assert2_fpu() ((void)0)
>>  #endif
>>  
>> +/**
>> + * Asserts that are used as compiler optimization hints depending
>> + * upon ASSERT_LEVEL and NBDEBUG.
>> + *
>> + * Undefined behaviour occurs if execution reaches a point marked
>> + * with av_unreachable or if a condition used with av_assume()
>> + * is false.
>> + *
>> + * The condition used with av_assume() should not have side-effects
>> + * and should be visible to the compiler.
>> + */
> 
> this feels wrong
> 
> We have 3 assert functions
> 
> one for security relevant code or other things we always want to check and not play around
> 
> one for speed relevant code where we dont want to check in production code. But may want
> to do checks if we are debuging.
> 
> and one for the cases between
> 
> 
> What is an assert ? Its a statement about a condition that is true unless the code
> is broken. Its never correct to use an assert to check for a condition that is known
> to be false for some input.
> So a assert really already is either
> 
> A. Check, print, abort
> or
> B. undefined if false
> 
> But if an assert already is "undefined if false" then what you add is not
> usefull, just add the compiler specific "assume" code to the disabled asserts

1. So you want me to change the disabled asserts into a "if (!(cond))
__builtin_unreachable();" (like dav1d does)? This is problematic,
because asserts (as they are used right now) contain no requirement at
all that the condition be visible to the compiler; it may contain
function calls that the compiler cannot elide (unless it is an LTO
compiler, but even they stop at library boundaries). While we could of
course look through our own asserts and change them, we must not simply
do so for our users.
(The PutBits API has checks for the buffer being too small:
            av_log(NULL, AV_LOG_ERROR, "Internal error, put_bits buffer
too small\n");
            av_assert2(0);
If the av_assert2 is changed into an __builtin_unreachable() in case the
latter assert is disabled, then this defeats the purpose of this check.
This shows that all our asserts need to be checked and be potentially
changed to be consistent with using them as optimization hints.)

2. It is useful, it is just a different usecase: Here the focus is not
on correctness, but on telling the compiler something that is presumed
to be beneficial for performance.

> This would also keep the API simpler

IMO using av_unreachable instead of av_assertX(0) expresses the intent
better (so for me the current usage feels wrong). Same for av_assume
(see 2. for the intent).

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 5986 bytes --]

Hi

On Sun, May 26, 2024 at 02:59:35AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Hi
> > 
> > On Fri, May 24, 2024 at 11:58:21PM +0200, Andreas Rheinhardt wrote:
> >> Useful to let the compiler and static analyzers know that
> >> something is unreachable without adding an av_assert
> >> (which would be either dead for the compiler or add runtime
> >> overhead) for this.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> ---
> >> I can add more macros if it is desired to differentiate between
> >> ASSERT_LEVEL == 1 and ASSERT_LEVEL > 1.
> >>
> >>  doc/APIchanges       |  3 +++
> >>  libavutil/avassert.h | 33 +++++++++++++++++++++++++++++++++
> >>  2 files changed, 36 insertions(+)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index 60f056b863..5a3ae37999 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
> >>  
> >>  API changes, most recent first:
> >>  
> >> +2024-05-24 - xxxxxxxxxx - lavu 59.xx.100 - avassert.h
> >> +  Add av_unreachable and av_assume() macros.
> >> +
> >>  2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
> >>    Add av_channel_layout_ambisonic_order().
> >>  
> >> diff --git a/libavutil/avassert.h b/libavutil/avassert.h
> >> index 1895fb7551..41e29c7687 100644
> >> --- a/libavutil/avassert.h
> >> +++ b/libavutil/avassert.h
> >> @@ -31,6 +31,7 @@
> >>  #ifdef HAVE_AV_CONFIG_H
> >>  #   include "config.h"
> >>  #endif
> >> +#include "attributes.h"
> >>  #include "log.h"
> >>  #include "macros.h"
> >>  
> >> @@ -68,6 +69,38 @@
> >>  #define av_assert2_fpu() ((void)0)
> >>  #endif
> >>  
> >> +/**
> >> + * Asserts that are used as compiler optimization hints depending
> >> + * upon ASSERT_LEVEL and NBDEBUG.
> >> + *
> >> + * Undefined behaviour occurs if execution reaches a point marked
> >> + * with av_unreachable or if a condition used with av_assume()
> >> + * is false.
> >> + *
> >> + * The condition used with av_assume() should not have side-effects
> >> + * and should be visible to the compiler.
> >> + */
> > 
> > this feels wrong
> > 
> > We have 3 assert functions
> > 
> > one for security relevant code or other things we always want to check and not play around
> > 
> > one for speed relevant code where we dont want to check in production code. But may want
> > to do checks if we are debuging.
> > 
> > and one for the cases between
> > 
> > 
> > What is an assert ? Its a statement about a condition that is true unless the code
> > is broken. Its never correct to use an assert to check for a condition that is known
> > to be false for some input.
> > So a assert really already is either
> > 
> > A. Check, print, abort
> > or
> > B. undefined if false
> > 
> > But if an assert already is "undefined if false" then what you add is not
> > usefull, just add the compiler specific "assume" code to the disabled asserts
> 
> 1. So you want me to change the disabled asserts into a "if (!(cond))
> __builtin_unreachable();" (like dav1d does)? This is problematic,
> because asserts (as they are used right now) contain no requirement at
> all that the condition be visible to the compiler;


> it may contain
> function calls that the compiler cannot elide (unless it is an LTO
> compiler, but even they stop at library boundaries).

would that break anything ?


> While we could of
> course look through our own asserts and change them, we must not simply
> do so for our users.

the feature can be delayed outside our code until the next API bump


> (The PutBits API has checks for the buffer being too small:
>             av_log(NULL, AV_LOG_ERROR, "Internal error, put_bits buffer
> too small\n");
>             av_assert2(0);
> If the av_assert2 is changed into an __builtin_unreachable() in case the
> latter assert is disabled, then this defeats the purpose of this check.

indeed but finding this now, gives the oppertunity to improve this code


> This shows that all our asserts need to be checked and be potentially
> changed to be consistent with using them as optimization hints.)

maybe but introducing a redundant "assert" isnt avoiding that.
You still either check everything or you dont check
and you either use unchecked cases for optimization or only checked cases.
Its just more assert like macros


> 
> 2. It is useful, it is just a different usecase: Here the focus is not
> on correctness, but on telling the compiler something that is presumed
> to be beneficial for performance.
> 

> > This would also keep the API simpler
> 
> IMO using av_unreachable instead of av_assertX(0) expresses the intent
> better (so for me the current usage feels wrong). Same for av_assume
> (see 2. for the intent).

I dont agree

if we do add av_unreachable() we will for debuging need a way to
disabled it and print something, same as av_assert*() already does
so it really is a av_assert*() just with a different name

about the name, well

we already use

av_assert0(!"reached");

and various variations of that.
The advantage is that the API is simpler, there are fewer things one needs to remember
to use it.

Its "most pretty for core developers who work on this every day" vs.
    "easy to use for developers rarely interacting with FFmpeg"

The more our API grows, the harder it becomes for outsiders to learn and use
in this respect i dont think increasing the 3 av_assert*() with 2+ more macros is a
good idea more so because they need a way to be turned off which one too needs
to remember, ...

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".