| Message ID | 20200409123844.25208-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/jpeg2000dec: Fix mixed declaration and code | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt <andreas.rheinhardt@gmail.com>: > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/jpeg2000dec.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > index 846e31d0a2..9684e57b34 100644 > --- a/libavcodec/jpeg2000dec.c > +++ b/libavcodec/jpeg2000dec.c > @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) > static int get_ppt(Jpeg2000DecoderContext *s, int n) > { > Jpeg2000Tile *tile; > + void *new; > > if (n < 3) { > av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT data.\n"); > @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > > tile->has_ppt = 1; // this tile has a ppt marker > bytestream2_get_byte(&s->g); // Zppt is skipped and not used > - void* new = av_realloc(tile->packed_headers, > - tile->packed_headers_size + n - 3); > + new = av_realloc(tile->packed_headers, > + tile->packed_headers_size + n - 3); Please push such fixes directly, there is too much traffic already on this mailing list. Thank you, Carl Eugen
Carl Eugen Hoyos: > Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt > <andreas.rheinhardt@gmail.com>: >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/jpeg2000dec.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c >> index 846e31d0a2..9684e57b34 100644 >> --- a/libavcodec/jpeg2000dec.c >> +++ b/libavcodec/jpeg2000dec.c >> @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) >> static int get_ppt(Jpeg2000DecoderContext *s, int n) >> { >> Jpeg2000Tile *tile; >> + void *new; >> >> if (n < 3) { >> av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT data.\n"); >> @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) >> >> tile->has_ppt = 1; // this tile has a ppt marker >> bytestream2_get_byte(&s->g); // Zppt is skipped and not used >> - void* new = av_realloc(tile->packed_headers, >> - tile->packed_headers_size + n - 3); >> + new = av_realloc(tile->packed_headers, >> + tile->packed_headers_size + n - 3); > > Please push such fixes directly, there is too much traffic already > on this mailing list. > Ok. Should patches like the ones contained in the patchset [1] also be pushed directly? - Andreas [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/252107.html
Am Do., 9. Apr. 2020 um 18:35 Uhr schrieb Andreas Rheinhardt <andreas.rheinhardt@gmail.com>: > Ok. Should patches like the ones contained in the patchset [1] also be > pushed directly? > > - Andreas > > [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/252107.html Imo, after you tested them: Definitely. Carl Eugen
Quoting Carl Eugen Hoyos (2020-04-09 18:13:53) > Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt > <andreas.rheinhardt@gmail.com>: > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > --- > > libavcodec/jpeg2000dec.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > > index 846e31d0a2..9684e57b34 100644 > > --- a/libavcodec/jpeg2000dec.c > > +++ b/libavcodec/jpeg2000dec.c > > @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) > > static int get_ppt(Jpeg2000DecoderContext *s, int n) > > { > > Jpeg2000Tile *tile; > > + void *new; > > > > if (n < 3) { > > av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT data.\n"); > > @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > > > > tile->has_ppt = 1; // this tile has a ppt marker > > bytestream2_get_byte(&s->g); // Zppt is skipped and not used > > - void* new = av_realloc(tile->packed_headers, > > - tile->packed_headers_size + n - 3); > > + new = av_realloc(tile->packed_headers, > > + tile->packed_headers_size + n - 3); > > Please push such fixes directly, there is too much traffic already > on this mailing list. I disagree. IMO everything that goes into the repo should pass through the mailing list.
On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Carl Eugen Hoyos (2020-04-09 18:13:53) >> Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt >> <andreas.rheinhardt@gmail.com>: >> > >> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> > --- >> > libavcodec/jpeg2000dec.c | 5 +++-- >> > 1 file changed, 3 insertions(+), 2 deletions(-) >> > >> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c >> > index 846e31d0a2..9684e57b34 100644 >> > --- a/libavcodec/jpeg2000dec.c >> > +++ b/libavcodec/jpeg2000dec.c >> > @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) >> > static int get_ppt(Jpeg2000DecoderContext *s, int n) >> > { >> > Jpeg2000Tile *tile; >> > + void *new; >> > >> > if (n < 3) { >> > av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT >> > data.\n"); >> > @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) >> > >> > tile->has_ppt = 1; // this tile has a ppt marker >> > bytestream2_get_byte(&s->g); // Zppt is skipped and not used >> > - void* new = av_realloc(tile->packed_headers, >> > - tile->packed_headers_size + n - 3); >> > + new = av_realloc(tile->packed_headers, >> > + tile->packed_headers_size + n - 3); >> >> Please push such fixes directly, there is too much traffic already >> on this mailing list. > > I disagree. IMO everything that goes into the repo should pass through > the mailing list. > I 100% disagree.
Am Sa., 11. Apr. 2020 um 10:58 Uhr schrieb Anton Khirnov <anton@khirnov.net>: > > Quoting Carl Eugen Hoyos (2020-04-09 18:13:53) > > Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt > > <andreas.rheinhardt@gmail.com>: > > > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > > --- > > > libavcodec/jpeg2000dec.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > > > index 846e31d0a2..9684e57b34 100644 > > > --- a/libavcodec/jpeg2000dec.c > > > +++ b/libavcodec/jpeg2000dec.c > > > @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) > > > static int get_ppt(Jpeg2000DecoderContext *s, int n) > > > { > > > Jpeg2000Tile *tile; > > > + void *new; > > > > > > if (n < 3) { > > > av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT data.\n"); > > > @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > > > > > > tile->has_ppt = 1; // this tile has a ppt marker > > > bytestream2_get_byte(&s->g); // Zppt is skipped and not used > > > - void* new = av_realloc(tile->packed_headers, > > > - tile->packed_headers_size + n - 3); > > > + new = av_realloc(tile->packed_headers, > > > + tile->packed_headers_size + n - 3); > > > > Please push such fixes directly, there is too much traffic already > > on this mailing list. > > I disagree. IMO everything that goes into the repo should pass through > the mailing list. Fortunately, this is not how it's done here. Furthermore, I would strongly suggest you stop making such comments that are not helpful. Carl Eugen
Quoting Paul B Mahol (2020-04-11 10:59:34) > On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Carl Eugen Hoyos (2020-04-09 18:13:53) > >> Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt > >> <andreas.rheinhardt@gmail.com>: > >> > > >> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> > --- > >> > libavcodec/jpeg2000dec.c | 5 +++-- > >> > 1 file changed, 3 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > >> > index 846e31d0a2..9684e57b34 100644 > >> > --- a/libavcodec/jpeg2000dec.c > >> > +++ b/libavcodec/jpeg2000dec.c > >> > @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) > >> > static int get_ppt(Jpeg2000DecoderContext *s, int n) > >> > { > >> > Jpeg2000Tile *tile; > >> > + void *new; > >> > > >> > if (n < 3) { > >> > av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT > >> > data.\n"); > >> > @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > >> > > >> > tile->has_ppt = 1; // this tile has a ppt marker > >> > bytestream2_get_byte(&s->g); // Zppt is skipped and not used > >> > - void* new = av_realloc(tile->packed_headers, > >> > - tile->packed_headers_size + n - 3); > >> > + new = av_realloc(tile->packed_headers, > >> > + tile->packed_headers_size + n - 3); > >> > >> Please push such fixes directly, there is too much traffic already > >> on this mailing list. > > > > I disagree. IMO everything that goes into the repo should pass through > > the mailing list. > > > > I 100% disagree. 100%? Meaning you think no patches should go to the mailing list? Or only some specific patches are exempt? Based on what criterion? And who applies it? And most importantly, why?
Quoting Carl Eugen Hoyos (2020-04-11 11:06:44) > Am Sa., 11. Apr. 2020 um 10:58 Uhr schrieb Anton Khirnov <anton@khirnov.net>: > > Fortunately, this is not how it's done here. It is done the way we, as a whole community, decide it is done. In practice, many people send ALL of their patches to the ML. > > Furthermore, I would strongly suggest you stop making such > comments that are not helpful. What about my comment is "not helpful"? And how is you expressing your personal opinion different from me doing the same?
On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Paul B Mahol (2020-04-11 10:59:34) >> On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: >> > Quoting Carl Eugen Hoyos (2020-04-09 18:13:53) >> >> Am Do., 9. Apr. 2020 um 14:39 Uhr schrieb Andreas Rheinhardt >> >> <andreas.rheinhardt@gmail.com>: >> >> > >> >> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> >> > --- >> >> > libavcodec/jpeg2000dec.c | 5 +++-- >> >> > 1 file changed, 3 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c >> >> > index 846e31d0a2..9684e57b34 100644 >> >> > --- a/libavcodec/jpeg2000dec.c >> >> > +++ b/libavcodec/jpeg2000dec.c >> >> > @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int >> >> > n) >> >> > static int get_ppt(Jpeg2000DecoderContext *s, int n) >> >> > { >> >> > Jpeg2000Tile *tile; >> >> > + void *new; >> >> > >> >> > if (n < 3) { >> >> > av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT >> >> > data.\n"); >> >> > @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int >> >> > n) >> >> > >> >> > tile->has_ppt = 1; // this tile has a ppt marker >> >> > bytestream2_get_byte(&s->g); // Zppt is skipped and not used >> >> > - void* new = av_realloc(tile->packed_headers, >> >> > - tile->packed_headers_size + n - 3); >> >> > + new = av_realloc(tile->packed_headers, >> >> > + tile->packed_headers_size + n - 3); >> >> >> >> Please push such fixes directly, there is too much traffic already >> >> on this mailing list. >> > >> > I disagree. IMO everything that goes into the repo should pass through >> > the mailing list. >> > >> >> I 100% disagree. > > 100%? Meaning you think no patches should go to the mailing list? > > Or only some specific patches are exempt? Based on what criterion? And > who applies it? > And most importantly, why? Only trivial patches, like cosmetics of few lines, and this one above and functional patches that clean ups code. Why? Because it adds too much burden for real review work on this mailing list.
Am Sa., 11. Apr. 2020 um 11:27 Uhr schrieb Anton Khirnov <anton@khirnov.net>: > > Quoting Carl Eugen Hoyos (2020-04-11 11:06:44) > > Am Sa., 11. Apr. 2020 um 10:58 Uhr schrieb Anton Khirnov <anton@khirnov.net>: > > > > Fortunately, this is not how it's done here. > > It is done the way we, as a whole community, decide it is done. > In practice, many people send ALL of their patches to the ML. You do realize that this is completely orthogonal? > > Furthermore, I would strongly suggest you stop making such > > comments that are not helpful. > > What about my comment is "not helpful"? > And how is you expressing your personal opinion different > from me doing the same? Yes, this is - sadly - exactly the right question. Carl Eugen
Anton Khirnov (12020-04-11): > It is done the way we, as a whole community, decide it is done. In > practice, many people send ALL of their patches to the ML. I am with Anton on this one. Rules are not set in stone, “and even stone changes, anyway”. And I think too it would be much better if all patches were submitted to the mailing-list. To have many eyes spot small mistakes, if only typos in the commit message. To get heads-up about possible conflicting work. To suggests transversal enhancements, etc. It is not as if sending the mail and waiting a few days to push the branch will hinder anybody's work much. And if it does, well, 2005 called and they want their SVN habits back. Code maintainership is first and foremost a responsibility. It only gives rights to sustain that responsibility. Regards,
On 4/11/20, Nicolas George <george@nsup.org> wrote: > Anton Khirnov (12020-04-11): >> It is done the way we, as a whole community, decide it is done. In >> practice, many people send ALL of their patches to the ML. > > I am with Anton on this one. Rules are not set in stone, “and even stone > changes, anyway”. > > And I think too it would be much better if all patches were submitted to > the mailing-list. To have many eyes spot small mistakes, if only typos > in the commit message. To get heads-up about possible conflicting work. > To suggests transversal enhancements, etc. > > It is not as if sending the mail and waiting a few days to push the > branch will hinder anybody's work much. And if it does, well, 2005 > called and they want their SVN habits back. > > Code maintainership is first and foremost a responsibility. It only > gives rights to sustain that responsibility. > I own right to directly commit to any file in codebase I maintain. Because only I know such code well. Allowing various "nice" interruptions by sending patches to list is waste of everybody time.
Quoting Carl Eugen Hoyos (2020-04-11 11:56:40) > > > > Furthermore, I would strongly suggest you stop making such > > > comments that are not helpful. > > > > What about my comment is "not helpful"? > > And how is you expressing your personal opinion different > > from me doing the same? > > Yes, this is - sadly - exactly the right question. Which you just neglected to answer. I would strongly suggest you stop accusing me of being "not helpful" when you are not willing or able to state precisely how am I "not helpful".
Quoting Paul B Mahol (2020-04-11 11:29:40) > On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: > > 100%? Meaning you think no patches should go to the mailing list? > > > > Or only some specific patches are exempt? Based on what criterion? And > > who applies it? > > And most importantly, why? > > Only trivial patches, like cosmetics of few lines, and this one above In my experience the line between "trivial cosmetics" and "nontrivial changes" is very fuzzy and observer-dependent. Better to be safe and just send everything. It's not like there is a massive flood of trivial patches all the time. > and functional patches that clean ups code. Not sure what you mean here. "clean up" can mean refactoring patches, which can be highly fragile and should most certainly be reviewed when possible. > > Why? Because it adds too much burden for real review work on this mailing list. It's certainly worth considering how to structure patch submission better, so that people do not need to wade through piles of emails that don't concern them. E.g. splitting the mailing lists into per-library, or implementing some sort of a tagging system or moving to some kind of a merge-request system come to mind. But I do not think directly pushing patches without any possibility of review is a good solution to this.
On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Paul B Mahol (2020-04-11 11:29:40) >> On 4/11/20, Anton Khirnov <anton@khirnov.net> wrote: >> > 100%? Meaning you think no patches should go to the mailing list? >> > >> > Or only some specific patches are exempt? Based on what criterion? And >> > who applies it? >> > And most importantly, why? >> >> Only trivial patches, like cosmetics of few lines, and this one above > > In my experience the line between "trivial cosmetics" and "nontrivial > changes" is very fuzzy and observer-dependent. Better to be safe and > just send everything. It's not like there is a massive flood of trivial > patches all the time. > >> and functional patches that clean ups code. > > Not sure what you mean here. "clean up" can mean refactoring patches, > which can be highly fragile and should most certainly be reviewed when > possible. > >> >> Why? Because it adds too much burden for real review work on this mailing >> list. > > It's certainly worth considering how to structure patch submission > better, so that people do not need to wade through piles of emails that > don't concern them. E.g. splitting the mailing lists into per-library, > or implementing some sort of a tagging system or moving to some kind of > a merge-request system come to mind. But I do not think directly pushing > patches without any possibility of review is a good solution to this. > Disagree, there are people that just comment for just showing how they lack insight in actual code. Or they want to force someone into specific nits.
Paul B Mahol (12020-04-11):
> Or they want to force someone into specific nits.
Respectfully, I think things would go a lot better around here if people
were to assume good will instead of ill intent. We are all working for
the good of FFmpeg, here, or at least we are.
As for the merits of review, the strong correlation between patches
pushed without review and the appearance of new coverity errors is a
good argument for systematic reviews.
On 4/11/20, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12020-04-11): >> Or they want to force someone into specific nits. > > Respectfully, I think things would go a lot better around here if people > were to assume good will instead of ill intent. We are all working for > the good of FFmpeg, here, or at least we are. > > As for the merits of review, the strong correlation between patches > pushed without review and the appearance of new coverity errors is a > good argument for systematic reviews. I never found developer that have coverity in brain or run patches via it.
Paul B Mahol (12020-04-11):
> I never found developer that have coverity in brain or run patches via it.
Exactly. And I hope you have the insight to count yourself in these
numbers.
This is exactly why we need reviews: individually, we are fallible,
collectively we are still fallible, but much less so.
On the other hand, if among the collective there are elements who refuse
to cooperate, it harms the whole. It is sad to say, but if they are too
uncooperative, it is sometime better to isolate them from the whole,
however personally talented they may be.
On 4/11/20, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12020-04-11): >> I never found developer that have coverity in brain or run patches via >> it. > > Exactly. And I hope you have the insight to count yourself in these > numbers. > > This is exactly why we need reviews: individually, we are fallible, > collectively we are still fallible, but much less so. > > On the other hand, if among the collective there are elements who refuse > to cooperate, it harms the whole. It is sad to say, but if they are too > uncooperative, it is sometime better to isolate them from the whole, > however personally talented they may be. Look at yourself! You are extremist!
diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c index 846e31d0a2..9684e57b34 100644 --- a/libavcodec/jpeg2000dec.c +++ b/libavcodec/jpeg2000dec.c @@ -862,6 +862,7 @@ static int get_plt(Jpeg2000DecoderContext *s, int n) static int get_ppt(Jpeg2000DecoderContext *s, int n) { Jpeg2000Tile *tile; + void *new; if (n < 3) { av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPT data.\n"); @@ -879,8 +880,8 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) tile->has_ppt = 1; // this tile has a ppt marker bytestream2_get_byte(&s->g); // Zppt is skipped and not used - void* new = av_realloc(tile->packed_headers, - tile->packed_headers_size + n - 3); + new = av_realloc(tile->packed_headers, + tile->packed_headers_size + n - 3); if (new) { tile->packed_headers = new; } else
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/jpeg2000dec.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)