On Thu, 26 Jan 2017 17:02:37 +0100 Michael Niedermayer <michae...@gmx.at> wrote:
> On Thu, Jan 26, 2017 at 06:43:45AM +0100, wm4 wrote: > > On Thu, 26 Jan 2017 03:20:02 +0100 > > Michael Niedermayer <michae...@gmx.at> wrote: > > > > > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > > > > On 26.01.2017 02:29, Ronald S. Bultje wrote: > > > > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > > > > > andreas.cadhal...@googlemail.com> wrote: > > > > > > > > > >> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com> > > > > >> --- > > > > >> libavformat/nistspheredec.c | 11 +++++++++++ > > > > >> 1 file changed, 11 insertions(+) > > > > >> > > > > >> diff --git a/libavformat/nistspheredec.c > > > > >> b/libavformat/nistspheredec.c > > > > >> index 782d1dfbfb..3386497682 100644 > > > > >> --- a/libavformat/nistspheredec.c > > > > >> +++ b/libavformat/nistspheredec.c > > > > >> @@ -21,6 +21,7 @@ > > > > >> > > > > >> #include "libavutil/avstring.h" > > > > >> #include "libavutil/intreadwrite.h" > > > > >> +#include "libavcodec/internal.h" > > > > >> #include "avformat.h" > > > > >> #include "internal.h" > > > > >> #include "pcm.h" > > > > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > > > > >> return 0; > > > > >> } else if (!memcmp(buffer, "channel_count", 13)) { > > > > >> sscanf(buffer, "%*s %*s %"SCNd32, > > > > >> &st->codecpar->channels); > > > > >> + if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { > > > > >> + av_log(s, AV_LOG_ERROR, "Too many channels %d > > > > > >> %d\n", > > > > >> + st->codecpar->channels, FF_SANE_NB_CHANNELS); > > > > >> + return AVERROR(ENOSYS); > > > > >> + } > > > > > > > > > > > > > > > I've said this before, but again - please don't add useless log > > > > > messages. > > > > > > > > I disagree that these log messages are useless and I'm not the only one > > > > [1]. > > > > > > +1 > > > > > > Log messages make debuging the code easier, as a developer i like to > > > know why something fails having a hard failure but no clear and easy > > > findable error message is bad > > > > > > > > > [...] > > > > -1 > > > > This kind of things bloat the code with rare corner cases. One point > > would be that this increases binary size (why do we even still have the > > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but > > the current use of the macro will barely even make a dent into the > > number of bytes "wasted" for optional strings, yet we bother). > > > > Another point is that code becomes unreadable if obscure corner cases > > take up most of the code. I think that's w worrisome direction we're > > taking here. > > While it doesnt apply to this patch here but, > Error messages in obscure checks that describe the error condition > in the source would make at least some checks easier to understand > than just a generic > "return AVERROR_INVALIDDATA" In my opinion, there's no choice but to debug such cases manually anyway. Whether you think that log messages help you better than breakpoints in a source debugger is a different question. > > > > When I debug FFmpeg API use, the thing that annoys me most is that I > > can't know where an error happens. I have to trace the origin manually. > > Error codes are almost always completely useless. This patchset adds a > > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% > > meaningless and resolves to an even worse error message when using > > av_err2str(). Adding an av_log to error error point would "work" (at > > least you could use grep), but isn't a good solution. > > I think the idea about something more informative than a int32 error > code did come up previously. > I certainly would be in favor of having an error value that could be > used to pinpoint the error location(s) and or function(s) it passed > through, this would be usefull for debguging in general We could probably return a struct that contains the __FILE__ etc. value of the error site, but this would probably be too over-engineered and intrusive for a real solution. Back to this patch and the core issue: I think there's no ideal solution. So balance is required. Don't add av_logs to every error, but add them to cases that are interesting and would be otherwise hard to find. I think fine grained error logging for read_header in a really obscure and tiny demuxer isn't an interesting/useful case. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel