[FFmpeg-devel] [PATCH] avcodec/snowdec: Check intra block dc differences.

[James Almer]

On 9/6/2017 6:55 AM, wm4 wrote:
> On Tue, 5 Sep 2017 22:14:39 -0300
> James Almer <jamrial at 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 at 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.