[FFmpeg-devel] [PATCH] avcodec/snowdec: Check ld/cbd/crd
michael at niedermayer.cc
Mon Sep 4 18:01:31 EEST 2017
On Mon, Sep 04, 2017 at 11:58:07AM -0300, James Almer wrote:
> 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
> "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.
sure, ill reword so its more clear what is wrong and resubmit
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 181 bytes
Desc: Digital signature
More information about the ffmpeg-devel