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

[Michael Niedermayer]

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
> 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.

sure, ill reword so its more clear what is wrong and resubmit

Thanks!

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170904/950cfb24/attachment.sig>