[FFmpeg-devel] [PATCH] avcodec/snowdec: Check ld/cbd/crd

[James Almer]

On 9/4/2017 6:48 AM, Michael Niedermayer wrote:
> On Mon, Sep 04, 2017 at 12:57:32AM +0000, Kieran Kunhya wrote:
>> On Mon, 4 Sep 2017 at 00:56 Michael Niedermayer <michael at niedermayer.cc>
>> wrote:
>>
>>> We have always printed error messages for the case that an error
>>> occured.
>>> Its unprofessional to fail decoding a file but not provide any
>>> hint as to why a failure occured.
>>>
>>> If we remove all error messages and just print a generic "failed
>>> decoding header" or "failed to decode frame"
>>> We would leave users wondering about each error and we would no longer
>>> have differentiated bug reports.
>>> There would be one very huge bug report about
>>> "Error while decoding stream #0:0: Invalid data found when processing
>>> input"
>>> Because thats all detail the user can get if the message is not in the
>>> binary.
>>> That bug report then would be marked invalid of course and would help
>>> neither users nor developers.
>>>
>>
>> We ask users to report bugs with "--loglevel 99" so this is irrelevant if
>> it's a DEBUG.
> 
> yes, true
> 
> 
>>
>>
>>> Iam not sure why error messages are since about a year or so
>>> considered controversial by some developers but not before
>>>
>>
>> In my case I get a lot of reports from users about ffmpeg spamming logs
>> with obscure warnings.
>> This happens on legal files as per my "SPS/PPS truncation" patch which
>> scares them into thinking the file is broken.
>>
>> The same goes with spamming logs when there is some data corruption with
>> warnings which are very obscure.
> 
> The messages from libavcodec are by the nature of it quite technical.
> Does showing these messages to your users have any value for them at
> all ?
> 
> AV_LOG_ERROR is documented as
>     /**
>     * Something went wrong and cannot losslessly be recovered.
>     * However, not all future data is affected.
>     */
>     #define AV_LOG_ERROR    16
> 
> This matches the case its used in.
> 
> I am not against using DEBUG level for detailed error messages, and
> ERROR for generic ones if thats what more people prefer.
> But for this to achive any usefull effect it has to be done
> consistently accross the codebase. ATM we more commonly use
> AV_LOG_ERROR for these.

What some people dislike here is not that you're adding new error
messages but that said error messages mean nothing even to developers in
general.

"Failed to allocate a frame size %dx%d", "RGB not supported in profile
%d", "Requested reference %d not available", "Profile %d is not yet
supported" are concise and informative. You ran out of memory, the file
reports invalid parameters, the file is missing data, the file contains
features not yet supported.
Be it an user or a dev, you can easily realize what went wrong.

However, "Invalid cbd %d", crd %d", "Invalid ld %d" mean nothing to
pretty much anyone except the person that wrote the relevant code. The
three are names for local variables you added as duplicate of another
three local variables for the purpose of doing range checks.
They are not even labels in the snow spec (assuming there's one), they
are just variable names that could as well have been x, y and z.

Maybe you could replace them with an error message that mentions
something like a block within a frame has out of range or otherwise
invalid data/values? Anything that actually informs whoever sees the
message that something is wrong in the file and not that some internal
variable has an invalid value.