From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 67CCD4BF07 for ; Thu, 18 Jul 2024 11:18:03 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 230B768DAC4; Thu, 18 Jul 2024 14:18:01 +0300 (EEST) Received: from xmbghk7.mail.qq.com (xmbghk7.mail.qq.com [43.163.128.53]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C69B468DA7F for ; Thu, 18 Jul 2024 14:17:15 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foxmail.com; s=s201512; t=1721301430; bh=1hlIzdSjCJ+0BEzpSQQXQJizQVajXXJGIOIAXd3y8RE=; h=From:Subject:Date:References:To:In-Reply-To; b=baoMZjYrsBl5R8yrBWFYdEkOrLxw0ouoAak+A1gZAHdUi2wCxLM4PgY7nA6zQz6C8 wGkoTtgtn2yLRuohy4tRpB6G8S6E8Z+rxxVqA9/sMtuS9XtsLh+mhxQpyv/rsw/Ncx 3fmFAHpizd2QAIv3Ey1X1rxrk8CJkudUvmxzFiRM= Received: from smtpclient.apple ([119.147.10.192]) by newxmesmtplogicsvrszc5-2.qq.com (NewEsmtp) with SMTP id 448B44E0; Thu, 18 Jul 2024 19:17:08 +0800 X-QQ-mid: xmsmtpt1721301428tli1wz7nj Message-ID: X-QQ-XMAILINFO: OZZSS56D9fAjRQzFW/NdoW/A6fJm7rcsGJh+et9Ex1pjpAdStXqCU9zkOkDUyI UITScA/LmlnXwRaWBcsTouC/j3pyAZcfU/54JeHCNsUKobluyx9+/A9L5bcUtuRFG+AznGVgdC/h k+EPMhm8YMi1n4UawPr+cYT6lqUdtNXiGiBPe8WO4S/Y2X7A/zuJLl0W8ZiBJHi4ZEiXnMr9//IU mIeqh7RAq51cRNpDTsO+CfwQGmEpnsHetqq4RpmYF30NmzVV4/F/lZz+shiPbLpLymrRgXYhc3X2 U14yrr6WvpeGG8qPjcBjXvhDJPppJPHbkqdj1cRb8cdlAtt8l0yNHXs4/l9LkQZ2YJx32Y4BekB9 xl5ISo0uDF95yEL81h0DIyac4EVIoYNR09gyf3fvn1NCrwuR1g4nD6prn02rzzsfePE59Ho9kIvt gz/gcYvplnawHJ2SzPZbO523CMQVYToYPJfTphijfyvq0wT2kqjPo3RN20yZbKbnvgpUe2j0AK74 aLxENKDTMu6SnOpR1UdzGL1VG7Yobu6qVtMpWy8l0QX4uUtc5veYlCi+rOEDGGOTkQ/HKut20PV4 AqdF7iMiHyqmdk0jYhOwkMy3NGJ3Th+nqaeSIrXnvJeC+BjeRQRfFdxw/mRqQ/dD8Y7dc49b/YM+ 427zdE+eFZufYqqzUN7g5PD8SU8DjbJvaq5RaGB23/1BMKFuigJ0MnoZl0nNU2mrXVMOXPTj4yku mnhvFmauXx1nH3stZWn8F27KoA6yin3oRV5IUe9sWMbUp/TOswv9dm6JcmbBHJ8dsQRkeT7x6NPF qu6kF9xqGsrwD8lY3u8ECzIP/ysvjqxhRUgMNpxFO3gUY0PePbjNJmyoQP2tKiOfn3HEDvfXY2l+ 6CEtAYspOXdtDN5yv691J8T41bMq7VCcM5jjBtcIRHoGv+tG9wj9oialSi9sDGE6AFsgt6zMRg0g 3/4FSljL1SFYb/+5Y4LppaXc+Fp0qljPDMxo1xK39quwHskt/pmQ== X-QQ-XMRINFO: OWPUhxQsoeAVDbp3OJHYyFg= From: Zhao Zhili Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\)) Date: Thu, 18 Jul 2024 19:16:58 +0800 References: <9e16396a-4ef2-fdb3-fce8-94e19a141473@passwd.hu> <20240716111543.2437488-1-ffmpeg-devel@pileofstuff.org> To: FFmpeg development discussions and patches In-Reply-To: X-OQ-MSGID: <37D2CFB6-09DB-4FF7-9D38-4EC527EBE492@foxmail.com> X-Mailer: Apple Mail (2.3774.500.171.1.1) Subject: Re: [FFmpeg-devel] [RFC PATCH v2 0/1] avutil/error: Provide better feedback about unknown error codes X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: > On Jul 18, 2024, at 17:26, Marton Balint wrote: > > > On Tue, 16 Jul 2024, Andrew Sayers wrote: > >> I'm having trouble managing this conversation. On one hand, you've brought up >> several important details that would need to be included in a new patch. >> On the other hand, I'm pretty sure we're talking past each other on the big >> problems, and need to start over. So let's fork the discussion. >> >> # First, let's haggle over some details >> >> The patch below fixes a number of small issues brought up by your comments... >> >> Error numbers are always expressed in the code as either uppercase hex numbers >> or FourCCs (or ThreeCCs, but you get the point). This patch prints error codes >> as hex, which is no less unintelligible for ordinary users, might make problems >> easier to find on Google, and will sometimes make them easier to grep for. >> >> Having said that, this patch prints non-negative numbers in decimal, >> because all bets are off if that manages to happen. >> >> A developer could create an error code that just happens to be valid ASCII. >> In that situation, the previous patch would have printed something like >> "Unrecognised error code \"~!X\"" occurred", which is worse than the current >> behaviour. This patch includes both (hex) number and name in those messages. >> >> This patch adds "please report this bug" for all unknown error messages. >> I'll cover the reasoning below, but the relevant detail is that the previous >> patch just gave users a different heiroglyphic before abandoning them. >> >> # Second, let's talk about the big picture >> >> Consider the following scenario: >> >> 1. a new developer adds some code to FFmpeg that calls an existing function >> 2. it turns out that function sometimes calls another function that >> returns a variety of internal error codes (FFERROR_REDO among others) >> 3. their testing uncovers a situation that intermittently returns an unknown >> error number, but they don't notice there are two different numbers >> 4. they spend a lot of time tracking down an error message based on a random >> number, and eventually fix "the" bug (actually one of two intermittent bugs) >> 5. the review doesn't catch the other bug, and the new code goes live >> 6. a user trips over the other bug and sees "Error number occurred" >> 7. the user wastes a lot of time trying to work out what they did wrong, >> badmouthing FFmpeg to anyone who will listen as they do >> 8. they eventually catch the attention of a developer >> 9. that developer spends a lot of time bisecting the bug >> 10. the new developer is expected to fix this patch, and feels like they're >> to blame for the whole situation >> >> An error message like "Unrecognised error code \"REDO\" occurred, please report >> this bug" would give the newbie a fighting chance to catch both bugs at step 3, >> would make step 4 much shorter, and would optimise steps 7-10 to almost nothing. >> >> Catching this in a fate test would involve checking for an unknown function >> returning an unknown number that gets reused in a context it's subtly >> inappropriate for. I have no idea where to begin with that, and anyway it >> wouldn't help a developer in the process of tracking down an intermittent bug. > > The fate test should be added for checking that all ffmpeg-specific errors (defined with AVERROR_ prefix in error.h) has a textual representation. That does not help the FFERROR_REDO case, but it does help if somebody adds a new AVERROR_xxx constant but forget to add the text counterpart for it. > >> >> As mentioned above, the v2 patch adds "please report this bug" in a few places. >> Any negative error code can be valid, but returning a raw error number is always >> a bug, so it's all the same to users - if they see this message, they're not >> expected to fix it themselves, they're expected to let us know. > > It is not necessarily a bug though. AVERROR values can be based on any system errno, and not all errno-s used by system libraries necessarily are supported by the platform strerrro_r() or our drop-in replacement if > that is not available. > > I still feel like you are adding a lot of code for questionable benefit, so I suggest the following simple change: > > diff --git a/libavutil/error.c b/libavutil/error.c > index 90bab7b9d3..f78c4b35b4 100644 > --- a/libavutil/error.c > +++ b/libavutil/error.c > @@ -20,6 +20,7 @@ > #define _XOPEN_SOURCE 600 /* XSI-compliant version of strerror_r */ > #include > #include > +#include "avutil.h" > #include "config.h" > #include "avstring.h" > #include "error.h" > @@ -126,7 +127,7 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size) > ret = -1; > #endif > if (ret < 0) > - snprintf(errbuf, errbuf_size, "Error number %d occurred", errnum); > + snprintf(errbuf, errbuf_size, "Error number %d (%s) occurred", errnum, av_fourcc2str(-errnum)); > } > > return ret; I like this version. > > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".