[FFmpeg-devel] [PATCH] ffmdec: sanitize codec parameters

[Carl Eugen Hoyos]

2016-11-22 0:06 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun at googlemail.com>:
> On 20.11.2016 21:02, Carl Eugen Hoyos wrote:
>> 2016-11-20 20:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun at googlemail.com>:
>>> Currently many demuxers silently accept wrong (i.e. negative) values
>>> for channels, bit_rate, block_align and so on. I'd like to fix that,
>>> so the question is now, how?
>>>
>>> There are a few possibilities:
>>> a) error out for negative values and also for zero
>>
>> Only if -fstrict strict was set.
>>
>>> b) error out for negative values, silently accept zero
>>
>> Only if -fstrict strict was set.
>
> So you have no preference between accepting zero or not?

Sorry:
By default, FFmpeg should not error out because of an
invalid value in some field, neither if it is zero although
it should be positive nor when it is negative.

> I tend to silently accept zero, because I'd like a
> consistent solution and in some cases rejecting zero
> causes FATE failures.

If there were a bug, you should of course fix fate, fate
alone is generally no argument in favour or against a patch.

> If a zero causes SIGFPE crashes, it obviously needs to
> be rejected with an error.

The crash has to be fixed.

If a simple fix is possible that allows to decode such samples,
it should be preferred over a fix that doesn't allow it.

> I also like the idea to only error out if strict is set
> and otherwise only warn.
>
>>> c) warn for negative values and also for zero
>>> d) warn for negative values, silently accept zero
>>
>> I obviously cannot stop you if you feel this should
>> be done, but note that users will report regressions
>> "warnings are shown for playable files".
>
> Isn't that a good thing?

I tried to explain above why it is not necessarily a good thing.

> Because either our demuxer has a bug and should be fixed,
> or some other tool created broken files, and if ffmpeg
> informs it's users about that, they can try to get that
> tool fixed.

(Träum weiter)
Seriously: We have an uncountable amount of files that
do not respect some "specification", many of them apparently
by intention, many written by applications that do not exist
since a long time, and some written by applications that have
fixed the issue meanwhile: Of course it can be a (very) good
thing to print warnings but in general, we should try to fix a
few of the thousands of known bugs in FFmpeg and not
spend infinite time on possible issues in other applications.

Sorry if I just misunderstand you.

Carl Eugen