Re: [FFmpeg-devel] [PATCH 3/3] avcodec/mpeg4videodec: Replace always true check by assert

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Mon, May 16, 2022 at 01:05:28PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Maybe helps coverity
> > Helps: CID1433771
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/mpeg4videodec.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> > index e2bde73639..715cb606c9 100644
> > --- a/libavcodec/mpeg4videodec.c
> > +++ b/libavcodec/mpeg4videodec.c
> > @@ -1981,7 +1981,8 @@ static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n
> >                  return AVERROR_INVALIDDATA;
> >              j = scantable[idx++];
> >              block[j] = get_xbits(&s->gb, additional_code_len);
> > -        } else if (group == 21) {
> > +        } else {
> > +            av_assert2(group == 21);
> >              /* Escape */
> >              if (idx > 63)
> >                  return AVERROR_INVALIDDATA;
> 
> This also reminds me of an old attempt of mine to add an AV_UNREACHABLE
> macro for such scenarios:
> https://github.com/mkver/FFmpeg/commits/unreachable. There are two
> reasons why I never sent it to the ML:

> a) It uses ASSERT_LEVEL (i.e. with a high ASSERT_LEVEL it degenarates
> into an actual assert). But this is only defined internally, so useless
> to an API user. Therefore I wonder whether this should be in a public
> header (the same issue exists for av_assert1 and av_assert2).

for av_assertX to be usefull to a user app, the developer of that app
needs to be able to set ASSERT_LEVEL. It would be less useful if it
was fixed to the value ffmpeg used in its built

The developer should be able to set ASSERT_LEVEL before the include
or with -D
but some other way could be added too


> b) Both Clang and MSVC have something more, namely a
> __builtin_assume(cond)  resp. __assume(cond). I was unsure whether this
> should not be added, too. It could be translated to "if (!(cond))
> __builtin_unreachable()" to GCC, but would be more natural in general.
> (Of course, cond must not have any side effects.)


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

Nations do behave wisely once they have exhausted all other alternatives. 
-- Abba Eban

[-- 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".