| Message ID | 20191101094011.4021-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | e4fdeb3fcefeb98f2225f7ccded156fb175959c5 |
| Headers | show |
On Fri, Nov 1, 2019 at 10:42 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > This compared to the other suggestions is cleaner and easer to understand > keeping the condition in the if() simple > > See: [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak > See: [FFmpeg-devel] [PATCH]lavf/nutenc: Do not call memcmp() with NULL > argument > > Fixes: Ticket 7980 > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/nutenc.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > index d212f0c245..46dce7722d 100644 > --- a/libavformat/nutenc.c > +++ b/libavformat/nutenc.c > @@ -792,11 +792,12 @@ static int get_needed_flags(NUTContext *nut, > StreamContext *nus, FrameCode *fc, > flags |= FLAG_CHECKSUM; > if (FFABS(pkt->pts - nus->last_pts) > nus->max_pts_distance) > flags |= FLAG_CHECKSUM; > - if (pkt->size < nut->header_len[fc->header_idx] || > - (pkt->size > 4096 && fc->header_idx) || > - memcmp(pkt->data, nut->header[fc->header_idx], > - nut->header_len[fc->header_idx])) > - flags |= FLAG_HEADER_IDX; > + if (fc->header_idx) > + if (pkt->size < nut->header_len[fc->header_idx] || > + pkt->size > 4096 || > + memcmp(pkt->data, nut->header [fc->header_idx], > + nut->header_len[fc->header_idx])) > + flags |= FLAG_HEADER_IDX; > > return flags | (fc->flags & FLAG_CODED); > } > -- > 2.23.0 > > The reference to [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak is inappropriate. Carl's patch was titled "[FFmpeg-devel] lavf/nutenc: Do not call memcmp() with NULL argument". You could also mention that this affected lots of fate-tests. And "easer" is missing an "i". I have no objections apart from that. I didn't test your code, but it looks fine. - Andreas
On Fri, Nov 01, 2019 at 11:22:41AM +0100, Andreas Rheinhardt wrote: > On Fri, Nov 1, 2019 at 10:42 AM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > This compared to the other suggestions is cleaner and easer to understand > > keeping the condition in the if() simple > > > > See: [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak > > See: [FFmpeg-devel] [PATCH]lavf/nutenc: Do not call memcmp() with NULL > > argument > > > > Fixes: Ticket 7980 > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/nutenc.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > > index d212f0c245..46dce7722d 100644 > > --- a/libavformat/nutenc.c > > +++ b/libavformat/nutenc.c > > @@ -792,11 +792,12 @@ static int get_needed_flags(NUTContext *nut, > > StreamContext *nus, FrameCode *fc, > > flags |= FLAG_CHECKSUM; > > if (FFABS(pkt->pts - nus->last_pts) > nus->max_pts_distance) > > flags |= FLAG_CHECKSUM; > > - if (pkt->size < nut->header_len[fc->header_idx] || > > - (pkt->size > 4096 && fc->header_idx) || > > - memcmp(pkt->data, nut->header[fc->header_idx], > > - nut->header_len[fc->header_idx])) > > - flags |= FLAG_HEADER_IDX; > > + if (fc->header_idx) > > + if (pkt->size < nut->header_len[fc->header_idx] || > > + pkt->size > 4096 || > > + memcmp(pkt->data, nut->header [fc->header_idx], > > + nut->header_len[fc->header_idx])) > > + flags |= FLAG_HEADER_IDX; > > > > return flags | (fc->flags & FLAG_CODED); > > } > > -- > > 2.23.0 > > > > > The reference to [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak is > inappropriate. yes, oops that was a copy and paste error fixed locally > Carl's patch was titled "[FFmpeg-devel] lavf/nutenc: Do not call memcmp() > with NULL argument". Its copy and pasted from here: 179255 0701 1:12 Carl Eugen Hoyo (2,2K) [FFmpeg-devel] [PATCH]lavf/nutenc: Do not call memcmp() with NULL argument iam not sure what you refer to, but theres a [PATCH] in the subject with a missing space and the reference should duplicate that if theres a reference > You could also mention that this affected lots of fate-tests. will do > And "easer" > is missing an "i". will correct > I have no objections apart from that. I didn't test your code, but it looks > fine. ok, ill wait a bit more in case others want to comment or test Thanks! [...]
On Fri, Nov 1, 2019 at 1:32 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Nov 01, 2019 at 11:22:41AM +0100, Andreas Rheinhardt wrote: > > On Fri, Nov 1, 2019 at 10:42 AM Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > > > This compared to the other suggestions is cleaner and easer to > understand > > > keeping the condition in the if() simple > > > > > > See: [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak > > > See: [FFmpeg-devel] [PATCH]lavf/nutenc: Do not call memcmp() with NULL > > > argument > > > > > > Fixes: Ticket 7980 > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/nutenc.c | 11 ++++++----- > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > > > index d212f0c245..46dce7722d 100644 > > > --- a/libavformat/nutenc.c > > > +++ b/libavformat/nutenc.c > > > @@ -792,11 +792,12 @@ static int get_needed_flags(NUTContext *nut, > > > StreamContext *nus, FrameCode *fc, > > > flags |= FLAG_CHECKSUM; > > > if (FFABS(pkt->pts - nus->last_pts) > nus->max_pts_distance) > > > flags |= FLAG_CHECKSUM; > > > - if (pkt->size < nut->header_len[fc->header_idx] || > > > - (pkt->size > 4096 && fc->header_idx) || > > > - memcmp(pkt->data, nut->header[fc->header_idx], > > > - nut->header_len[fc->header_idx])) > > > - flags |= FLAG_HEADER_IDX; > > > + if (fc->header_idx) > > > + if (pkt->size < nut->header_len[fc->header_idx] || > > > + pkt->size > 4096 || > > > + memcmp(pkt->data, nut->header [fc->header_idx], > > > + nut->header_len[fc->header_idx])) > > > + flags |= FLAG_HEADER_IDX; > > > > > > return flags | (fc->flags & FLAG_CODED); > > > } > > > -- > > > 2.23.0 > > > > > > > > The reference to [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak is > > inappropriate. > > yes, oops that was a copy and paste error > fixed locally > > > > Carl's patch was titled "[FFmpeg-devel] lavf/nutenc: Do not call memcmp() > > with NULL argument". > > Its copy and pasted from here: > 179255 0701 1:12 Carl Eugen Hoyo (2,2K) [FFmpeg-devel] > [PATCH]lavf/nutenc: Do not call memcmp() with NULL argument > > iam not sure what you refer to, but theres a [PATCH] in the subject with a > missing space and the reference should duplicate that if theres a reference > > Sorry, my mistake: I thought that you mentioned two of my patches (namely the memleak and my nutenc patch) when in reality you actually referenced my memleak patch and Carl's patch. - Andreas
diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c index d212f0c245..46dce7722d 100644 --- a/libavformat/nutenc.c +++ b/libavformat/nutenc.c @@ -792,11 +792,12 @@ static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc, flags |= FLAG_CHECKSUM; if (FFABS(pkt->pts - nus->last_pts) > nus->max_pts_distance) flags |= FLAG_CHECKSUM; - if (pkt->size < nut->header_len[fc->header_idx] || - (pkt->size > 4096 && fc->header_idx) || - memcmp(pkt->data, nut->header[fc->header_idx], - nut->header_len[fc->header_idx])) - flags |= FLAG_HEADER_IDX; + if (fc->header_idx) + if (pkt->size < nut->header_len[fc->header_idx] || + pkt->size > 4096 || + memcmp(pkt->data, nut->header [fc->header_idx], + nut->header_len[fc->header_idx])) + flags |= FLAG_HEADER_IDX; return flags | (fc->flags & FLAG_CODED); }
This compared to the other suggestions is cleaner and easer to understand keeping the condition in the if() simple See: [FFmpeg-devel] [PATCH] avformat/nutenc: Fix memleak See: [FFmpeg-devel] [PATCH]lavf/nutenc: Do not call memcmp() with NULL argument Fixes: Ticket 7980 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/nutenc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)