| Message ID | 20170904161144.11739-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 4 Sep 2017 18:11:44 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: Timeout > Fixes: 3142/clusterfuzz-testcase-5007853163118592 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> Wasn't this patch rejected before? Can't we just remove this codec? It has no use other than causing potential security issues and maintenance.
On 9/4/17, wm4 <nfxjfg@googlemail.com> wrote: > On Mon, 4 Sep 2017 18:11:44 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > >> Fixes: Timeout >> Fixes: 3142/clusterfuzz-testcase-5007853163118592 >> >> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > Wasn't this patch rejected before? No. > > Can't we just remove this codec? It has no use other than causing > potential security issues and maintenance. No.
On 9/4/2017 2:23 PM, wm4 wrote: > On Mon, 4 Sep 2017 18:11:44 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > >> Fixes: Timeout >> Fixes: 3142/clusterfuzz-testcase-5007853163118592 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > Wasn't this patch rejected before? Ronald rejected the previous version because of the obscure error messages. I suggested an alternative and Michael sent a new patch following it. If this is also not ok then please say so. That way the error messages can be removed before pushing the patch and we can go back to focus on things worth discussing. > > Can't we just remove this codec? It has no use other than causing > potential security issues and maintenance. I agree about removing the encoder, but the decoder is needed for existing streams. Unless everyone just argues it has no real world samples for being an avcodec-only toy codec. If you send a patch removing the encoder without also trying to remove all the things that currently depend on it (One or two filters i think) then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of testing, fate or otherwise, that might require a native encoder.
On Mon, Sep 04, 2017 at 02:36:45PM -0300, James Almer wrote: > On 9/4/2017 2:23 PM, wm4 wrote: > > On Mon, 4 Sep 2017 18:11:44 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > >> Fixes: Timeout > >> Fixes: 3142/clusterfuzz-testcase-5007853163118592 > >> > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > Wasn't this patch rejected before? > > Ronald rejected the previous version because of the obscure error > messages. I suggested an alternative and Michael sent a new patch > following it. > > If this is also not ok then please say so. That way the error messages > can be removed before pushing the patch and we can go back to focus on > things worth discussing. > > > > > Can't we just remove this codec? It has no use other than causing > > potential security issues and maintenance. > > I agree about removing the encoder, but the decoder is needed for > existing streams. Unless everyone just argues it has no real world > samples for being an avcodec-only toy codec. > > If you send a patch removing the encoder without also trying to remove > all the things that currently depend on it (One or two filters i think) > then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of > testing, fate or otherwise, that might require a native encoder. I find it a bit offensive that people suggest to remove the encoder i maintain. [...]
On Mon, 4 Sep 2017 20:16:20 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Sep 04, 2017 at 02:36:45PM -0300, James Almer wrote: > > On 9/4/2017 2:23 PM, wm4 wrote: > > > On Mon, 4 Sep 2017 18:11:44 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > >> Fixes: Timeout > > >> Fixes: 3142/clusterfuzz-testcase-5007853163118592 > > >> > > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > Wasn't this patch rejected before? > > > > Ronald rejected the previous version because of the obscure error > > messages. I suggested an alternative and Michael sent a new patch > > following it. > > > > If this is also not ok then please say so. That way the error messages > > can be removed before pushing the patch and we can go back to focus on > > things worth discussing. > > > > > > > > > Can't we just remove this codec? It has no use other than causing > > > potential security issues and maintenance. > > > > I agree about removing the encoder, but the decoder is needed for > > existing streams. Unless everyone just argues it has no real world > > samples for being an avcodec-only toy codec. > > > > If you send a patch removing the encoder without also trying to remove > > all the things that currently depend on it (One or two filters i think) > > then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of > > testing, fate or otherwise, that might require a native encoder. > > I find it a bit offensive that people suggest to remove the encoder i > maintain. Can I add my own unused fringe codec with no users, as long as I maintain it?
On 9/4/2017 3:16 PM, Michael Niedermayer wrote: > On Mon, Sep 04, 2017 at 02:36:45PM -0300, James Almer wrote: >> On 9/4/2017 2:23 PM, wm4 wrote: >>> On Mon, 4 Sep 2017 18:11:44 +0200 >>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>> >>>> Fixes: Timeout >>>> Fixes: 3142/clusterfuzz-testcase-5007853163118592 >>>> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> >>> Wasn't this patch rejected before? >> >> Ronald rejected the previous version because of the obscure error >> messages. I suggested an alternative and Michael sent a new patch >> following it. >> >> If this is also not ok then please say so. That way the error messages >> can be removed before pushing the patch and we can go back to focus on >> things worth discussing. >> > >>> >>> Can't we just remove this codec? It has no use other than causing >>> potential security issues and maintenance. >> >> I agree about removing the encoder, but the decoder is needed for >> existing streams. Unless everyone just argues it has no real world >> samples for being an avcodec-only toy codec. >> >> If you send a patch removing the encoder without also trying to remove >> all the things that currently depend on it (One or two filters i think) >> then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of >> testing, fate or otherwise, that might require a native encoder. > > I find it a bit offensive that people suggest to remove the encoder i > maintain. Sending a patch to remove something can mean the start of a discussion, and not that it will be inevitably applied. If you don't agree with it you can argue against the patch. It's what has happened plenty of times (There were like three attempts to remove libmpcodecs each with its own patch before it was finally accepted by everyone). So please, don't be offended. I'm sure plenty of people think the redspark demuxer is a waste of bits and I wouldn't find it odd if someone sends a patch to purge it. I would if anything argue against its removal. Also, this all is assuming wm4 actually sends a patch for this purpose.
Hi, On Mon, Sep 4, 2017 at 12:11 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > if(type){ > + int ld, cbd, crd; > pred_mv(s, &mx, &my, 0, left, top, tr); > - l += get_symbol(&s->c, &s->block_state[32], 1); > + ld = get_symbol(&s->c, &s->block_state[32], 1); > + if (ld < -255 || ld > 255) { > + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of range) > intra luma block DC difference %d\n", ld); > + return AVERROR_INVALIDDATA; > + } > + l += ld; > if (s->nb_planes > 2) { > - cb+= get_symbol(&s->c, &s->block_state[64], 1); > - cr+= get_symbol(&s->c, &s->block_state[96], 1); > + cbd = get_symbol(&s->c, &s->block_state[64], 1); > + crd = get_symbol(&s->c, &s->block_state[96], 1); > + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { > + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of > range) intra chroma block DC difference %d, %d\n", cbd, crd); > + return AVERROR_INVALIDDATA; > + } > + cb += cbd; > + cr += crd; > } I recognize the great improvements in your messages. They are much better than before. They are still not appropriate for display to end users. Please use ff_tlog(), as was suggested in the original thread. Ronald
On 9/4/2017 11:00 PM, Ronald S. Bultje wrote: > Hi, > > On Mon, Sep 4, 2017 at 12:11 PM, Michael Niedermayer <michael@niedermayer.cc >> wrote: > >> if(type){ >> + int ld, cbd, crd; >> pred_mv(s, &mx, &my, 0, left, top, tr); >> - l += get_symbol(&s->c, &s->block_state[32], 1); >> + ld = get_symbol(&s->c, &s->block_state[32], 1); >> + if (ld < -255 || ld > 255) { >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of range) >> intra luma block DC difference %d\n", ld); >> + return AVERROR_INVALIDDATA; >> + } >> + l += ld; >> if (s->nb_planes > 2) { >> - cb+= get_symbol(&s->c, &s->block_state[64], 1); >> - cr+= get_symbol(&s->c, &s->block_state[96], 1); >> + cbd = get_symbol(&s->c, &s->block_state[64], 1); >> + crd = get_symbol(&s->c, &s->block_state[96], 1); >> + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of >> range) intra chroma block DC difference %d, %d\n", cbd, crd); >> + return AVERROR_INVALIDDATA; >> + } >> + cb += cbd; >> + cr += crd; >> } > > > I recognize the great improvements in your messages. They are much better > than before. They are still not appropriate for display to end users. > Please use ff_tlog(), as was suggested in the original thread. Now that i looked at ff_tlog() closely it's only enabled if -DTRACE is used during compilation, and then it prints stuff at the trace log level, which is even lower than debug and used to print a bunch of assorted values that are not error log messages. The chances for a dev to ever use that is very low outside of very specific cases, seeing that even a sane h264 clip will trash the terminal with an endless amount of log lines. So if anything, use debug level instead of error in the av_log call.
On Mon, Sep 04, 2017 at 08:35:17PM +0200, wm4 wrote: [...] > > > > Can't we just remove this codec? It has no use other than causing > > > > potential security issues and maintenance. > > > > > > I agree about removing the encoder, but the decoder is needed for > > > existing streams. Unless everyone just argues it has no real world > > > samples for being an avcodec-only toy codec. > > > > > > If you send a patch removing the encoder without also trying to remove > > > all the things that currently depend on it (One or two filters i think) > > > then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of > > > testing, fate or otherwise, that might require a native encoder. > > > > I find it a bit offensive that people suggest to remove the encoder i > > maintain. > > Can I add my own unused fringe codec with no users, as long as I > maintain it? Is there any reason to be so obsessed with snow? There are plenty of other "fringe" codecs in the codebase that only one person in the world cared about 10 years ago. Snow is one of the rare wavelet codecs, and as a result is much more valuable than many random basic game flavored codecs. And somehow, no one ever mention those. I don't personally care about game codecs or snow myself (probably nobody does), but I don't support this snow/swscale/whatever-michael-did killing circle jerk. This really feels like some form of constant harassment (I'm not even talking about IRC), and that's not acceptable. Please people, chill out. I understand the frustration when someone doesn't understand something that feel obvious to "everyone" and keep insisting on it, but that doesn't mean you should enter in some form of obsession and vengeance about anything he did/does/will do. ...aaand here I am, back in the "Insane people defending Michael all the time" category. Regards,
On Tue, 5 Sep 2017 10:03:11 +0200 Clément Bœsch <u@pkh.me> wrote: > On Mon, Sep 04, 2017 at 08:35:17PM +0200, wm4 wrote: > [...] > > > > > Can't we just remove this codec? It has no use other than causing > > > > > potential security issues and maintenance. > > > > > > > > I agree about removing the encoder, but the decoder is needed for > > > > existing streams. Unless everyone just argues it has no real world > > > > samples for being an avcodec-only toy codec. > > > > > > > > If you send a patch removing the encoder without also trying to remove > > > > all the things that currently depend on it (One or two filters i think) > > > > then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of > > > > testing, fate or otherwise, that might require a native encoder. > > > > > > I find it a bit offensive that people suggest to remove the encoder i > > > maintain. > > > > Can I add my own unused fringe codec with no users, as long as I > > maintain it? > > Is there any reason to be so obsessed with snow? There are plenty of other > "fringe" codecs in the codebase that only one person in the world cared > about 10 years ago. Snow is one of the rare wavelet codecs, and as a > result is much more valuable than many random basic game flavored codecs. > And somehow, no one ever mention those. Those game codecs have actually some use. There are files in the wild that are available to many, and that aren't just demo/test files. But it's true that all these game codecs bloat the codebase, cause maintenance and security issues, and have little use. They barely have a justification to exist too. The only argument for snow is that it's a nice ideas. It might serve as some proof of concept. As a real codec, it appears to be unusable. > I don't personally care about game codecs or snow myself (probably nobody > does), but I don't support this snow/swscale/whatever-michael-did killing > circle jerk. This really feels like some form of constant harassment (I'm > not even talking about IRC), and that's not acceptable. Well, michael could just cut back on trying to force insane stuff. His obstinate attitude to force ridiculous patches and defending them like they're the only choice doesn't help. Even when we try to remove some of his sins, he'll defend it to death, no matter how crazy and stupid the code was (side data merging comes to mind). If you feel like what I'm doing is harassment, I can end my involvement with michael to avoid this - but only if you stop him from doing bad things. > Please people, chill out. I understand the frustration when someone > doesn't understand something that feel obvious to "everyone" and keep > insisting on it, but that doesn't mean you should enter in some form of > obsession and vengeance about anything he did/does/will do. > > ...aaand here I am, back in the "Insane people defending Michael all the > time" category. We don't really need more of that category.
Hi, On Mon, Sep 4, 2017 at 10:11 PM, James Almer <jamrial@gmail.com> wrote: > On 9/4/2017 11:00 PM, Ronald S. Bultje wrote: > > Hi, > > > > On Mon, Sep 4, 2017 at 12:11 PM, Michael Niedermayer > <michael@niedermayer.cc > >> wrote: > > > >> if(type){ > >> + int ld, cbd, crd; > >> pred_mv(s, &mx, &my, 0, left, top, tr); > >> - l += get_symbol(&s->c, &s->block_state[32], 1); > >> + ld = get_symbol(&s->c, &s->block_state[32], 1); > >> + if (ld < -255 || ld > 255) { > >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of range) > >> intra luma block DC difference %d\n", ld); > >> + return AVERROR_INVALIDDATA; > >> + } > >> + l += ld; > >> if (s->nb_planes > 2) { > >> - cb+= get_symbol(&s->c, &s->block_state[64], 1); > >> - cr+= get_symbol(&s->c, &s->block_state[96], 1); > >> + cbd = get_symbol(&s->c, &s->block_state[64], 1); > >> + crd = get_symbol(&s->c, &s->block_state[96], 1); > >> + if (cbd < -255 || cbd > 255 || crd < -255 || crd > > 255) { > >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of > >> range) intra chroma block DC difference %d, %d\n", cbd, crd); > >> + return AVERROR_INVALIDDATA; > >> + } > >> + cb += cbd; > >> + cr += crd; > >> } > > > > > > I recognize the great improvements in your messages. They are much better > > than before. They are still not appropriate for display to end users. > > Please use ff_tlog(), as was suggested in the original thread. > > Now that i looked at ff_tlog() closely it's only enabled if -DTRACE is > used during compilation, and then it prints stuff at the trace log > level, which is even lower than debug and used to print a bunch of > assorted values that are not error log messages. You're right, ff_dlog() seems better (see libavcodec/internal.h). I also agree with y'all, let's stop discussing this further, we're all working on real, actually important stuff, this particular issue really isn't worth our time... Ronald
On Tue, Sep 05, 2017 at 07:13:00AM -0400, Ronald S. Bultje wrote: > Hi, > > On Mon, Sep 4, 2017 at 10:11 PM, James Almer <jamrial@gmail.com> wrote: > > > On 9/4/2017 11:00 PM, Ronald S. Bultje wrote: > > > Hi, > > > > > > On Mon, Sep 4, 2017 at 12:11 PM, Michael Niedermayer > > <michael@niedermayer.cc > > >> wrote: > > > > > >> if(type){ > > >> + int ld, cbd, crd; > > >> pred_mv(s, &mx, &my, 0, left, top, tr); > > >> - l += get_symbol(&s->c, &s->block_state[32], 1); > > >> + ld = get_symbol(&s->c, &s->block_state[32], 1); > > >> + if (ld < -255 || ld > 255) { > > >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of range) > > >> intra luma block DC difference %d\n", ld); > > >> + return AVERROR_INVALIDDATA; > > >> + } > > >> + l += ld; > > >> if (s->nb_planes > 2) { > > >> - cb+= get_symbol(&s->c, &s->block_state[64], 1); > > >> - cr+= get_symbol(&s->c, &s->block_state[96], 1); > > >> + cbd = get_symbol(&s->c, &s->block_state[64], 1); > > >> + crd = get_symbol(&s->c, &s->block_state[96], 1); > > >> + if (cbd < -255 || cbd > 255 || crd < -255 || crd > > > 255) { > > >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of > > >> range) intra chroma block DC difference %d, %d\n", cbd, crd); > > >> + return AVERROR_INVALIDDATA; > > >> + } > > >> + cb += cbd; > > >> + cr += crd; > > >> } > > > > > > > > > I recognize the great improvements in your messages. They are much better > > > than before. They are still not appropriate for display to end users. > > > Please use ff_tlog(), as was suggested in the original thread. > > > > Now that i looked at ff_tlog() closely it's only enabled if -DTRACE is > > used during compilation, and then it prints stuff at the trace log > > level, which is even lower than debug and used to print a bunch of > > assorted values that are not error log messages. > > > You're right, ff_dlog() seems better (see libavcodec/internal.h). ff_dlog() is not part of a user build so a users bug report will not contain it at any verbose / debug level. If that is what the community wants, we can of course go that way. My oppinion is that its a bad idea for the future maintainability of the project to use ff_dlog() widely for error details. But its the communities project and neither mine nor yours. Its also the community that will have to live with the resulting codebase and set of bug reports ... [...]
Hi, On Tue, Sep 5, 2017 at 7:47 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > ff_dlog() is not part of a user build so a users bug report will not > contain it at any verbose / debug level. > That is intentional. The messages are not meant for end users, since they are not helpful or informative for them. Bug reports always ask for files, and therefore it will be trivial for us as experienced developers to reproduce the bugs (which we do anyway) with a debug build and -v debug to get the appropriate level of technical verbosity that results in information appropriate for developers while debugging such issues. Ronald
On Tue, Sep 05, 2017 at 07:53:49PM -0400, Ronald S. Bultje wrote: > Hi, > > On Tue, Sep 5, 2017 at 7:47 PM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > ff_dlog() is not part of a user build so a users bug report will not > > contain it at any verbose / debug level. > > > > That is intentional. The messages are not meant for end users, since they > are not helpful or informative for them. I think the only point we agree on is that we disagree. > > Bug reports always ask for files, and therefore it will be trivial for us > as experienced developers to reproduce the bugs (which we do anyway) with a > debug build and -v debug to get the appropriate level of technical > verbosity that results in information appropriate for developers while > debugging such issues. thanks for volunteering [...]
On 9/5/2017 5:38 AM, wm4 wrote: > On Tue, 5 Sep 2017 10:03:11 +0200 > Clément Bœsch <u@pkh.me> wrote: > >> On Mon, Sep 04, 2017 at 08:35:17PM +0200, wm4 wrote: >> [...] >>>>>> Can't we just remove this codec? It has no use other than causing >>>>>> potential security issues and maintenance. >>>>> >>>>> I agree about removing the encoder, but the decoder is needed for >>>>> existing streams. Unless everyone just argues it has no real world >>>>> samples for being an avcodec-only toy codec. >>>>> >>>>> If you send a patch removing the encoder without also trying to remove >>>>> all the things that currently depend on it (One or two filters i think) >>>>> then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of >>>>> testing, fate or otherwise, that might require a native encoder. >>>> >>>> I find it a bit offensive that people suggest to remove the encoder i >>>> maintain. >>> >>> Can I add my own unused fringe codec with no users, as long as I >>> maintain it? >> >> Is there any reason to be so obsessed with snow? There are plenty of other >> "fringe" codecs in the codebase that only one person in the world cared >> about 10 years ago. Snow is one of the rare wavelet codecs, and as a >> result is much more valuable than many random basic game flavored codecs. >> And somehow, no one ever mention those. > > Those game codecs have actually some use. There are files in the wild > that are available to many, and that aren't just demo/test files. But > it's true that all these game codecs bloat the codebase, cause > maintenance and security issues, and have little use. They barely have > a justification to exist too. Don't be one of those that go "h264/aac/mov is all ffmpeg should care about". > > The only argument for snow is that it's a nice ideas. It might serve as > some proof of concept. As a real codec, it appears to be unusable. > >> I don't personally care about game codecs or snow myself (probably nobody >> does), but I don't support this snow/swscale/whatever-michael-did killing >> circle jerk. This really feels like some form of constant harassment (I'm >> not even talking about IRC), and that's not acceptable. > > Well, michael could just cut back on trying to force insane stuff. His > obstinate attitude to force ridiculous patches and defending them like > they're the only choice doesn't help. Even when we try to remove some > of his sins, he'll defend it to death, no matter how crazy and stupid > the code was (side data merging comes to mind). Seeing that stuff is effectively deprecated, i don't think he defended it to death. Everyone argued it was a pointless feature, and he ultimately conceded. > > If you feel like what I'm doing is harassment, I can end my involvement > with michael to avoid this - but only if you stop him from doing bad > things. You're very critical of all his patches. In many cases you give a detailed technical explanation of why you disagree, but most times you just make a snarky comment or are otherwise kinda rude. Even when you were proven wrong (the runtime cpu flag stuff) you answer was "Then go and rewrite the entire dsp initialization infrastructure". What made you think that was a good reply? If anything, that's what whoever is trying to introduce the faulty behavior should do, not him that reported why it's faulty. I tried to find a common ground regarding the error messages in this thread an the other and almost got it. But then i fucked up by agreeing with you about removing snowenc which started this stupid branch of the discussion. So please, like Clement said, chill out. We gain nothing with all this and lose plenty. > >> Please people, chill out. I understand the frustration when someone >> doesn't understand something that feel obvious to "everyone" and keep >> insisting on it, but that doesn't mean you should enter in some form of >> obsession and vengeance about anything he did/does/will do. >> >> ...aaand here I am, back in the "Insane people defending Michael all the >> time" category. > > We don't really need more of that category. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Tue, 5 Sep 2017 22:14:39 -0300 James Almer <jamrial@gmail.com> wrote: > On 9/5/2017 5:38 AM, wm4 wrote: > > On Tue, 5 Sep 2017 10:03:11 +0200 > > Clément Bœsch <u@pkh.me> wrote: > > > >> On Mon, Sep 04, 2017 at 08:35:17PM +0200, wm4 wrote: > >> [...] > >>>>>> Can't we just remove this codec? It has no use other than causing > >>>>>> potential security issues and maintenance. > >>>>> > >>>>> I agree about removing the encoder, but the decoder is needed for > >>>>> existing streams. Unless everyone just argues it has no real world > >>>>> samples for being an avcodec-only toy codec. > >>>>> > >>>>> If you send a patch removing the encoder without also trying to remove > >>>>> all the things that currently depend on it (One or two filters i think) > >>>>> then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of > >>>>> testing, fate or otherwise, that might require a native encoder. > >>>> > >>>> I find it a bit offensive that people suggest to remove the encoder i > >>>> maintain. > >>> > >>> Can I add my own unused fringe codec with no users, as long as I > >>> maintain it? > >> > >> Is there any reason to be so obsessed with snow? There are plenty of other > >> "fringe" codecs in the codebase that only one person in the world cared > >> about 10 years ago. Snow is one of the rare wavelet codecs, and as a > >> result is much more valuable than many random basic game flavored codecs. > >> And somehow, no one ever mention those. > > > > Those game codecs have actually some use. There are files in the wild > > that are available to many, and that aren't just demo/test files. But > > it's true that all these game codecs bloat the codebase, cause > > maintenance and security issues, and have little use. They barely have > > a justification to exist too. > > Don't be one of those that go "h264/aac/mov is all ffmpeg should care > about". I didn't argue that at all. I'm arguing that FFmpeg probably shouldn't support a codec that was used by 1 or 2 released games, and for which we can't guarantee security. Obscure features are a pretty big and popular attack surface. That's very different from arguing that we should drop all codecs but 3. > > > > The only argument for snow is that it's a nice ideas. It might serve as > > some proof of concept. As a real codec, it appears to be unusable. > > > >> I don't personally care about game codecs or snow myself (probably nobody > >> does), but I don't support this snow/swscale/whatever-michael-did killing > >> circle jerk. This really feels like some form of constant harassment (I'm > >> not even talking about IRC), and that's not acceptable. > > > > Well, michael could just cut back on trying to force insane stuff. His > > obstinate attitude to force ridiculous patches and defending them like > > they're the only choice doesn't help. Even when we try to remove some > > of his sins, he'll defend it to death, no matter how crazy and stupid > > the code was (side data merging comes to mind). > > Seeing that stuff is effectively deprecated, i don't think he defended > it to death. Everyone argued it was a pointless feature, and he > ultimately conceded. He only "conceded" after I put all of my energy into it, and even then he was letting us know that he thought side data merging was great. And of course this whole thing was not without claiming that side data merging was part of the ABI, which is pure BS (but he probably did that so that I had even more work with creating separate version guards). Shit like this happens every fucking time. I'm tired of it, so excuse me if I sometimes get "rude". > > > > If you feel like what I'm doing is harassment, I can end my involvement > > with michael to avoid this - but only if you stop him from doing bad > > things. > > You're very critical of all his patches. In many cases you give a > detailed technical explanation of why you disagree, but most times you > just make a snarky comment or are otherwise kinda rude. > Even when you were proven wrong (the runtime cpu flag stuff) you answer I wasn't proven wrong in this case. You can change the CPU flags at runtime, but, as I pointed out, it obviously doesn't work if there's anything still "running". It shows that 1. runtime modification doesn't really work, i.e. it's an obscure case that didn't work before, and all the heckmeck Michael is creating is probably only passive aggressive BS to "prove" his point 2. the various API to change CPU flags are a bad idea, and we should go into the opposite direction It blows my mind that some developer argues FFmpeg should adapt to runtime CPU changes. This is obviously batshit, because it's highly complex, the rest of _all_ the code doesn't handle it, and the API is global mutable state anyway. Getting back to this bit: > You're very critical of all his patches. In many cases you give a > detailed technical explanation of why you disagree, but most times you > just make a snarky comment or are otherwise kinda rude. In general, Michael often tends to post new patches with exactly the same bad issues as past patches, even though there was lengthy discussion about them, how they were bad, and how Michael shouldn't do that. So excuse me if I don't repeat myself every time. We had this discussion about fine grained error messages for fuzzed samples before too. Several times. Michael doesn't learn, or most likely, doesn't want to learn. It's rude to ignore our comments. Why are you complaining about me again? > I tried to find a common ground regarding the error messages in this > thread an the other and almost got it. But then i fucked up by agreeing > with you about removing snowenc which started this stupid branch of the > discussion. So please, like Clement said, chill out. We gain nothing > with all this and lose plenty. What could we possibly lose at this point.
On 9/6/2017 6:55 AM, wm4 wrote: > On Tue, 5 Sep 2017 22:14:39 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 9/5/2017 5:38 AM, wm4 wrote: >>> On Tue, 5 Sep 2017 10:03:11 +0200 >>> Clément Bœsch <u@pkh.me> wrote: >>> >>>> On Mon, Sep 04, 2017 at 08:35:17PM +0200, wm4 wrote: >>>> [...] >>>>>>>> Can't we just remove this codec? It has no use other than causing >>>>>>>> potential security issues and maintenance. >>>>>>> >>>>>>> I agree about removing the encoder, but the decoder is needed for >>>>>>> existing streams. Unless everyone just argues it has no real world >>>>>>> samples for being an avcodec-only toy codec. >>>>>>> >>>>>>> If you send a patch removing the encoder without also trying to remove >>>>>>> all the things that currently depend on it (One or two filters i think) >>>>>>> then I'll +1 it. ffv1 and mpeg4 encoders are enough for any kind of >>>>>>> testing, fate or otherwise, that might require a native encoder. >>>>>> >>>>>> I find it a bit offensive that people suggest to remove the encoder i >>>>>> maintain. >>>>> >>>>> Can I add my own unused fringe codec with no users, as long as I >>>>> maintain it? >>>> >>>> Is there any reason to be so obsessed with snow? There are plenty of other >>>> "fringe" codecs in the codebase that only one person in the world cared >>>> about 10 years ago. Snow is one of the rare wavelet codecs, and as a >>>> result is much more valuable than many random basic game flavored codecs. >>>> And somehow, no one ever mention those. >>> >>> Those game codecs have actually some use. There are files in the wild >>> that are available to many, and that aren't just demo/test files. But >>> it's true that all these game codecs bloat the codebase, cause >>> maintenance and security issues, and have little use. They barely have >>> a justification to exist too. >> >> Don't be one of those that go "h264/aac/mov is all ffmpeg should care >> about". > > I didn't argue that at all. I'm arguing that FFmpeg probably shouldn't > support a codec that was used by 1 or 2 released games, and for which > we can't guarantee security. Obscure features are a pretty big and > popular attack surface. > > That's very different from arguing that we should drop all codecs but 3. > >>> >>> The only argument for snow is that it's a nice ideas. It might serve as >>> some proof of concept. As a real codec, it appears to be unusable. >>> >>>> I don't personally care about game codecs or snow myself (probably nobody >>>> does), but I don't support this snow/swscale/whatever-michael-did killing >>>> circle jerk. This really feels like some form of constant harassment (I'm >>>> not even talking about IRC), and that's not acceptable. >>> >>> Well, michael could just cut back on trying to force insane stuff. His >>> obstinate attitude to force ridiculous patches and defending them like >>> they're the only choice doesn't help. Even when we try to remove some >>> of his sins, he'll defend it to death, no matter how crazy and stupid >>> the code was (side data merging comes to mind). >> >> Seeing that stuff is effectively deprecated, i don't think he defended >> it to death. Everyone argued it was a pointless feature, and he >> ultimately conceded. > > He only "conceded" after I put all of my energy into it, Nicolas and others agreed with you that it was a pointless feature. Nobody sided with Michael's views on the feature, so it was a lost battle. > and even then > he was letting us know that he thought side data merging was great. And > of course this whole thing was not without claiming that side data > merging was part of the ABI, which is pure BS (but he probably did that > so that I had even more work with creating separate version guards). You have no reason to assume this kind of malice. At all. Also, had you also removed the automated merging parts of the code without waiting for a major bump, if a pre-removal lavf were to send a packet to a post-removal lavc, the latter would shit the bed with all the packets full of unexpected data it doesn't understand. The major bump patch i sent effectively gets rid of the ABI part of this feature as you scheduled it. It's no longer an issue. Maybe we should just bump major versions with every release instead and forget about backwards compatibility? libav essentially does that since they release once a year or less often. No two of their releases share major versions. It would certainly get rid of a lot of headaches for us. > > Shit like this happens every fucking time. I'm tired of it, so excuse > me if I sometimes get "rude". > >>> >>> If you feel like what I'm doing is harassment, I can end my involvement >>> with michael to avoid this - but only if you stop him from doing bad >>> things. >> >> You're very critical of all his patches. In many cases you give a >> detailed technical explanation of why you disagree, but most times you >> just make a snarky comment or are otherwise kinda rude. >> Even when you were proven wrong (the runtime cpu flag stuff) you answer > > I wasn't proven wrong in this case. You can change the CPU flags at > runtime, but, as I pointed out, it obviously doesn't work if there's > anything still "running". It shows that > > 1. runtime modification doesn't really work, i.e. it's an obscure case > that didn't work before, and all the heckmeck Michael is creating > is probably only passive aggressive BS to "prove" his point Of course it doesn't work, but nothing stops an API user to attempt it and make things crash. We could mention in the doxy that all these CPU flag functions should not be used after init (Basically, call it first or call it never). That way if the user missuses the API it's their own fucking problem. > 2. the various API to change CPU flags are a bad idea, and we should > go into the opposite direction Hendrik also agrees with you, but right now neither ffmpeg or libav have such plans, so maybe suggest a patch or approach to start a discussion. But personally, i find the -cpuflag option useful to test simd code as i write it. > > It blows my mind that some developer argues FFmpeg should adapt to > runtime CPU changes. This is obviously batshit, because it's highly > complex, the rest of _all_ the code doesn't handle it, and the API is > global mutable state anyway. It shouldn't. He only mentioned how volatile and prone to crashes the potential use of the new API was. As i said above, maybe just telling the user to refrain from doing so is enough? We've never gone the extra mile to stop users from doing things "wrong" by adding extra abstraction or whatever. The user is not a child. > > Getting back to this bit: > >> You're very critical of all his patches. In many cases you give a >> detailed technical explanation of why you disagree, but most times you >> just make a snarky comment or are otherwise kinda rude. > > In general, Michael often tends to post new patches with exactly the > same bad issues as past patches, even though there was lengthy > discussion about them, how they were bad, and how Michael shouldn't do > that. So excuse me if I don't repeat myself every time. > > We had this discussion about fine grained error messages for fuzzed > samples before too. Several times. Michael doesn't learn, or most > likely, doesn't want to learn. It's rude to ignore our comments. Why are > you complaining about me again? I agree that it seems he has ignored some of the request to remove unhelpful error messages. But i also agree with him that, currently, the tree is littered with error messages that go "$spec_field is out of range" or whatever, and that they have not been seen as an issue before. He basically argues that wanting to make new ones debug level is not consistent with the existing code, so the proper approach would be to change all of them to debug in the long run, or keep adding new ones as error (as long as they are at least understandable). I personally don't care. I just want this stupid discussion about error messages to end. > >> I tried to find a common ground regarding the error messages in this >> thread an the other and almost got it. But then i fucked up by agreeing >> with you about removing snowenc which started this stupid branch of the >> discussion. So please, like Clement said, chill out. We gain nothing >> with all this and lose plenty. > > What could we possibly lose at this point. Time, patience, hair, etc. Lot of things.
On Wed, 6 Sep 2017 11:33:05 -0300 James Almer <jamrial@gmail.com> wrote: > Also, had you also removed the automated merging parts of the code > without waiting for a major bump, if a pre-removal lavf were to send a > packet to a post-removal lavc, the latter would shit the bed with all > the packets full of unexpected data it doesn't understand. Yes, because that's such a common and well-working configuration. Only lesser Linux distros will do this anyway, and they don't matter. How many other changes did we make and ignore such issues completely? You also forget that pre-bump libavformat was still supposed to generate merged side data, because otherwise it would "break" applications. So basically the BS was rubbed in my face. > Maybe we should just bump major versions with every release instead and > forget about backwards compatibility? libav essentially does that since > they release once a year or less often. No two of their releases share > major versions. > It would certainly get rid of a lot of headaches for us. Yes, it would be an improvement over doing complicated work for absolutely no reason. > > What could we possibly lose at this point. > > Time, patience, hair, etc. Lot of things. I lost much of those.
Hi, On Wed, Sep 6, 2017 at 10:33 AM, James Almer <jamrial@gmail.com> wrote: > But i also agree with him that, currently, the > tree is littered with error messages that go "$spec_field is out of > range" or whatever, and that they have not been seen as an issue before. > He basically argues that wanting to make new ones debug level is not > consistent with the existing code, so the proper approach would be to > change all of them to debug in the long run, or keep adding new ones as > error (as long as they are at least understandable). I'm happy to help replace them with ff_dlog() or remove them, for the sake of resolving the "consistency" argument. Why care now? I don't mind minor issues, our tree is littered with them, but with the amount of changes Michael is making in this area, this isn't "minor" anymore, and it's growing. It's better to resolve such issues before it grows out of hand and we're stuck with another MpegEncContext or DspUtil. Ronald
On Wed, Sep 06, 2017 at 11:55:41AM +0200, wm4 wrote: > On Tue, 5 Sep 2017 22:14:39 -0300 > James Almer <jamrial@gmail.com> wrote: > > > On 9/5/2017 5:38 AM, wm4 wrote: > > > On Tue, 5 Sep 2017 10:03:11 +0200 > > > Clément Bœsch <u@pkh.me> wrote: > > > > > >> On Mon, Sep 04, 2017 at 08:35:17PM +0200, wm4 wrote: [...] > > > > > > The only argument for snow is that it's a nice ideas. It might serve as > > > some proof of concept. As a real codec, it appears to be unusable. > > > > > >> I don't personally care about game codecs or snow myself (probably nobody > > >> does), but I don't support this snow/swscale/whatever-michael-did killing > > >> circle jerk. This really feels like some form of constant harassment (I'm > > >> not even talking about IRC), and that's not acceptable. > > > > > > Well, michael could just cut back on trying to force insane stuff. His > > > obstinate attitude to force ridiculous patches and defending them like > > > they're the only choice doesn't help. Even when we try to remove some > > > of his sins, he'll defend it to death, no matter how crazy and stupid > > > the code was (side data merging comes to mind). > > > > Seeing that stuff is effectively deprecated, i don't think he defended > > it to death. Everyone argued it was a pointless feature, and he > > ultimately conceded. > > He only "conceded" after I put all of my energy into it, and even then > he was letting us know that he thought side data merging was great. This is a false statement about me Its my oppinion that side data merging gave FFmpeg a strategic advantage over Libav at the time it was added. Without a competition at the time i would not have supported adding it most likely. [...] Iam refraining from commenting on some of the snipped aggression above > Getting back to this bit: > > > You're very critical of all his patches. In many cases you give a > > detailed technical explanation of why you disagree, but most times you > > just make a snarky comment or are otherwise kinda rude. > > In general, Michael often tends to post new patches with exactly the > same bad issues as past patches, even though there was lengthy > discussion about them, how they were bad, and how Michael shouldn't do > that. So excuse me if I don't repeat myself every time. > > We had this discussion about fine grained error messages for fuzzed > samples before too. Several times. Michael doesn't learn, or most > likely, doesn't want to learn. It's rude to ignore our comments. Why are > you complaining about me again? I think you misunderstand or at least we have a different understanding of the situation In FFmpeg each piece of code has a maintainer. In fact thats true everywhere, theres always someone or more who does the work. And in general, not specific to FFmpeg The people doing the work most of the time have the authority about how to do it or they tend to be paid. In FFmpeg, the person doing the work has the final decission on how the code is written. Within the rules of our development policy. For snow, a few others and defacto jpeg2000 thats me jpeg 2000 is btw a terrible codec, i would be more than happy to give the maintainer hat to someone else. git shortlog -sn libavcodec/jpeg2000* 317 Michael Niedermayer 14 Luca Barbato 9 Carl Eugen Hoyos 9 Hendrik Leppkes 7 Nicolas Bertrand 6 Vittorio Giovara 4 James Almer 4 Janne Grunau 3 Diego Biurrun 3 Ganesh Ajjanagadde 3 Paul B Mahol 2 Anton Khirnov 2 Clément Bœsch 1 Derek Buitenhuis 1 Lou Logan 1 Lukasz Marek 1 Martin Storsjö 1 Reimar Döffinger If the mainatiner of some code has a preferrance that differs from your preferrance thats nothing to be upset about. And i stated repeatedly that iam happy to follow the community preferrance. But instead of an attempt to poll the community for everyones preferrance. there are 3 or 4 people who keep telling me with increased aggression to do what they say. That is with mails worded as if there was alot of authority behind them. I find these mails quite rude honestly. Rude toward the community and toward me. Thus you have the current situation where patches i write for code maintained by people who stated they want "no error messages" to have no such messages. (unless i forget) And patches writen for code maintained by me and others who didnt state anything often contain error messages. (unless i forget) Thats not ignoring people, i try to respect everyones preferrance in code they maintain. And i would wish they would respect mine in code i maintain, but some dont do that. If somone explicitly objects to a patch i have always refrained from pushing it. And it seems people very easily get upset if I propose changes to code i maintain that they previously told me they dont like. "Why" really is beyond my comperhension. Iam the one maintaining this code, and even have only proposed the changes in a posted patch. > > > I tried to find a common ground regarding the error messages in this > > thread an the other and almost got it. But then i fucked up by agreeing > > with you about removing snowenc which started this stupid branch of the > > discussion. So please, like Clement said, chill out. We gain nothing > > with all this and lose plenty. > > What could we possibly lose at this point. a lot anger and hostility rarely leads to good things [...]
diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c index b74c468ce3..ff28ec1d12 100644 --- a/libavcodec/snowdec.c +++ b/libavcodec/snowdec.c @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int level, int x, int y){ int my_context= av_log2(2*FFABS(left->my - top->my)) + 0*av_log2(2*FFABS(tr->my - top->my)); type= get_rac(&s->c, &s->block_state[1 + left->type + top->type]) ? BLOCK_INTRA : 0; - if(type){ + int ld, cbd, crd; pred_mv(s, &mx, &my, 0, left, top, tr); - l += get_symbol(&s->c, &s->block_state[32], 1); + ld = get_symbol(&s->c, &s->block_state[32], 1); + if (ld < -255 || ld > 255) { + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of range) intra luma block DC difference %d\n", ld); + return AVERROR_INVALIDDATA; + } + l += ld; if (s->nb_planes > 2) { - cb+= get_symbol(&s->c, &s->block_state[64], 1); - cr+= get_symbol(&s->c, &s->block_state[96], 1); + cbd = get_symbol(&s->c, &s->block_state[64], 1); + crd = get_symbol(&s->c, &s->block_state[96], 1); + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { + av_log(s->avctx, AV_LOG_ERROR, "Invalid (Out of range) intra chroma block DC difference %d, %d\n", cbd, crd); + return AVERROR_INVALIDDATA; + } + cb += cbd; + cr += crd; } }else{ if(s->ref_frames > 1)
Fixes: Timeout Fixes: 3142/clusterfuzz-testcase-5007853163118592 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/snowdec.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)