I think we're talking past each other. You're asking me to demonstrate a current way in which NULL might be passed to this function. I don't think it's necessary to do that in order for this patch to be a good idea.
> Is this about something iam missing or a lib user passing invalidly > setup AVOptions ? (he should fix his code) Absolutely, he should fix his code. But let's say some code gets by code review that has a corner case whereby NULL can be passed. Isn't it better for that condition to be handled cleanly (as it was before a500b975) rather than causing undefined behaviour? Then the error will be reported to the user with a clear error message and can be diagnosed and fixed quickly. Currently, what happens in this case will be implementation-dependent, making it much more difficult to diagnose. To be clear, I do not have a specific case in mind that will currently pass NULL to this function. The point of this patch is to stop relying on all current and future callers to this function always being completely bug-free, but instead to handle any error condition properly, as it is at most of the other av_parse_video_rate callsites. Could you explain why you would not want to do that? Thanks Jack On Mon, Aug 3, 2020 at 11:27 PM Michael Niedermayer <mich...@niedermayer.cc> wrote: > On Mon, Aug 03, 2020 at 02:07:36PM +0100, Jack Haughton wrote: > > A NULL check in av_parse_video_rate() would certainly not be a bad idea. > It > > wouldn't (on its own) restore the NULL input robustness to > > set_string_video_rate() though, as any error value returned from > > av_parse_video_rate() would result in val being logged using %s, which is > > the whole problem that a500b975 was aiming to solve. > > > > av_parse_video_rate() is also called from a lot more places (most of > which > > already have an explicit NULL check immediately before the call), so > > changing the behaviour of that function by adding a NULL check > potentially > > has a lot more knock-on effects than simply restoring the previously > > existing robustness to set_string_video_rate(), which is also in keeping > > with the other av_parse_video_rate() callsites. > > > > You say that the caller 'should not' pass NULL to > set_string_video_rate(). > > Absolutely, agreed, but the point of checks is to handle unexpected > > situations. > > > If someone does pass NULL, as of a500b975 the response will be > > who is passing NULL in which way ? > this is not a public function > > You can achieve such a NULL input by seting up an invalid AVOption > but you dont seem to speak about that. > > please explain what sort of robustness you want to achieve here > Is this about something iam missing or a lib user passing invalidly > setup AVOptions ? (he should fix his code) or some FFmpeg internal > robustness or what else ? > > > > undefined behaviour, where previously it would have been cleanly handled > > with an explicit error code. I'd contend that this is a bad response. > > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If a bugfix only changes things apparently unrelated to the bug with no > further explanation, that is a good sign that the bugfix is wrong. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". -- Argon Design Ltd., Registration No. 06977690, a Broadcom Inc. company Registered in England with registered office at St John's Innovation Centre, Cowley Road, Cambridge, CB4 0WS _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
