[FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix macro definition and use
Soft Works
softworkz at hotmail.com
Tue May 17 01:03:02 EEST 2022
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Monday, May 16, 2022 10:49 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix
> macro definition and use
>
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Sunday, May 15, 2022 8:12 PM
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec:
> fix
> >> macro definition and use
> >>
> >> softworkz:
> >>> From: softworkz <softworkz at hotmail.com>
> >>>
> >>> Signed-off-by: softworkz <softworkz at hotmail.com>
> >>> ---
> >>> libavformat/asfdec_f.c | 24 ++++++++++++------------
> >>> 1 file changed, 12 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> >>> index 81a29f99d5..91c3874ac7 100644
> >>> --- a/libavformat/asfdec_f.c
> >>> +++ b/libavformat/asfdec_f.c
> >>> @@ -906,21 +906,21 @@ static int asf_read_header(AVFormatContext
> *s)
> >>> }
> >>>
> >>> #define DO_2BITS(bits, var, defval) \
> >>> - switch (bits & 3) { \
> >>> + switch ((bits) & 3) { \
> >>> case 3: \
> >>> - var = avio_rl32(pb); \
> >>> + (var) = avio_rl32(pb); \
> >>> rsize += 4; \
> >>> break; \
> >>> case 2: \
> >>> - var = avio_rl16(pb); \
> >>> + (var) = avio_rl16(pb); \
> >>> rsize += 2; \
> >>> break; \
> >>> case 1: \
> >>> - var = avio_r8(pb); \
> >>> + (var) = avio_r8(pb); \
> >>> rsize++; \
> >>> break; \
> >>> default: \
> >>> - var = defval; \
> >>> + (var) = (defval); \
> >>> break; \
> >>> }
> >>>
> >>> @@ -1003,9 +1003,9 @@ static int asf_get_packet(AVFormatContext
> *s,
> >> AVIOContext *pb)
> >>> asf->packet_flags = c;
> >>> asf->packet_property = d;
> >>>
> >>> - DO_2BITS(asf->packet_flags >> 5, packet_length, s-
> >>> packet_size);
> >>> - DO_2BITS(asf->packet_flags >> 1, padsize, 0); // sequence
> >> ignored
> >>> - DO_2BITS(asf->packet_flags >> 3, padsize, 0); // padding
> length
> >>> + DO_2BITS(asf->packet_flags >> 5, packet_length, s-
> >packet_size)
> >>> + DO_2BITS(asf->packet_flags >> 1, padsize, 0) // sequence
> >> ignored
> >>> + DO_2BITS(asf->packet_flags >> 3, padsize, 0) // padding
> length
> >>>
> >>> // the following checks prevent overflows and infinite loops
> >>> if (!packet_length || packet_length >= (1U << 29)) {
> >>> @@ -1066,9 +1066,9 @@ static int
> >> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
> >>> asf->stream_index = asf->asfid2avid[num & 0x7f];
> >>> asfst = &asf->streams[num & 0x7f];
> >>> // sequence should be ignored!
> >>> - DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0);
> >>> - DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset,
> >> 0);
> >>> - DO_2BITS(asf->packet_property, asf->packet_replic_size, 0);
> >>> + DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0)
> >>> + DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset,
> 0)
> >>> + DO_2BITS(asf->packet_property, asf->packet_replic_size, 0)
> >>> av_log(asf, AV_LOG_TRACE, "key:%d stream:%d seq:%d offset:%d
> >> replic_size:%d num:%X packet_property %X\n",
> >>> asf->packet_key_frame, asf->stream_index, asf-
> >>> packet_seq,
> >>> asf->packet_frag_offset, asf->packet_replic_size,
> num,
> >> asf->packet_property);
> >>> @@ -1144,7 +1144,7 @@ static int
> >> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
> >>> return AVERROR_INVALIDDATA;
> >>> }
> >>> if (asf->packet_flags & 0x01) {
> >>> - DO_2BITS(asf->packet_segsizetype >> 6, asf-
> >>> packet_frag_size, 0); // 0 is illegal
> >>> + DO_2BITS(asf->packet_segsizetype >> 6, asf-
> >>> packet_frag_size, 0) // 0 is illegal
> >>> if (rsize > asf->packet_size_left) {
> >>> av_log(s, AV_LOG_ERROR, "packet_replic_size is
> >> invalid\n");
> >>> return AVERROR_INVALIDDATA;
> >>
> >> While protecting macro arguments is good, it is not really a "fix"
> >> unless current usage is buggy.
> >
> > Ok, I will rephrase the commit message.
> >
> >> Which it isn't here, because >> has higher precedence than &.
> >
> > Could you explain which change you are referring to?
> >
>
> Putting "bits" in parentheses. It doesn't change anything, because >>
> has higher precedence than &.
Ah, that's what you mean. I didn't even look at the usages of
the macro, because I think a macro should be safe intrinsically,
not only based on its current usages.
Actually this had also caught my attention due to a clang warning:
https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-macro-parentheses.html
> > All this patch does is to put macro variables in brackets
> > and remove semicolons..
> >
> >> Furthermore I am not really sure whether removing the ';' is even
> >> something worthwhile; they are surely unnecessary (being null
> >> statements), but does this matter?
> >
> > It causes a warning
> >
> >
> https://releases.llvm.org/13.0.0/tools/clang/docs/DiagnosticsReference
> .html#wextra-semi-stmt
> >
>
> I don't receive this warning despite using Clang 13.0. Do you have -
> Wall
> or -Wextra or something like that enabled?
I'm using ReSharper C++ which is using clang-tidy from clang 13.0
with -Weverything
What settings do you use?
> IMO a better fix for this would be to wrap the macro in a do {} while
> (0) to keep the macro calls function-like.
Isn't that a bit too... hm.. much/ugly?
> Anyway, you should have mentioned in the commit message that your aim
> is to fix this uncommon warning.
Yes, that makes sense.
> > I don't know how others are working, but I use to work in a way
> where
> > such warnings are shown in the editor and in lists in the IDE
> > even without compilation. Now - when you have a code file that
> > generates like 20, 50 or more warnings, it's much harder to spot
> > those warnings that might be really relevant and hinting at a
> mistake,
> > and you might be just too lazy to go through them each time.
> >
> > The clang diagnostics have been helpful in spotting some actual
> > issues in this very file. That's why I consider it worthwhile
> > to also eliminate such "non-issues".
> >
>
> I also work like that; e.g. my recent ac3.h header patchset was
> inspired
> by clangd not liking cycles in header inclusions ("In included file:
> main file cannot be included recursively when building a preamble").
Yea, I gathered from some of your patches that you must be using
some tooling as well. Would you allow me the question which IDE you
are using?
Thanks a lot,
softworkz
More information about the ffmpeg-devel
mailing list