[FFmpeg-devel] [Libav-user] [PATCH]Fix a crash in vaapi_encode if "." is not the decimal separator

[Mark Thompson]

On 02/08/16 18:39, Nicolas George wrote:
> Le sextidi 16 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit :
>> -    { "i_qfactor",      "1.0" },
>> -    { "i_qoffset",      "0.0" },
>> -    { "b_qfactor",      "1.2" },
>> -    { "b_qoffset",      "0.0" },
>> +    { "i_qfactor",      "1"   },
>> +    { "i_qoffset",      "0"   },
>> +    { "b_qfactor",      "6/5" },
>> +    { "b_qoffset",      "0"   },

Urgh :(  I did not consider this problem at all when writing the code.

I tested the patch and it works as intended.

> I think this is not correct at all.
> 
> First, the code should not crash with incorrect parameters, it should either
> return a proper error code or accept the values with a sane meaning.
> 
> Second, this is not fixing the issue, it is just working around it. These
> options can also be set by applications or users, and the point is supposed
> to work.
> 
> The correct fix for the parsing side of the issue would be either to
> document that the FFmpeg libraries cannot be used with LC_NUMERIC set to
> anything else than C/POSIX (and possibly add a check at init time) or change
> the code to use an unlocalized version of strtod().
> 
> I am rather in favour of the first solution, as anybody who
> setlocale(LC_NUMERIC) in their are idiots and will break much more than
> FFmpeg libraries.

I agree that this would be the best answer.  However, I'm not sure about the backward compatibility - with the current behaviour users can have scripts containing "1,2" in any numeric argument to ffmpeg (meaning 6/5 as here) and have it parsed correctly because of their locale settings.

Therefore, I'm inclined to go for the proposed workaround (or something similar*) for now, including backporting it to 3.1 for distributions such as Debian which are encountering it, and consider the underlying locale problem separately.

Thanks,

- Mark


* The default values here don't really matter: setting them to 1, 0, 1, 0 (respectively) might be cleaner as a way to duck the problem.  (I only defined them like this so that it does something vaguely reasonable when testing with ffmpeg with no options, the values themselves don't matter because any real use will set them manually.)