[PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros
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
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
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
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
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); }
Will apply tomorrow unless there are objections. - Andreas
Hi Andreas On Sun, Mar 09, 2025 at 07:18:34PM +0100, Andreas Rheinhardt wrote:
Andreas Rheinhardt:
Avoids accessing internals of PutBitContext. [...] @@ -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); }
Will apply tomorrow unless there are objections.
i like the new code more thanks for improving this and other things [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange
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
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
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
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
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
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
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
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
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
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
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;
Will apply tomorrow unless there are objections. - Andreas
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
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
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;
Will apply this patch tomorrow unless there are objections. - Andreas
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
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
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.
participants (2)
-
Andreas Rheinhardt -
Michael Niedermayer