Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Wed, May 15, 2019 at 12:13:07PM -0700, Adam Richter wrote: > On Tue, May 14, 2019 at 6:48 PM myp...@gmail.com wrote: > > > > On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes wrote: > > > > > > On Tue, May 14, 2019 at 11:25 PM Adam Richter > > > wrote: [...] > > > > > > > > Also after this, I may take a look at adding a branch hint to the > > > > av_assertN > > > > macros if nobody objects. > > > > > > > > > > Please don't, we don't do branch hints anywhere, and this is a bad > > > place to start. > > > If an assert is truely performance sensitive (as in, it makes a > > > measurable difference), it should probably be moved from assert0 to > > > assert1, and as such is only enabled in testing builds. > > > > > If assert0 or assert1 lead to performance drop, we need some profiling > > data, then try to fix it. > > The above comments by Hendrick and you does not identify a cost to > adding a branch hint to assert. There would be a downside in the form of > a few lines of code complexity in libavutil/avassert.h. If that is > your concern, > I guess our disagreement is that I see reducing the cost of assert so that > perhaps CPU usage with and without would be a tiny bit more similar for > reproducing problems and maybe (I'm not saying it's likely) it might tip a > trade-off in favor of keeping an assert enabled in some borderline > circumstance. I'd take that trade (add the branch prediction). > > If you want to educate me on some other reason why I may be wrong, > about adding the branch prediction for assertion checks, I'd been keen > to know, but I realize everyone's time is limited, and if I haven't > convinced you and also created consensus in favor of adding the > branch prediction to assertion checking, then I don't expect to advocate > further on this list for merging such a change into your tree at this time. I think a key question here would be if a speedeffect or code generation improvment can be shown to have actually occured. Instead of assuming that it could. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Into a blind darkness they enter who follow after the Ignorance, they as if into a greater darkness enter who devote themselves to the Knowledge alone. -- Isha Upanishad signature.asc Description: PGP signature ___ 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] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Wed, May 15, 2019 at 9:21 PM Adam Richter wrote: > > On Tue, May 14, 2019 at 6:48 PM myp...@gmail.com wrote: > > > > On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes wrote: > > > > > > On Tue, May 14, 2019 at 11:25 PM Adam Richter > > > wrote: > > > > > > > > Consider, for example, if you agree that columnization makes this range > > > > check > > > > more recognizable in a glance and makes it easier to spot what the > > > > bounds are > > > > (the sixteen space indentation is taken from the code in which it > > > > appeared): > > > > > > > > av_assert0(par->bits_per_coded_sample >= 0 && > > > > par->bits_per_coded_sample <= 8); > > > > > > > > ...vs... > > > > > > > > av_assert0(par->bits_per_coded_sample >= 0); > > > > av_assert0(par->bits_per_coded_sample <= 8); > > > > > > > > A possible counter-argument to this might be that, in a long sequence > > > > of assertions, can be informative to group related assertions > > > > together, which I think is true, but it is possible to get this other > > > > means, such as by using blank lines to separate express such grouping. > > > > > > > > So, Mark, if you decide you are OK with my complete patches, I encourage > > > > you to let me know. Otherwise, if there are any of those changes which > > > > you > > > > are OK with, I would like to just to to get those merged. Finally, if > > > > you would > > > > rather see none of those changes merged (one one to split the > > > > assertions in > > > > libavformat and one was for everywhere else in ffmpeg), please let me > > > > know > > > > about that too, in which case, if no one advocates for their > > > > inclusion, I'll drop > > > > my proposal to merge these changes. > > > > > > > > > > Unfortunately I have to agree with Mark. asserst that check the same > > > value or extremely closely related values should stay combined. > > > > > I agree this part > > I am trying to understand what problem you see with this. > > It occurs to me that you might be concerned about generating less > efficient code for the common assert success case, in particular, > in the example I showed for readability, potentially dereferencing > "par" twice, but I made a test program (attached) and determined > from reading the generated assembly that at least for gcc with > optimization -O2 on x86_64, x86_32, aarch64 and arm32, as > long as the abort function has "__attribute__ ((noreturn))", the > compiler seems to be able to avoid repeating the memory fetch > for the second assertion. I also check this for clang -O2 on > on x86_64. > > Of course, without the noreturn attribute on the abort function, > the compiler realizes that the abort function could have written > to memory, so it refetches the value in the split assertion case, > which I think might have been your concern, but as long as > the abort function is declared with an attribute noreturn, we > should be OK for gcc. > > On the other hand, I'm not sure what compilers people use > for other platforms such as Microsoft Windows and if you tell > me that it is a known problem on a specific platform that is > potentially relevant to ffmpeg, that would probably change my > mind. > > Of course, it's not necessary for you change my mind or for > you to invest further time in this discussion, as I imagine you > and other participants have other things to do. So, if I don't > get a further explanation, I may still believe that you're wrong, > but I'll respect your need to prioritize tasks other than continuing > this discussion, and will not expect to see my proposed change > merged unless the predominant opinion of others in this discussion > changes to being in favor it, which, so far, I acknowledge, it is not. You seem to be totally overthinking this. I'm not concerned about code generation or anything like that, just the simple fact that I believe that the checks are more logical and actually easier to understand if they are logically grouped and combined. And I really don't see the advantage in any of these changes, personally. > > > > > > > > > Also after this, I may take a look at adding a branch hint to the > > > > av_assertN > > > > macros if nobody objects. > > > > > > > > > > Please don't, we don't do branch hints anywhere, and this is a bad > > > place to start. > > > If an assert is truely performance sensitive (as in, it makes a > > > measurable difference), it should probably be moved from assert0 to > > > assert1, and as such is only enabled in testing builds. > > > > > If assert0 or assert1 lead to performance drop, we need some profiling > > data, then try to fix it. > > The above comments by Hendrick and you does not identify a cost to > adding a branch hint to assert. There would be a downside in the form of > a few lines of code complexity in libavutil/avassert.h. If that is > your concern, > I guess our disagreement is that I see reducing the cost of assert so that >
Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Tue, May 14, 2019 at 6:48 PM myp...@gmail.com wrote: > > On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes wrote: > > > > On Tue, May 14, 2019 at 11:25 PM Adam Richter > > wrote: > > > > > > Consider, for example, if you agree that columnization makes this range > > > check > > > more recognizable in a glance and makes it easier to spot what the bounds > > > are > > > (the sixteen space indentation is taken from the code in which it > > > appeared): > > > > > > av_assert0(par->bits_per_coded_sample >= 0 && > > > par->bits_per_coded_sample <= 8); > > > > > > ...vs... > > > > > > av_assert0(par->bits_per_coded_sample >= 0); > > > av_assert0(par->bits_per_coded_sample <= 8); > > > > > > A possible counter-argument to this might be that, in a long sequence > > > of assertions, can be informative to group related assertions > > > together, which I think is true, but it is possible to get this other > > > means, such as by using blank lines to separate express such grouping. > > > > > > So, Mark, if you decide you are OK with my complete patches, I encourage > > > you to let me know. Otherwise, if there are any of those changes which > > > you > > > are OK with, I would like to just to to get those merged. Finally, if > > > you would > > > rather see none of those changes merged (one one to split the assertions > > > in > > > libavformat and one was for everywhere else in ffmpeg), please let me know > > > about that too, in which case, if no one advocates for their > > > inclusion, I'll drop > > > my proposal to merge these changes. > > > > > > > Unfortunately I have to agree with Mark. asserst that check the same > > value or extremely closely related values should stay combined. > > > I agree this part I am trying to understand what problem you see with this. It occurs to me that you might be concerned about generating less efficient code for the common assert success case, in particular, in the example I showed for readability, potentially dereferencing "par" twice, but I made a test program (attached) and determined from reading the generated assembly that at least for gcc with optimization -O2 on x86_64, x86_32, aarch64 and arm32, as long as the abort function has "__attribute__ ((noreturn))", the compiler seems to be able to avoid repeating the memory fetch for the second assertion. I also check this for clang -O2 on on x86_64. Of course, without the noreturn attribute on the abort function, the compiler realizes that the abort function could have written to memory, so it refetches the value in the split assertion case, which I think might have been your concern, but as long as the abort function is declared with an attribute noreturn, we should be OK for gcc. On the other hand, I'm not sure what compilers people use for other platforms such as Microsoft Windows and if you tell me that it is a known problem on a specific platform that is potentially relevant to ffmpeg, that would probably change my mind. Of course, it's not necessary for you change my mind or for you to invest further time in this discussion, as I imagine you and other participants have other things to do. So, if I don't get a further explanation, I may still believe that you're wrong, but I'll respect your need to prioritize tasks other than continuing this discussion, and will not expect to see my proposed change merged unless the predominant opinion of others in this discussion changes to being in favor it, which, so far, I acknowledge, it is not. > > > > > > Also after this, I may take a look at adding a branch hint to the > > > av_assertN > > > macros if nobody objects. > > > > > > > Please don't, we don't do branch hints anywhere, and this is a bad > > place to start. > > If an assert is truely performance sensitive (as in, it makes a > > measurable difference), it should probably be moved from assert0 to > > assert1, and as such is only enabled in testing builds. > > > If assert0 or assert1 lead to performance drop, we need some profiling > data, then try to fix it. The above comments by Hendrick and you does not identify a cost to adding a branch hint to assert. There would be a downside in the form of a few lines of code complexity in libavutil/avassert.h. If that is your concern, I guess our disagreement is that I see reducing the cost of assert so that perhaps CPU usage with and without would be a tiny bit more similar for reproducing problems and maybe (I'm not saying it's likely) it might tip a trade-off in favor of keeping an assert enabled in some borderline circumstance. I'd take that trade (add the branch prediction). If you want to educate me on some other reason why I may be wrong, about adding the branch prediction for assertion checks, I'd been keen to know, but I realize everyone's time is limited, and if I haven't convinced you and also created consensus in favor of adding the branch prediction to assertion
Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes wrote: > > On Tue, May 14, 2019 at 11:25 PM Adam Richter wrote: > > > > Consider, for example, if you agree that columnization makes this range > > check > > more recognizable in a glance and makes it easier to spot what the bounds > > are > > (the sixteen space indentation is taken from the code in which it appeared): > > > > av_assert0(par->bits_per_coded_sample >= 0 && > > par->bits_per_coded_sample <= 8); > > > > ...vs... > > > > av_assert0(par->bits_per_coded_sample >= 0); > > av_assert0(par->bits_per_coded_sample <= 8); > > > > A possible counter-argument to this might be that, in a long sequence > > of assertions, can be informative to group related assertions > > together, which I think is true, but it is possible to get this other > > means, such as by using blank lines to separate express such grouping. > > > > So, Mark, if you decide you are OK with my complete patches, I encourage > > you to let me know. Otherwise, if there are any of those changes which you > > are OK with, I would like to just to to get those merged. Finally, if you > > would > > rather see none of those changes merged (one one to split the assertions in > > libavformat and one was for everywhere else in ffmpeg), please let me know > > about that too, in which case, if no one advocates for their > > inclusion, I'll drop > > my proposal to merge these changes. > > > > Unfortunately I have to agree with Mark. asserst that check the same > value or extremely closely related values should stay combined. > I agree this part > > > > Also after this, I may take a look at adding a branch hint to the av_assertN > > macros if nobody objects. > > > > Please don't, we don't do branch hints anywhere, and this is a bad > place to start. > If an assert is truely performance sensitive (as in, it makes a > measurable difference), it should probably be moved from assert0 to > assert1, and as such is only enabled in testing builds. > If assert0 or assert1 lead to performance drop, we need some profiling data, then try to fix it. > - Hendrik ___ 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] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Tue, May 14, 2019 at 11:25 PM Adam Richter wrote: > > Consider, for example, if you agree that columnization makes this range check > more recognizable in a glance and makes it easier to spot what the bounds are > (the sixteen space indentation is taken from the code in which it appeared): > > av_assert0(par->bits_per_coded_sample >= 0 && > par->bits_per_coded_sample <= 8); > > ...vs... > > av_assert0(par->bits_per_coded_sample >= 0); > av_assert0(par->bits_per_coded_sample <= 8); > > A possible counter-argument to this might be that, in a long sequence > of assertions, can be informative to group related assertions > together, which I think is true, but it is possible to get this other > means, such as by using blank lines to separate express such grouping. > > So, Mark, if you decide you are OK with my complete patches, I encourage > you to let me know. Otherwise, if there are any of those changes which you > are OK with, I would like to just to to get those merged. Finally, if you > would > rather see none of those changes merged (one one to split the assertions in > libavformat and one was for everywhere else in ffmpeg), please let me know > about that too, in which case, if no one advocates for their > inclusion, I'll drop > my proposal to merge these changes. > Unfortunately I have to agree with Mark. asserst that check the same value or extremely closely related values should stay combined. > > Also after this, I may take a look at adding a branch hint to the av_assertN > macros if nobody objects. > Please don't, we don't do branch hints anywhere, and this is a bad place to start. If an assert is truely performance sensitive (as in, it makes a measurable difference), it should probably be moved from assert0 to assert1, and as such is only enabled in testing builds. - Hendrik ___ 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] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Sun, May 12, 2019 at 11:16 AM Michael Niedermayer
wrote:
>
> On Sun, May 12, 2019 at 05:58:50PM +0100, Mark Thompson wrote:
> > On 12/05/2019 16:24, Adam Richter wrote:
> > > This patch separates statements of the form "assert(a && b);" into
> > > "assert(a);" and "assert(b);", typically involving an assertion
> > > function like av_assert0.
> > >
> > > This patch covers all of ffmpeg, except for the libavformat, which I
> > > have already submitted separately.
> > >
> > > I have not tested this patch other than observing that ffmpeg still
> > > builds without any apparent new complaints, that no complaints in the
> > > build contain "assert", and that "make fate" seems to succeed.
> > >
> > > Thanks in advance for considering the attached patch.
> > >
> > > Adam
> > >
> > >
> > > From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001
> > > From: Adam Richter
> > > Date: Sun, 12 May 2019 08:02:51 -0700
> > > Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more
> > > precise diagnostics, except for libformat.
> > >
> > > This patch separates statements of the form "assert(a && b);" into
> > > "assert(a);" and "assert(b);", for more precise diagnostics when
> > > an assertion fails, which can be especially important in cases where
> > > the problem may not be easily reproducible and save developer time.
> > > Usually, this involves functions like av_assert0.
> >
> > I don't feel very convinced by the general case of this argument.
> > Assertions are primarily present to assist a developer reading/writing the
> > code; they should never be triggering at runtime in non-development
> > contexts.
> >
> > Where the statements a and b are not related then yes, splitting them is a
> > good idea. But when it's something like a bounds check on one variable
> > ("av_assert0(A < n && n < B)", as appears quite a few times below) then I
> > think keeping it as one statement feels clearer. Similarly for highly
> > related conditions ("av_assert0(p && p->f)" or "av_assert0(x < width && y <
> > height)").
> >
> > > There are a couple of cases that this patch does not change:
> > > (1) assert conjunctions of the form assert(condition && "string literal
> > > to pass to the user as a helpful tip."), and
> > > (2) assert condjunctions where the first part contained a variable
> > > assignment that was used in the second part of the assertion and
> > > not outside the assert (so that the variable assignment be elided
> > > if the assertion checking omitted).
> > >
> > > This patch covers all of ffmpeg except for libavformat, which was
> > > covered in a patch that I previously submitted separately.
> > >
> > > These changes build without any new complaints that I noticed, and
> > > "make fate" succeeds, but I have not otherwise tested them.
> > >
> > > Signed-off-by: Adam Richter
> > > ...
> > > diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> > > index fca692cb15..bef52e8b02 100644
> > > --- a/libavcodec/aacpsy.c
> > > +++ b/libavcodec/aacpsy.c
> > > @@ -504,9 +504,11 @@ static int calc_bit_demand(AacPsyContext *ctx, float
> > > pe, int bits, int size,
> > > fill_level = av_clipf((float)ctx->fill_level / size, clip_low,
> > > clip_high);
> > > clipped_pe = av_clipf(pe, ctx->pe.min, ctx->pe.max);
> > > bit_save = (fill_level + bitsave_add) * bitsave_slope;
> > > -assert(bit_save <= 0.3f && bit_save >= -0.0501f);
> > > +assert(bit_save <= 0.3f);
> > > +assert(bit_save >= -0.0501f);
> > > bit_spend = (fill_level + bitspend_add) * bitspend_slope;
> > > -assert(bit_spend <= 0.5f && bit_spend >= -0.1f);
> > > +assert(bit_spend <= 0.5f);
> > > +assert(bit_spend >= -0.1f);
> >
> > While you're touching calls to traditional assert() please consider turning
> > them into suitable av_assertN().
>
> I agree that they should be changed to av_assertN() but it should be
> in a seperate commit/patch
>
> These assert -> av_assertN changes will likely in some cases "activate"
> "dormant" checks which fail. Managing / Testing / Reviewing and correcting
> these should be easier if they are not in a large change splitting asserts
>
> Thanks
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
[...]
Hello, Michael and Mark.
Thank you for your reviews of my proposed changes that would split
most assertions
of the form "a && b" into separate assertions.
Michael, if I understand correctly, you are OK with the patches after
having looked at
them to spot problems, but are not advocating for or against their
inclusion. Mark, if I
understand correctly, you would rather see assertions split only where "a and
b are not related." I don't think anyone else has chimed in, but I
would welcome being
told if I missed anyone else's expressed opinion or if anyone wants to
add anything now.
To make the most efficient use of your time, I would like to proceed
as follows. In the rest
of this
Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Sun, May 12, 2019 at 08:24:11AM -0700, Adam Richter wrote: > This patch separates statements of the form "assert(a && b);" into > "assert(a);" and "assert(b);", typically involving an assertion > function like av_assert0. > > This patch covers all of ffmpeg, except for the libavformat, which I > have already submitted separately. > > I have not tested this patch other than observing that ffmpeg still > builds without any apparent new complaints, that no complaints in the > build contain "assert", and that "make fate" seems to succeed. > > Thanks in advance for considering the attached patch. > > Adam > fftools/cmdutils.c|3 +- > fftools/ffmpeg.c |3 +- > libavcodec/4xm.c | 12 ++--- > libavcodec/aaccoder_twoloop.h |3 +- > libavcodec/aacenc.c |3 +- > libavcodec/aacenc_quantization_misc.h |3 +- > libavcodec/aacpsy.c |6 +++- > libavcodec/ac3enc.c | 15 > libavcodec/acelp_filters.c|3 +- > libavcodec/amfenc.c |5 ++-- > libavcodec/amrnbdec.c |3 +- > libavcodec/av1_frame_split_bsf.c |3 +- > libavcodec/avpacket.c |3 +- > libavcodec/cbs.c | 42 > ++ > libavcodec/cbs_av1.c |9 --- > libavcodec/cbs_av1_syntax_template.c |3 +- > libavcodec/cbs_h2645.c| 10 > libavcodec/cbs_mpeg2.c|4 +-- > libavcodec/cbs_vp9.c |8 -- > libavcodec/celp_filters.c |3 +- > libavcodec/dca_core.c |3 +- > libavcodec/decode.c |4 +-- > libavcodec/dvdsubdec.c|3 +- > libavcodec/dvenc.c|3 +- > libavcodec/dxva2_h264.c |3 +- > libavcodec/dxva2_hevc.c |3 +- > libavcodec/dxva2_vp9.c|3 +- > libavcodec/error_resilience.c |3 +- > libavcodec/ffv1dec.c |3 +- > libavcodec/flacenc.c |6 +++- > libavcodec/get_bits.h | 35 +++- > libavcodec/h263dec.c |3 +- > libavcodec/h2645_parse.c |6 +++- > libavcodec/h264_refs.c|3 +- > libavcodec/h264_slice.c |3 +- > libavcodec/h264chroma_template.c | 20 > libavcodec/hevc_filter.c |6 +++- > libavcodec/huffyuv.c |3 +- > libavcodec/huffyuvenc.c |5 +++- > libavcodec/ituh263enc.c |3 +- > libavcodec/ivi.c |4 ++- > libavcodec/jpeg2000dec.c |3 +- > libavcodec/lclenc.c |3 +- > libavcodec/lpc.c |5 ++-- > libavcodec/lzwenc.c |9 --- > libavcodec/mips/h264chroma_msa.c | 31 +++-- > libavcodec/mips/vc1dsp_mmi.c | 20 > libavcodec/mjpegdec.c | 11 +--- > libavcodec/motion_est.c | 12 - > libavcodec/motion_est_template.c | 10 ++-- > libavcodec/mpeg12.c |3 +- > libavcodec/mpeg12enc.c|6 +++- > libavcodec/mpeg4videoenc.c|6 +++- > libavcodec/mpegaudiodec_template.c|3 +- > libavcodec/mpegaudioenc_template.c|6 +++- > libavcodec/mpegutils.c|6 +++- > libavcodec/mpegvideo_enc.c|9 --- > libavcodec/mpegvideo_xvmc.c |3 +- > libavcodec/mpegvideoencdsp.c |3 +- > libavcodec/mqcenc.c |3 +- > libavcodec/put_bits.h |9 --- > libavcodec/rv34.c |3 +- > libavcodec/rv40dsp.c | 10 ++-- > libavcodec/sanm.c |3 +- > libavcodec/sinewin_tablegen.h |3 +- > libavcodec/snow.c |3 +- > libavcodec/snow.h |3 +- > libavcodec/snow_dwt.c |3 +- > libavcodec/snowenc.c | 14 +++ > libavcodec/svq1enc.c | 24 --- > libavcodec/vaapi_encode.c |8 -- > libavcodec/vaapi_encode_h264.c|3 +- > libavcodec/vaapi_encode_h265.c|6 +++- > libavcodec/vaapi_encode_vp9.c |4 +-- > libavcodec/vc1_pred.c |3 +- > libavcodec/vc1dsp.c | 20 > libavcodec/videodsp_template.c|6 +++- > libavcodec/vp9.c |3 +- > libavcodec/vp9recon.c |3 +- > libavcodec/wmaenc.c |6 +++- > libavcodec/x86/videodsp_init.c|
Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Sun, May 12, 2019 at 05:58:50PM +0100, Mark Thompson wrote:
> On 12/05/2019 16:24, Adam Richter wrote:
> > This patch separates statements of the form "assert(a && b);" into
> > "assert(a);" and "assert(b);", typically involving an assertion
> > function like av_assert0.
> >
> > This patch covers all of ffmpeg, except for the libavformat, which I
> > have already submitted separately.
> >
> > I have not tested this patch other than observing that ffmpeg still
> > builds without any apparent new complaints, that no complaints in the
> > build contain "assert", and that "make fate" seems to succeed.
> >
> > Thanks in advance for considering the attached patch.
> >
> > Adam
> >
> >
> > From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001
> > From: Adam Richter
> > Date: Sun, 12 May 2019 08:02:51 -0700
> > Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more
> > precise diagnostics, except for libformat.
> >
> > This patch separates statements of the form "assert(a && b);" into
> > "assert(a);" and "assert(b);", for more precise diagnostics when
> > an assertion fails, which can be especially important in cases where
> > the problem may not be easily reproducible and save developer time.
> > Usually, this involves functions like av_assert0.
>
> I don't feel very convinced by the general case of this argument. Assertions
> are primarily present to assist a developer reading/writing the code; they
> should never be triggering at runtime in non-development contexts.
>
> Where the statements a and b are not related then yes, splitting them is a
> good idea. But when it's something like a bounds check on one variable
> ("av_assert0(A < n && n < B)", as appears quite a few times below) then I
> think keeping it as one statement feels clearer. Similarly for highly
> related conditions ("av_assert0(p && p->f)" or "av_assert0(x < width && y <
> height)").
>
> > There are a couple of cases that this patch does not change:
> > (1) assert conjunctions of the form assert(condition && "string literal
> > to pass to the user as a helpful tip."), and
> > (2) assert condjunctions where the first part contained a variable
> > assignment that was used in the second part of the assertion and
> > not outside the assert (so that the variable assignment be elided
> > if the assertion checking omitted).
> >
> > This patch covers all of ffmpeg except for libavformat, which was
> > covered in a patch that I previously submitted separately.
> >
> > These changes build without any new complaints that I noticed, and
> > "make fate" succeeds, but I have not otherwise tested them.
> >
> > Signed-off-by: Adam Richter
> > ...
> > diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> > index fca692cb15..bef52e8b02 100644
> > --- a/libavcodec/aacpsy.c
> > +++ b/libavcodec/aacpsy.c
> > @@ -504,9 +504,11 @@ static int calc_bit_demand(AacPsyContext *ctx, float
> > pe, int bits, int size,
> > fill_level = av_clipf((float)ctx->fill_level / size, clip_low,
> > clip_high);
> > clipped_pe = av_clipf(pe, ctx->pe.min, ctx->pe.max);
> > bit_save = (fill_level + bitsave_add) * bitsave_slope;
> > -assert(bit_save <= 0.3f && bit_save >= -0.0501f);
> > +assert(bit_save <= 0.3f);
> > +assert(bit_save >= -0.0501f);
> > bit_spend = (fill_level + bitspend_add) * bitspend_slope;
> > -assert(bit_spend <= 0.5f && bit_spend >= -0.1f);
> > +assert(bit_spend <= 0.5f);
> > +assert(bit_spend >= -0.1f);
>
> While you're touching calls to traditional assert() please consider turning
> them into suitable av_assertN().
I agree that they should be changed to av_assertN() but it should be
in a seperate commit/patch
These assert -> av_assertN changes will likely in some cases "activate"
"dormant" checks which fail. Managing / Testing / Reviewing and correcting
these should be easier if they are not in a large change splitting asserts
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2
signature.asc
Description: PGP signature
___
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] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On 12/05/2019 16:24, Adam Richter wrote:
> This patch separates statements of the form "assert(a && b);" into
> "assert(a);" and "assert(b);", typically involving an assertion
> function like av_assert0.
>
> This patch covers all of ffmpeg, except for the libavformat, which I
> have already submitted separately.
>
> I have not tested this patch other than observing that ffmpeg still
> builds without any apparent new complaints, that no complaints in the
> build contain "assert", and that "make fate" seems to succeed.
>
> Thanks in advance for considering the attached patch.
>
> Adam
>
>
> From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001
> From: Adam Richter
> Date: Sun, 12 May 2019 08:02:51 -0700
> Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more
> precise diagnostics, except for libformat.
>
> This patch separates statements of the form "assert(a && b);" into
> "assert(a);" and "assert(b);", for more precise diagnostics when
> an assertion fails, which can be especially important in cases where
> the problem may not be easily reproducible and save developer time.
> Usually, this involves functions like av_assert0.
I don't feel very convinced by the general case of this argument. Assertions
are primarily present to assist a developer reading/writing the code; they
should never be triggering at runtime in non-development contexts.
Where the statements a and b are not related then yes, splitting them is a good
idea. But when it's something like a bounds check on one variable
("av_assert0(A < n && n < B)", as appears quite a few times below) then I think
keeping it as one statement feels clearer. Similarly for highly related
conditions ("av_assert0(p && p->f)" or "av_assert0(x < width && y < height)").
> There are a couple of cases that this patch does not change:
> (1) assert conjunctions of the form assert(condition && "string literal
> to pass to the user as a helpful tip."), and
> (2) assert condjunctions where the first part contained a variable
> assignment that was used in the second part of the assertion and
> not outside the assert (so that the variable assignment be elided
> if the assertion checking omitted).
>
> This patch covers all of ffmpeg except for libavformat, which was
> covered in a patch that I previously submitted separately.
>
> These changes build without any new complaints that I noticed, and
> "make fate" succeeds, but I have not otherwise tested them.
>
> Signed-off-by: Adam Richter
> ...
> diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> index fca692cb15..bef52e8b02 100644
> --- a/libavcodec/aacpsy.c
> +++ b/libavcodec/aacpsy.c
> @@ -504,9 +504,11 @@ static int calc_bit_demand(AacPsyContext *ctx, float pe,
> int bits, int size,
> fill_level = av_clipf((float)ctx->fill_level / size, clip_low,
> clip_high);
> clipped_pe = av_clipf(pe, ctx->pe.min, ctx->pe.max);
> bit_save = (fill_level + bitsave_add) * bitsave_slope;
> -assert(bit_save <= 0.3f && bit_save >= -0.0501f);
> +assert(bit_save <= 0.3f);
> +assert(bit_save >= -0.0501f);
> bit_spend = (fill_level + bitspend_add) * bitspend_slope;
> -assert(bit_spend <= 0.5f && bit_spend >= -0.1f);
> +assert(bit_spend <= 0.5f);
> +assert(bit_spend >= -0.1f);
While you're touching calls to traditional assert() please consider turning
them into suitable av_assertN().
- Mark
___
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".
